Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add FPM::Util.execmd and fix python_spec.rb #1091

Merged
merged 1 commit into from
Apr 28, 2016

Conversation

ptomulik
Copy link
Contributor

Created mainly to fix the issue spotted here.

@hatt
Copy link
Contributor

hatt commented Apr 13, 2016

I might be missing something but this looks a bit more like it's replacing what the safesystem wrapper is doing, maybe better to update/patch it for the extra functionality?

@ptomulik
Copy link
Contributor Author

I can't imagine how to provide special argument with the input string (for stdin) to safesystem, as its interface is

def safesystem(*args)

and all args are treated as command-line arguments (except the first, which is a command name). I think, execmd is quite more generic, so safesystem could internally call execmd and preserve its interface.

@ptomulik
Copy link
Contributor Author

See 7b2d139

django_bindir = %x{python -c 'from distutils.sysconfig import get_config_var; print(get_config_var("BINDIR"))'}.chomp
django_bindir = nil
# Determine, where 'easy_install' is going to install scripts
execmd('python', code_easy_install_default_script_dir) do |code,out,err|
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an alternative, you could include this python script in fpm similar to the way the python package support already does this --

python code and how it's invoked in fpm.

Since this is just for a test, the python code could live in a file in spec/fixtures/ somewhere (common in other similar ruby projects) and you can invoke safesystem("python spec/fixtures/default_script_dir.py") possibly capturing the output to a file like done here

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, it may be good to leave execmd as, in opposite to safesystem* ones, it provides all standard communication channels to the executed command (stdin, stdout, stderr, exit_status). As you may se, I managed to implement both versions of safesystem* on top of execmd. I'm ok with moving the python code to fixture file (this could push me to organize it little bit better), but the actuall script consists of two pieces -- definition of a class and a code that uses this class. I think, the usage code (second part) is not worth giving it a fixture (it's a very small piece of code, and I don't think it's reusable), but hopefully it can be just passed to python interpreter via CLI.

@ptomulik ptomulik force-pushed the fix/python_spec branch 2 times, most recently from 5eced2e to 69dfd8e Compare April 14, 2016 23:13
@ptomulik
Copy link
Contributor Author

ptomulik commented Apr 14, 2016

So, I made changes, according to previous proposition. The new execmd provides convenient and still powerful interface to ChildProcess. It allows passing environment variables to child process and communicating it via pipes (stdin, stdout and stderr) . Also squashed all to a single commit.

@hatt
Copy link
Contributor

hatt commented Apr 15, 2016

Looks good to me!

# Determine default value of a given easy_install's option
def easy_install_default(option)
result = nil
execmd({:PYTHONPATH=>"#{example_dir}"}, 'python', :stderr=>false) do |stdin,stdout|
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using "example_dir" here may look little-bit out of context, but it should be fine as long as the function is used within the describe FPM:Package::Python... block.

@jordansissel
Copy link
Owner

Specs are passing with this PR, so I will merge.

@jordansissel jordansissel merged commit dcdec1a into jordansissel:master Apr 28, 2016
jordansissel added a commit that referenced this pull request Jun 20, 2016
add FPM::Util.execmd and fix python_spec.rb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants