-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
added: --install-option on requirements file #271 #515
Conversation
Someone asked about it in StackOverflow: http://stackoverflow.com/questions/10278708/pip-pass-custom-options-to-installer-with-requirements-txt After merging it would be nice to write an answer there. |
@hltbra, I already commented the question in stackoverflow saying about this pull request. |
@@ -81,6 +83,15 @@ def from_line(cls, name, comes_from=None): | |||
path = os.path.normpath(os.path.abspath(name)) | |||
link = None | |||
|
|||
inst_opt_pos = name.find('--install-options=') | |||
# should we use `argparse` or safer method instead? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If anyone could review this bad comment line and tell me if there's a better way to go about this I can go and fix it. argparse
seemed too complicated.
Example requirements.txt and caveats
pyzmq>=2.2.0 --install-options="--zmq=/usr/local/lib"
the string --install-options=
is searched for on the line, and chomped. Doesn't do any error checking, we just grab whatever is inside the quotes. There's no escaping double-quotes, or spaces... which is the next split() in L#90 so you can pass multiple parameters.
So, you could have:
--install-options="--libjpeg=/usr/local/lib --libpng=/usr/local/lib"
but
--install-options="--libjpeg=/home/conrado/library\ builds/"
will fail in a very bad way. So, is this acceptable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pull request.
I'd say we definitely should have tests around this, and this should be part of your pull request. If you need a hand with that just ask and I'll try help.
So shell parsing and escaping is tricky. For --install-options="--libjpeg=/home/conrado/library\ builds/"
does --install-options="--libjpeg=/home/conrado/library\\ builds/"
work?
Also have you looked at shlex
in the stdlib as this may help with splitting
Also we'd definitely want to support this both in requirements.txt and on the command line, so this is probably the wrong place for the support to be added.
as for parsing the options from the req, pip is already using optparse internally, and it's supported across all the python versions pip supports here's a way to realiably parse the req using that. import optparse the result: |
This is a thought I'm having that I want to preserve in the issue - thinking of other approaches to how to do this, eg we could on the command line using the standard A quick test shows this seems to work on 2.4 and 2.7. I'm not totally sold on that though so import optparse
parser = optparse.OptionParser()
parser.add_option('--noarg', action="store_true", default=False)
parser.add_option('--witharg', action="store", dest="witharg")
parser.add_option('--witharg2', action="store", dest="witharg2", type="int")
print parser.parse_args([ '--noarg', '--witharg', 'val', '--witharg2=3' ])
(<Values at 0x106018128: {'noarg': True, 'witharg': 'val', 'witharg2': 3}>, [])
print parser.parse_args([ '--noarg', '--witharg', 'val', '--', '--witharg2=3' ])
(<Values at 0x106018248: {'noarg': True, 'witharg': 'val', 'witharg2': None}>, ['--witharg2=3']) |
The issue here is that the argument parsing from shell and from the Question on functionality, should we have all pip shell options available to |
So from my perspective requirements.txt should be able to express everything that is required to install a package. If we need to pass specific options through to setup.py. @ianb has added another way to do this in #519 although I think you're right we should take a step back and think about what we need to support to fully work. Other cases such as @ niedbalski adding subdirectory support but via the URL #526 point to the need to handle the non trivial cases where just running setup.py won't work. |
yes, reusing the IntallCommand parser somehow and applying it to spec lines in requirements.txt (and overriding the CLI options) would be nice. |
I have looked into getting more options supported, but it really does need some work on InstallCommand, RequirementSet, possibly parse_requirements. The way it is now it should work fine as shlex is used to parse the arguments. |
Hey all - I don't have strong opinions here. Ideally we'd clean up requirements file parsing in general to not be so janky, and be able to offer a general "any option you can use on the command line you can use in a requirements file" guarantee, but that's going to be a sizable chunk of work and I'm not sure it's the highest priority. In practical terms I'm not too concerned about duplicating option names; we shouldn't really be changing them anyway for back-compat reasons. In this pull request, it seems odd to me to reuse the InstallCommand option parser when really the only thing we are supporting is install_option. I think I'd prefer to go with @qwcode 's suggestion and just use optparse directly to look for the only option we currently support here. |
the "reusing the IntallCommand parser" comment was about the possibility of supporting all or most of pip.commands.install options in req files, but not meant to block getting in --install-option support. |
@qwcode I know, but the current state of the pull request does already reuse the InstallCommand parser, even though none of the options are respected apart from |
@carljm , ok, I follow now. didn't really analyze how different this new commit was. more concerned about having tests for it. |
from pip.commands.install import InstallCommand | ||
parser = InstallCommand().parser | ||
(options, args) = parser.parse_args(line_arguments[1:]) | ||
name = line_arguments[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not following why this is just in the else block? and also not supported for editables.
e.g. we wouldn't be supporting install options for any of these forms
http://stuff.com/my_stash/package.tar.gz
git+http://github.com/frank/frodo@tag#egg=frodo
-e git+http://github.com/frank/frodo@tag#egg=frodo
I had a concern when it came to sifting through the RequirementSet and overriding options passed through the command line. What has authority and how should it behave? In fact, I think I need to look more at the current -r options. |
closing this due to #790. will be reviewing that in a few days. |
Hi,
I'm not really sure this is the best way about it... but it works.
Bye.