Add --install-options and --global-options to the requirements file parser. #2537

gvalkov · 2015-03-14T21:46:38Z

Hello,

This PR implements the functionality requested in #271 and #571. It is a rebase and cleanup of my previous attempt at implementing this - #790.

Notes:

The requirements file parser was overhauled and is best reviewed as a new file.
I'm not flake8 compliant with regard to max line length. Please consider bringing the recommended maximum up to 100-110.

Comments are welcome.

pfmoore · 2015-03-15T08:54:51Z

Build failures look mostly like compatibility issues python 2 vs python 3 (or 2.6 vs 2.7). I'll try to get a chance to do a more complete review in the next couple of days.

qwcode · 2015-03-16T02:42:13Z

I'll try to look too @pfmoore. kudos to @gvalkov for coming back again on this.

msabramo · 2015-03-16T14:01:51Z

Yes, thanks @gvalkov!

Not sure if this is Python version specific or just intermittent depending on dict ordering or something; leaning towards latter.

Has merge conflicts now too unfortunately, because of lots of activity.

gvalkov · 2015-03-16T19:26:23Z

Thanks everyone - I'm just happy to contribute to pip.

The PR has been rebased and force pushed. However, the req_file.py cleanup from #2497 was incompatible with the new parser and had to be reverted.

qwcode · 2015-03-18T02:25:00Z

pip/req/req_install.py

@@ -48,7 +48,7 @@ class InstallRequirement(object):

    def __init__(self, req, comes_from, source_dir=None, editable=False,
                 link=None, as_egg=False, update=True, editable_options=None,
-                 pycompile=True, markers=None, isolated=False):
+                 pycompile=True, markers=None, isolated=False, options=None):


just set options={} here and no if/else below

Done. I simply wanted to avoid the discussion about mutable default arguments.

ah... I see. yea, I honestly wasn't thinking about that. ok in this case, but I'm happy either way. I'd almost expect pylint/pep8 to have added a check for mutable kwargs?

qwcode · 2015-03-18T03:16:01Z

--install-option and --global-option are additive options. that's not working

my requirements.txt

peppercorn --global-option='--author' --global-option='--author-email'

the resulting command

Running command /home/qwcode/.qwdev/pip2/bin/python -c "import setuptools, tokenize;__file__='/tmp/pip-build-hp93Dz/peppercorn/setup.py';exec(compile(getattr(tokenize, 'open', open)(__file__).read().replace('\r\n', '\n'), __file__, 'exec'))" --author-email install --record /tmp/pip-Ev5FPG-record/install-record.txt --single-version-externally-managed --compile --install-headers /home/qwcode/.qwdev/pip2/include/site/python2.7

``

qwcode · 2015-03-18T03:20:06Z

pip/req/req_file.py

+    if flags:
+        long_opts += [i.lstrip('-') for i in flags]
+
+    opts, _ = getopt.getopt(shlex.split(req_line), '', long_opts)


why not build a mini pip parser?
reuse cmdoptions.install_options and cmdoptions.global_options

gvalkov · 2015-03-18T20:52:32Z

Thank you for the first wave of reviews - I've addressed some of the issues and am looking into the rest. The branch has also been rebased on top of the latest (slightly conflicting) develop.

gvalkov · 2015-03-18T23:38:35Z

Sigh - It was this trailing comma:

elif linetype == REQUIREMENT_EDITABLE:
    comes_from = '-r %s (line %s)' % (filename, line_number)
    isolated = options.isolated_mode if options else False,
    default_vcs = options.default_vcs if options else None,

False, == (False,) 
bool((False,)) is True

If isolated is true, then --no-user-cfg is added to the global options list.

gvalkov · 2015-03-21T17:58:04Z

Hello,

@pfmoore, I added the functional test that you requested.

At this point all tests pass, with the exception of the flake8 line-length ones. I'm going to make a small stand for 100 char lines. Imho, they improve readability in pip's case. Let me know if you're willing to consider such a change.

Also, let me know if you would like to have all commits squashed if you consider this ready for merging.

