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 --install-options and --global-options to the requirements file parser. #2537

Merged
merged 25 commits into from Apr 12, 2015

Conversation

Projects
None yet
4 participants
@gvalkov
Contributor

gvalkov commented Mar 14, 2015

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

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Mar 15, 2015

Member

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.

Member

pfmoore commented Mar 15, 2015

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

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Mar 16, 2015

Contributor

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

Contributor

qwcode commented Mar 16, 2015

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

@msabramo

This comment has been minimized.

Show comment
Hide comment
@msabramo

msabramo Mar 16, 2015

Contributor

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.

Contributor

msabramo commented Mar 16, 2015

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

This comment has been minimized.

Show comment
Hide comment
@gvalkov

gvalkov Mar 16, 2015

Contributor

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.

Contributor

gvalkov commented Mar 16, 2015

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.

@@ -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):

This comment has been minimized.

@qwcode

qwcode Mar 18, 2015

Contributor

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

@qwcode

qwcode Mar 18, 2015

Contributor

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

This comment has been minimized.

@gvalkov

gvalkov Mar 18, 2015

Contributor

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

@gvalkov

gvalkov Mar 18, 2015

Contributor

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

This comment has been minimized.

@qwcode

qwcode Mar 18, 2015

Contributor

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

qwcode Mar 18, 2015

Contributor

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?

# the setup.py call with the ones from the requirements file.
# Options specified in requirements file override those
# specified on the command line, since the last option given
# to setup.py is the one that is used.

This comment has been minimized.

@qwcode

qwcode Mar 18, 2015

Contributor

I recall this from the first go round I think : )
although, I can't find an example why this won't work, it's odd to see the resulting command using '-v'

@qwcode

qwcode Mar 18, 2015

Contributor

I recall this from the first go round I think : )
although, I can't find an example why this won't work, it's odd to see the resulting command using '-v'

This comment has been minimized.

@gvalkov

gvalkov Mar 18, 2015

Contributor

Good memory - that was 2 years ago :)

It's weird indeed and I think it should be mentioned somewhere that using this feature will void your warranty. There is nothing to stop people from having something like the following in their requirements:

six==1.9.0 --global-options="--dry-run"
@gvalkov

gvalkov Mar 18, 2015

Contributor

Good memory - that was 2 years ago :)

It's weird indeed and I think it should be mentioned somewhere that using this feature will void your warranty. There is nothing to stop people from having something like the following in their requirements:

six==1.9.0 --global-options="--dry-run"

This comment has been minimized.

@qwcode

qwcode Mar 22, 2015

Contributor

@pfmoore just want to make sure you understand that overriding only works here due to the 2nd option overriding the first.

e.g. if you specified "--install-lib" in the requirements file and the command line, both flags would be present in the "setup.py install" call, and the override would just work due to us assuming setuptools handles the options by ignoring the first.

although I can't disprove this won't work, if I were doing it, I would go to the trouble of removing the duplicate from the call

your opinion?

@qwcode

qwcode Mar 22, 2015

Contributor

@pfmoore just want to make sure you understand that overriding only works here due to the 2nd option overriding the first.

e.g. if you specified "--install-lib" in the requirements file and the command line, both flags would be present in the "setup.py install" call, and the override would just work due to us assuming setuptools handles the options by ignoring the first.

although I can't disprove this won't work, if I were doing it, I would go to the trouble of removing the duplicate from the call

your opinion?

This comment has been minimized.

@pfmoore

pfmoore Mar 22, 2015

Member

I don't think we can easily trim duplicates. For a start you'd need to know that -c and --compile are aliases, and that --no-compile is another variant. And projects could add extra options that might be additive.

I think documenting clearly that install and global options from the requirements file and the command line are both added to the final command, with a clear specification of the order they are added in, plus a note saying that the user is responsible for ensuring that the resulting command is valid, is all we can do.

For example, I use the following pip wheel command to build pyyaml:

pip wheel --global-option "--with-libyaml" --global-option "build_ext" --global-option "-DYAML_DECLARE_STATIC" --global-option "-LC:\Work\Scratch\pyyaml\yaml-0.1.5\win32\vs2008\Output\Release\lib" --global-option "-IC:\Work\Scratch\pyyaml\yaml-0.1.5\include"  .\PyYAML-3.10.tar.gz

Note that I'm using a global option of build_ext to insert an explicit extension build command into the setup.py invocation, because I need the include file options added to the build_ext command, and the bdist_wheel command doesn't support them. Putting that lot in a requirements file would be an obvious thing to do.

(I'm not saying this is good practice, it's clearly an abuse, but sometimes you do what you must :-))

@pfmoore

pfmoore Mar 22, 2015

Member

I don't think we can easily trim duplicates. For a start you'd need to know that -c and --compile are aliases, and that --no-compile is another variant. And projects could add extra options that might be additive.

I think documenting clearly that install and global options from the requirements file and the command line are both added to the final command, with a clear specification of the order they are added in, plus a note saying that the user is responsible for ensuring that the resulting command is valid, is all we can do.

For example, I use the following pip wheel command to build pyyaml:

pip wheel --global-option "--with-libyaml" --global-option "build_ext" --global-option "-DYAML_DECLARE_STATIC" --global-option "-LC:\Work\Scratch\pyyaml\yaml-0.1.5\win32\vs2008\Output\Release\lib" --global-option "-IC:\Work\Scratch\pyyaml\yaml-0.1.5\include"  .\PyYAML-3.10.tar.gz

Note that I'm using a global option of build_ext to insert an explicit extension build command into the setup.py invocation, because I need the include file options added to the build_ext command, and the bdist_wheel command doesn't support them. Putting that lot in a requirements file would be an obvious thing to do.

(I'm not saying this is good practice, it's clearly an abuse, but sometimes you do what you must :-))

This comment has been minimized.

@qwcode

qwcode Mar 22, 2015

Contributor

ok, good points

@qwcode

qwcode Mar 22, 2015

Contributor

ok, good points

Show outdated Hide outdated docs/reference/pip_install.rst Outdated
Show outdated Hide outdated pip/req/req_file.py Outdated
@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Mar 18, 2015

Contributor

--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

``
Contributor

qwcode commented Mar 18, 2015

--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

``
Show outdated Hide outdated pip/req/req_file.py Outdated
Show outdated Hide outdated docs/reference/pip_install.rst Outdated
elif opt == '--allow-external':
finder.allow_external |= set([normalize_name(value).lower()])
elif opt == '--allow-insecure':
# Remove after 7.0

This comment has been minimized.

@pfmoore

pfmoore Mar 18, 2015

Member

This list of options differs from what was deleted (e.g., -no-allow-insecure used to be present). It's not obvious to me that the behaviour here is the same - are there tests to confirm no regressions have been introduced?

@pfmoore

pfmoore Mar 18, 2015

Member

This list of options differs from what was deleted (e.g., -no-allow-insecure used to be present). It's not obvious to me that the behaviour here is the same - are there tests to confirm no regressions have been introduced?

This comment has been minimized.

@gvalkov

gvalkov Mar 18, 2015

Contributor

The options that were there for compatibility reasons were moved to the top of req_file.py:

# Encountering any of these is a no-op.
parser_compat = set([
    '-Z', '--always-unzip',
    '--use-wheel',          # Default in 1.5
    '--no-allow-external',  # Remove in 7.0
    '--no-allow-insecure',  # Remove in 7.0
])

There don't seem to be any tests that check if obsolete options are ignored at the moment.

@gvalkov

gvalkov Mar 18, 2015

Contributor

The options that were there for compatibility reasons were moved to the top of req_file.py:

# Encountering any of these is a no-op.
parser_compat = set([
    '-Z', '--always-unzip',
    '--use-wheel',          # Default in 1.5
    '--no-allow-external',  # Remove in 7.0
    '--no-allow-insecure',  # Remove in 7.0
])