Thanks,
Georgi

pfmoore · 2015-03-21T19:15:22Z

Regarding the 100-character lines, we have a project policy of following PEP 8, so these would need to be fixed.

Otherwise, I'll take a look, probably tomorrow. No problem from me with not squashing the commits.

pfmoore · 2015-03-22T12:50:13Z

This looks OK to me, and I'm happy to merge it. Before I do - @qwcode you mentioned the parsing code in a comment ("why not build a mini pip parser?"). I don't know if you have any concerns in this area - we're adding a big chunk of parsing code here, but I don't really see any real problem with the code here, at the end of the day we need to parse the new syntax. Can you give a +1 if you're OK for this to go, and I'll merge.

@gvalkov thanks for all your work on this, it'll be a nice improvement for pip.

qwcode · 2015-03-22T15:17:52Z

pip/req/req_file.py

+    opts, _ = _req_parser.parse_args(args)
+
+    if opts.install_options:
+        opts.install_options = flat_shlex_split(opts.install_options)


I don't see that the extra flatten step is needed.

if args is --global-option='--author' --global-option='--author-email', then opts.global_options is ['--author', '--author-email'].

you'd only need to flatten if people were misusing "--global-option" and adding multiple options into it?

Agreed - supplying --global-option='--author --author-email' should be an error rather than passing 2 arguments. At least that's how I believe the command line --global-option works (the docs aren't that clear) and it's what I think makes sense.

Alright - the shlex.split() and flatten step has been removed. I think the old behavior had a nice DWIM thing going for it, though.

Should --global-option='--author --author-email' really be handled by pip? The way I see it, the value of a single --global|install-option should by definition be passes as a single argument to setup.py.

…onfidence with unit tests that confirm what args are passed

… finder, joining lines,

gvalkov · 2015-04-12T22:14:16Z

Sure, no problem. Merging @qwcode branch would have resulted in some messy history, so I went ahead and rebased with develop and then applied @qwcode's commits on top (sans @qwcode's merge). It looks cleaner when it's linear, imho.

Add --install-options and --global-options to the requirements file parser.

pfmoore · 2015-04-12T22:24:00Z

Looks good - thanks for the contribution!

… pip yet We have to wait for a new version of PIP: pypa/pip#2537

gvalkov force-pushed the reqfileopts branch 3 times, most recently from 67fca3f to 718f12b Compare March 15, 2015 01:02

gvalkov mentioned this pull request Mar 15, 2015

Add --install-options and --global-options to requirement parser + refactor #790

Closed

gvalkov force-pushed the reqfileopts branch 2 times, most recently from 898a34e to 7fef584 Compare March 16, 2015 19:18

qwcode reviewed Mar 18, 2015
View reviewed changes

gvalkov force-pushed the reqfileopts branch from e1a4c4e to ed14334 Compare March 18, 2015 20:47

gvalkov force-pushed the reqfileopts branch from a7c7ff9 to 1924776 Compare March 21, 2015 17:35

gvalkov force-pushed the reqfileopts branch from cdb4ff7 to fcc415f Compare March 21, 2015 23:30

msabramo mentioned this pull request Mar 22, 2015

Allow to specify per-line --install-option in requirements.txt #271

Closed

qwcode reviewed Mar 22, 2015
View reviewed changes

gvalkov and others added 14 commits April 12, 2015 23:52

Remove misleading test cases and improve docs

0da51b1

remove the functional test. for this change, we can achieve as much c…

351d809

…onfidence with unit tests that confirm what args are passed

create new test module for req_file.py and stub out the classes

d051669

tests for pip.req.req_file

e26d67c

move some tests from test_req to test_req_file

6c747be

consolidate tests from test_req to test_req_file

90f1791

replace use of obscure '--one-two-3' option in test

1294c0d

assert the order of the install/global options in the install call

9159a1d

pep8 fixes

81a41e4

add tests for raising RequirementsFileParseError

480a6f4

functional test for --install-option in a requirements file

12eab18

tests for: isolated mode, default vcs, ignoring comments, setting the…

fc5c6e5

… finder, joining lines,

remove comment about setuppyargs test package

9f88946

fix join_lines test

3315242

gvalkov force-pushed the reqfileopts branch from af4ce8a to 3315242 Compare April 12, 2015 22:12

pfmoore added a commit that referenced this pull request Apr 12, 2015

Merge pull request #2537 from gvalkov/reqfileopts

3b4373c

Add --install-options and --global-options to the requirements file parser.

pfmoore merged commit 3b4373c into pypa:develop Apr 12, 2015

qwcode added a commit that referenced this pull request Apr 13, 2015

update changelog for #2537

b5762ca

xavfernandez mentioned this pull request Apr 13, 2015

Determine what should be done about --(install|global)-option with Wheels #2677

Open

gvalkov mentioned this pull request Apr 13, 2015

Cleanup and abstract setup.py command construction in req_install.py #2678

Closed

qwcode mentioned this pull request Apr 18, 2015

Refactor requirements file parsing #2697

Merged

qwcode mentioned this pull request May 1, 2015

fix for nested requirements files #2742

Merged

JohannesBertens added a commit to witlox/cumulonimbi that referenced this pull request May 25, 2015

Removed the fancy stuff from requirements.txt - it's not supported by…

21b0117

… pip yet We have to wait for a new version of PIP: pypa/pip#2537

soltanmm mentioned this pull request Jun 24, 2015

Python protobuf doesn't use C++ extensions by default grpc/grpc#2201

Closed

doutriaux1 mentioned this pull request Aug 14, 2015

SciPy fails to build on CentOS 6.6 CDAT/cdat#1192

Closed

obfuscurity mentioned this pull request Sep 1, 2016

pip install of 0.9.15 = No $GRAPHITE_ROOT/webapp/graphite exists graphite-project/graphite-web#1508

Closed

lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 4, 2019

lock bot locked as resolved and limited conversation to collaborators Jun 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --install-options and --global-options to the requirements file parser. #2537

Add --install-options and --global-options to the requirements file parser. #2537

gvalkov commented Mar 14, 2015

pfmoore commented Mar 15, 2015

qwcode commented Mar 16, 2015

msabramo commented Mar 16, 2015

gvalkov commented Mar 16, 2015

qwcode Mar 18, 2015

gvalkov Mar 18, 2015

qwcode Mar 18, 2015

qwcode commented Mar 18, 2015

qwcode Mar 18, 2015

gvalkov commented Mar 18, 2015

gvalkov commented Mar 18, 2015

gvalkov commented Mar 21, 2015

pfmoore commented Mar 21, 2015

pfmoore commented Mar 22, 2015

qwcode Mar 22, 2015

pfmoore Mar 22, 2015

gvalkov Mar 22, 2015

gvalkov commented Apr 12, 2015

pfmoore commented Apr 12, 2015

Add --install-options and --global-options to the requirements file parser. #2537

Add --install-options and --global-options to the requirements file parser. #2537

Conversation

gvalkov commented Mar 14, 2015

pfmoore commented Mar 15, 2015

qwcode commented Mar 16, 2015

msabramo commented Mar 16, 2015

gvalkov commented Mar 16, 2015

qwcode Mar 18, 2015

Choose a reason for hiding this comment

gvalkov Mar 18, 2015

Choose a reason for hiding this comment

qwcode Mar 18, 2015

Choose a reason for hiding this comment

qwcode commented Mar 18, 2015

qwcode Mar 18, 2015

Choose a reason for hiding this comment

gvalkov commented Mar 18, 2015

gvalkov commented Mar 18, 2015

gvalkov commented Mar 21, 2015

pfmoore commented Mar 21, 2015

pfmoore commented Mar 22, 2015

qwcode Mar 22, 2015

Choose a reason for hiding this comment

pfmoore Mar 22, 2015

Choose a reason for hiding this comment

gvalkov Mar 22, 2015

Choose a reason for hiding this comment

gvalkov commented Apr 12, 2015

pfmoore commented Apr 12, 2015