There don't seem to be any tests that check if obsolete options are ignored at the moment.

Show outdated Hide outdated tests/unit/test_req.py Outdated
@gvalkov

This comment has been minimized.

Show comment
Hide comment
@gvalkov

gvalkov Mar 18, 2015

Contributor

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.

Contributor

gvalkov commented Mar 18, 2015

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

This comment has been minimized.

Show comment
Hide comment
@gvalkov

gvalkov Mar 18, 2015

Contributor

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.

Contributor

gvalkov commented Mar 18, 2015

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

This comment has been minimized.

Show comment
Hide comment
@gvalkov

gvalkov Mar 21, 2015

Contributor

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

Contributor

gvalkov commented Mar 21, 2015

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

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Mar 21, 2015

Member

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.

Member

pfmoore commented Mar 21, 2015

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

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Mar 22, 2015

Member

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.

Member

pfmoore commented Mar 22, 2015

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.

Show outdated Hide outdated pip/req/req_file.py Outdated
@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Mar 22, 2015

Contributor

why not build a mini pip parser

@pfmoore , yes, he did what I wanted, in that he's now reusing our options, but note my comment above about the extra flatten step.

Contributor

qwcode commented Mar 22, 2015

why not build a mini pip parser

@pfmoore , yes, he did what I wanted, in that he's now reusing our options, but note my comment above about the extra flatten step.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Mar 22, 2015

Contributor

Can you give a +1 if you're OK for this to go

I only had a few mins this morning, but I can look again tomorrow night in more detail for a potential +1 (pending the flatten issue and override issue) if you want to wait.

Contributor

qwcode commented Mar 22, 2015

Can you give a +1 if you're OK for this to go

I only had a few mins this morning, but I can look again tomorrow night in more detail for a potential +1 (pending the flatten issue and override issue) if you want to wait.

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Mar 22, 2015

Member

@qwcode no rush, you're picking up on things I'd missed so an extra set of eyes is great.

Agreed on the flatten issue, that needs a fix to the patch. I've added a comment on the override issue.

Member

pfmoore commented Mar 22, 2015

@qwcode no rush, you're picking up on things I'd missed so an extra set of eyes is great.

Agreed on the flatten issue, that needs a fix to the patch. I've added a comment on the override issue.

Show outdated Hide outdated tests/unit/test_req.py Outdated
Show outdated Hide outdated tests/unit/test_req.py Outdated
Show outdated Hide outdated docs/reference/pip_install.rst Outdated
@gvalkov

This comment has been minimized.

Show comment
Hide comment
@gvalkov

gvalkov Mar 24, 2015

Contributor

Thank you for the helpful reviews @pfmoore and @qwcode. Everything has been addressed, except for the last test case you spoke of and the changes to the docs. You already seem to have very clear ideas about how those changes need to happen, so please implement them yourselves if possible (otherwise, please spell it out for me).

Contributor

gvalkov commented Mar 24, 2015

Thank you for the helpful reviews @pfmoore and @qwcode. Everything has been addressed, except for the last test case you spoke of and the changes to the docs. You already seem to have very clear ideas about how those changes need to happen, so please implement them yourselves if possible (otherwise, please spell it out for me).

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Mar 27, 2015

Contributor

@gvalkov noted. I'm willing to tweak some tests if need be, but I probably won't get back to this one until the weekend.

Contributor

qwcode commented Mar 27, 2015

@gvalkov noted. I'm willing to tweak some tests if need be, but I probably won't get back to this one until the weekend.

@gvalkov

This comment has been minimized.

Show comment
Hide comment
@gvalkov

gvalkov Apr 12, 2015

Contributor

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.

Contributor

gvalkov commented Apr 12, 2015

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.

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

Merge pull request #2537 from gvalkov/reqfileopts
Add --install-options and --global-options to the requirements file parser.

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Apr 12, 2015

Member

Looks good - thanks for the contribution!

Member

pfmoore commented Apr 12, 2015

Looks good - thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment