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

Fix issue #1433: parse requirements in markers #1472

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
@vstinner
Contributor

vstinner commented Jan 14, 2014

  • InstallRequirement supports PEP 426 markers
  • RequirementSet.add_requirement() ignores an InstallRequirement if
    markers don't match.

fixes #1433

@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner Jan 14, 2014

Contributor

Example of requirements for my Trollius project:

futures; python_version < '2.7'
mock; python_version < '3.3'
nose
ordereddict; python_version < '2.7'
unittest2; python_version < '2.7'

Output with Python 2.7:

$ python2.7 -c 'import pip; pip.main()' install -r req.txt 
Ignore futures: markers "python_version < '2.7'" don't match
Ignore ordereddict: markers "python_version < '2.7'" don't match
Ignore unittest2: markers "python_version < '2.7'" don't match
Requirement already satisfied (use --upgrade to upgrade): mock in /usr/lib/python2.7/site-packages (from -r req.txt (line 2))
Requirement already satisfied (use --upgrade to upgrade): nose in /usr/lib/python2.7/site-packages (from -r req.txt (line 3))
Cleaning up...

Trollius: https://bitbucket.org/haypo/trollius/

Contributor

vstinner commented Jan 14, 2014

Example of requirements for my Trollius project:

futures; python_version < '2.7'
mock; python_version < '3.3'
nose
ordereddict; python_version < '2.7'
unittest2; python_version < '2.7'

Output with Python 2.7:

$ python2.7 -c 'import pip; pip.main()' install -r req.txt 
Ignore futures: markers "python_version < '2.7'" don't match
Ignore ordereddict: markers "python_version < '2.7'" don't match
Ignore unittest2: markers "python_version < '2.7'" don't match
Requirement already satisfied (use --upgrade to upgrade): mock in /usr/lib/python2.7/site-packages (from -r req.txt (line 2))
Requirement already satisfied (use --upgrade to upgrade): nose in /usr/lib/python2.7/site-packages (from -r req.txt (line 3))
Cleaning up...

Trollius: https://bitbucket.org/haypo/trollius/

@dholth

This comment has been minimized.

Show comment
Hide comment
@dholth

dholth Jan 14, 2014

Member

The original markers specification explicitly did not support < and > against strings. Does it now?

Member

dholth commented Jan 14, 2014

The original markers specification explicitly did not support < and > against strings. Does it now?

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Jan 14, 2014

Contributor

in PEP426 it says:

MARKER: EXPR [(and|or) EXPR]*
EXPR: ("(" MARKER ")") | (SUBEXPR [CMPOP SUBEXPR])
CMPOP: (==|!=|<|>|<=|>=|in|not in)

I'm inclined to think we need "Requirement Files 2.0" for this. json format.
it would also help with #790

Contributor

qwcode commented Jan 14, 2014

in PEP426 it says:

MARKER: EXPR [(and|or) EXPR]*
EXPR: ("(" MARKER ")") | (SUBEXPR [CMPOP SUBEXPR])
CMPOP: (==|!=|<|>|<=|>=|in|not in)

I'm inclined to think we need "Requirement Files 2.0" for this. json format.
it would also help with #790

@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner Jan 15, 2014

Contributor

The new commit should fix tests on Travis. I'm not sure that "; " is the best separator for markers. Do you have a better proposition? The semicolon is not a random choice, it comes from setup.cfg.

Contributor

vstinner commented Jan 15, 2014

The new commit should fix tests on Travis. I'm not sure that "; " is the best separator for markers. Do you have a better proposition? The semicolon is not a random choice, it comes from setup.cfg.

@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner Jan 23, 2014

Contributor

@qwcode: Sorry, but I don't understand what do you mean by:

I'm inclined to think we need "Requirement Files 2.0" for this. json format.

Contributor

vstinner commented Jan 23, 2014

@qwcode: Sorry, but I don't understand what do you mean by:

I'm inclined to think we need "Requirement Files 2.0" for this. json format.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Jan 23, 2014

Contributor

@haypo see the thread I started last night on pypa-dev about this
https://groups.google.com/forum/#!topic/pypa-dev/0zI8Jw-PYVk

Contributor

qwcode commented Jan 23, 2014

@haypo see the thread I started last night on pypa-dev about this
https://groups.google.com/forum/#!topic/pypa-dev/0zI8Jw-PYVk

@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner Feb 12, 2014

Contributor

I just rebased the patch on lastest develop branch.

Contributor

vstinner commented Feb 12, 2014

I just rebased the patch on lastest develop branch.

@dholth

This comment has been minimized.

Show comment
Hide comment
@dholth

dholth Feb 12, 2014

Member

+1. Unlike Requirements 2.0 this is simple and it exists.

Member

dholth commented Feb 12, 2014

+1. Unlike Requirements 2.0 this is simple and it exists.

@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner Mar 3, 2014

Contributor

With my latest change, the syntax is now:

mock >= 0.9 # comment
{'name': 'mock >= 0.9'} # comment works here too
{'name': 'mock3', 'markers': 'python_version >= "3"'} # markers!
# {...} syntax opens the door for more options like "install-options" and "global-options" as well
Contributor

vstinner commented Mar 3, 2014

With my latest change, the syntax is now:

mock >= 0.9 # comment
{'name': 'mock >= 0.9'} # comment works here too
{'name': 'mock3', 'markers': 'python_version >= "3"'} # markers!
# {...} syntax opens the door for more options like "install-options" and "global-options" as well
@msabramo

This comment has been minimized.

Show comment
Hide comment
@msabramo

msabramo May 14, 2014

Contributor

👍

This would be very handy to be able to make requirements conditional. I've often wished for this when porting packages to Python 3.

Contributor

msabramo commented May 14, 2014

👍

This would be very handy to be able to make requirements conditional. I've often wished for this when porting packages to Python 3.

@Ivoz

This comment has been minimized.

Show comment
Hide comment
@Ivoz

Ivoz May 14, 2014

Member

I would prefer to leave out anything to do with {} and code eval until a requirements 2.0. i.e anything other than the first code example in #1472 (comment)

Member

Ivoz commented May 14, 2014

I would prefer to leave out anything to do with {} and code eval until a requirements 2.0. i.e anything other than the first code example in #1472 (comment)

@dholth

This comment has been minimized.

Show comment
Hide comment
@dholth

dholth May 22, 2014

Member

This feature should be merged. I agree with just allowing the ; for now, until we have time to overengineer a requirements 2.0.

Member

dholth commented May 22, 2014

This feature should be merged. I agree with just allowing the ; for now, until we have time to overengineer a requirements 2.0.

@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner May 22, 2014

Contributor

Ok, here is a new proposal of syntax for markers in requirements. Use the semicolon as separator for the common code, BUT requires semicolon+space separator if the line starts with an URL.

Valid syntax:

# semicolon + space separator
futures; python_version < '2.7'
# semicolon without space works too
mock;python_version < '3.3'
# URL with semicolon + space separator
http://foo.com/?p=bar.git;a=snapshot;h=v0.1;sf=tgz; python_version < '3.3'

Invalid syntax, ;python_version < '3.3' is part of the URL:

# URL with semicolon separator
http://foo.com/?p=bar.git;a=snapshot;h=v0.1;sf=tgz;python_version < '3.3'

These examples with URL are ugly but it was just to show you the worst case, when the URL also contains semicolons. A more common case looks like this:

six
futures; python_version < '2.7'
https://example.com/foo.tar.gz
https://example.com/bar.tar.gz; python_version < '2.7'
Contributor

vstinner commented May 22, 2014

Ok, here is a new proposal of syntax for markers in requirements. Use the semicolon as separator for the common code, BUT requires semicolon+space separator if the line starts with an URL.

Valid syntax:

# semicolon + space separator
futures; python_version < '2.7'
# semicolon without space works too
mock;python_version < '3.3'
# URL with semicolon + space separator
http://foo.com/?p=bar.git;a=snapshot;h=v0.1;sf=tgz; python_version < '3.3'

Invalid syntax, ;python_version < '3.3' is part of the URL:

# URL with semicolon separator
http://foo.com/?p=bar.git;a=snapshot;h=v0.1;sf=tgz;python_version < '3.3'

These examples with URL are ugly but it was just to show you the worst case, when the URL also contains semicolons. A more common case looks like this:

six
futures; python_version < '2.7'
https://example.com/foo.tar.gz
https://example.com/bar.tar.gz; python_version < '2.7'
@msabramo

This comment has been minimized.

Show comment
Hide comment
@msabramo

msabramo May 22, 2014

Contributor

👍

Contributor

msabramo commented May 22, 2014

👍

@dholth

This comment has been minimized.

Show comment
Hide comment
@dholth

dholth May 22, 2014

Member

I've done something like this, except the rule was "the last semicolon
in the line of text is the separator".

On Thu, May 22, 2014, at 11:59 AM, Marc Abramowitz wrote:

👍

Reply to this email directly or [1]view it on GitHub.
[208018__eyJzY29wZSI6Ik5ld3NpZXM6QmVhY29uIiwiZXhwaXJlcyI6MTcxNjM5MzU0OS
wiZGF0YSI6eyJpZCI6MjM1MTEyNjV9fQ==--b3d853556e0ecc5f5444ee106bd2305f15d
54e3a.gif]

References

  1. #1472 (comment)
Member

dholth commented May 22, 2014

I've done something like this, except the rule was "the last semicolon
in the line of text is the separator".

On Thu, May 22, 2014, at 11:59 AM, Marc Abramowitz wrote:

👍

Reply to this email directly or [1]view it on GitHub.
[208018__eyJzY29wZSI6Ik5ld3NpZXM6QmVhY29uIiwiZXhwaXJlcyI6MTcxNjM5MzU0OS
wiZGF0YSI6eyJpZCI6MjM1MTEyNjV9fQ==--b3d853556e0ecc5f5444ee106bd2305f15d
54e3a.gif]

References

  1. #1472 (comment)
@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner May 22, 2014

Contributor

I've done something like this, except the rule was "the last semicolon in the line of text is the separator".

Oh, it makes me realize that it would be nice to add a test checking that a marker can contain a semicolon. Travis was unhappy. There was a obvious bug in the method checking if markers match or not. It's now fixed but I also added unit tests on this method.

The whole change now has a nice coverage in term of unit tests, and I'm happy with the new syntax. So it's ready for a final review ;-)

Contributor

vstinner commented May 22, 2014

I've done something like this, except the rule was "the last semicolon in the line of text is the separator".

Oh, it makes me realize that it would be nice to add a test checking that a marker can contain a semicolon. Travis was unhappy. There was a obvious bug in the method checking if markers match or not. It's now fixed but I also added unit tests on this method.

The whole change now has a nice coverage in term of unit tests, and I'm happy with the new syntax. So it's ready for a final review ;-)

@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner May 24, 2014

Contributor

" Error — The Travis CI build could not complete due to an error · Details "

It looks like Python 3 tests are too slow for Travis. I played the tests manually on my PC and py33 tests pass: "435 passed, 8 skipped in 803.33 seconds".

Contributor

vstinner commented May 24, 2014

" Error — The Travis CI build could not complete due to an error · Details "

It looks like Python 3 tests are too slow for Travis. I played the tests manually on my PC and py33 tests pass: "435 passed, 8 skipped in 803.33 seconds".

@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner Oct 14, 2014

Contributor

Hey, did I forgot something to get my patch merged? What should I do?

Contributor

vstinner commented Oct 14, 2014

Hey, did I forgot something to get my patch merged? What should I do?

@matrixise

This comment has been minimized.

Show comment
Hide comment
@matrixise

matrixise Oct 14, 2014

+1 for this feature.

+1 for this feature.

@hobbestigrou

This comment has been minimized.

Show comment
Hide comment
@hobbestigrou

hobbestigrou Oct 14, 2014

I agree also for this feature.

I agree also for this feature.

@mlhamel

This comment has been minimized.

Show comment
Hide comment
@mlhamel

mlhamel Oct 14, 2014

+1, that would be awesome and would help limiting the code in, let's say... setup.py by example

mlhamel commented Oct 14, 2014

+1, that would be awesome and would help limiting the code in, let's say... setup.py by example

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Oct 14, 2014

Member

@haypo looks like the patch needs rebasing. Also, it could do with documentation.

@mihamel how would it affect setup.py? This specifically doesn't apply to setup_requires, so won't be useful for project dependencies. You'll need PEP 426 support in pip and setuptools for that.

Member

pfmoore commented Oct 14, 2014

@haypo looks like the patch needs rebasing. Also, it could do with documentation.

@mihamel how would it affect setup.py? This specifically doesn't apply to setup_requires, so won't be useful for project dependencies. You'll need PEP 426 support in pip and setuptools for that.

@mlhamel

This comment has been minimized.

Show comment
Hide comment
@mlhamel

mlhamel Oct 14, 2014

@pfmoore it doesn't, i was just pointing the fact that people are using setup.py for doing that kind of things, since it's python code. but sometime, you end up having a huge file which is super difficult to maintain. Being able to do it in a simpler way in a requirement.txt would be fantastic.

Take this one by example:

https://github.com/odoo/odoo/blob/6.0/setup.py

(it's way more better in their latest version although)

mlhamel commented Oct 14, 2014

@pfmoore it doesn't, i was just pointing the fact that people are using setup.py for doing that kind of things, since it's python code. but sometime, you end up having a huge file which is super difficult to maintain. Being able to do it in a simpler way in a requirement.txt would be fantastic.

Take this one by example:

https://github.com/odoo/odoo/blob/6.0/setup.py

(it's way more better in their latest version although)

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Oct 14, 2014

Member

setup.py and requirements.txt are for different things.

https://caremad.io/blog/setup-vs-requirement/

Member

dstufft commented Oct 14, 2014

setup.py and requirements.txt are for different things.

https://caremad.io/blog/setup-vs-requirement/

@mlhamel

This comment has been minimized.

Show comment
Hide comment
@mlhamel

mlhamel Oct 14, 2014

@dstufft yeah but people are using both. for different reason, that would be another reason to use requirement.txt for me... :)

mlhamel commented Oct 14, 2014

@dstufft yeah but people are using both. for different reason, that would be another reason to use requirement.txt for me... :)

Fix issue #1433: parse requirements in markers
* InstallRequirement supports PEP 426 markers
* RequirementSet.add_requirement() ignores an InstallRequirement if
  markers don't match.
@dholth

This comment has been minimized.

Show comment
Hide comment
@dholth

dholth Oct 14, 2014

Member

It is possible to use markers in setup.py by using a colon separator in the extra name. Wheel's own setup.py demonstrates. The empty extra name ':python_version=="2.6"' is how you do markers on dependencies that would otherwise go into install_requires = [].

  extras_require={
      ':python_version=="2.6"': ['argparse'],
      'signatures': ['keyring'],
      'signatures:sys_platform!="win32"': ['pyxdg'],
      'faster-signatures': ['ed25519ll'],
      'tool': []
      },
Member

dholth commented Oct 14, 2014

It is possible to use markers in setup.py by using a colon separator in the extra name. Wheel's own setup.py demonstrates. The empty extra name ':python_version=="2.6"' is how you do markers on dependencies that would otherwise go into install_requires = [].

  extras_require={
      ':python_version=="2.6"': ['argparse'],
      'signatures': ['keyring'],
      'signatures:sys_platform!="win32"': ['pyxdg'],
      'faster-signatures': ['ed25519ll'],
      'tool': []
      },
@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Oct 14, 2014

Member

I'd rather we get a properly documented and supported solution. @dholth I've no idea if this is supported (I've no doubt it works). Is it documented anywhere?

In retrospect, and given the confusion the setup.py vs requirements question has caused here, I'd rather see a properly agreed spec for using markers in "requirements" in the general sense, and an implementation that adds them to both setuptools (so they'll work in setup.py) and pip (so they'll be parsed from both install_requires and requirements.txt.

Member

pfmoore commented Oct 14, 2014

I'd rather we get a properly documented and supported solution. @dholth I've no idea if this is supported (I've no doubt it works). Is it documented anywhere?

In retrospect, and given the confusion the setup.py vs requirements question has caused here, I'd rather see a properly agreed spec for using markers in "requirements" in the general sense, and an implementation that adds them to both setuptools (so they'll work in setup.py) and pip (so they'll be parsed from both install_requires and requirements.txt.

@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner Oct 14, 2014

Contributor

@haypo looks like the patch needs rebasing.

Done.

Also, it could do with documentation.

I added something to the documentation, is it enough?

I ran "tox -e pep8,docs,py27,py33" with success. Let's see if Travis agrees with me ;-)

Contributor

vstinner commented Oct 14, 2014

@haypo looks like the patch needs rebasing.

Done.

Also, it could do with documentation.

I added something to the documentation, is it enough?

I ran "tox -e pep8,docs,py27,py33" with success. Let's see if Travis agrees with me ;-)

@berkerpeksag

View changes

Show outdated Hide outdated docs/reference/pip_install.rst
@@ -29,6 +29,12 @@ and like arguments to :ref:`pip install`, the following forms are supported::
[-e] <local project path>
[-e] <vcs project url>
Since the version 6.0, pip also supports markers using the "; " separator.

This comment has been minimized.

@berkerpeksag

berkerpeksag Oct 15, 2014

Member

I guess "version 6.0" means "setuptools 6.0", right? :)

@berkerpeksag

berkerpeksag Oct 15, 2014

Member

I guess "version 6.0" means "setuptools 6.0", right? :)

This comment has been minimized.

@vstinner

vstinner Oct 15, 2014

Contributor

I guess "version 6.0" means "setuptools 6.0", right? :)

I searched for the version of pip and I found version = "6.0.dev1" in pip/init.py. It is not the version of pip?

@vstinner

vstinner Oct 15, 2014

Contributor

I guess "version 6.0" means "setuptools 6.0", right? :)

I searched for the version of pip and I found version = "6.0.dev1" in pip/init.py. It is not the version of pip?

This comment has been minimized.

@pfmoore

pfmoore Oct 15, 2014

Member

Correct - we removed the initial "1." so version 6.X is what was previously being referred to as 1.6.X. By the way, a minor nit, "Since version 6.0" would be better grammatically.

@pfmoore

pfmoore Oct 15, 2014

Member

Correct - we removed the initial "1." so version 6.X is what was previously being referred to as 1.6.X. By the way, a minor nit, "Since version 6.0" would be better grammatically.

@matrixise

This comment has been minimized.

Show comment
Hide comment
@matrixise

matrixise Oct 15, 2014

Please, merge it. Thank you

Please, merge it. Thank you

@matrixise

This comment has been minimized.

Show comment
Hide comment
@matrixise

matrixise Oct 15, 2014

Example, OrderedDict is in Python 2.7 and Python 3.x but not in Python 2.6
On some Redhat or CentOS servers, it's Python 2.6.

Example, OrderedDict is in Python 2.7 and Python 3.x but not in Python 2.6
On some Redhat or CentOS servers, it's Python 2.6.

Support markers in requirements
It's now possible to specify requirements markers in requirements.
Examples::

    futures; python_version < '2.7'
    mock; python_version < '3.3'
    nose
    ordereddict; python_version < '2.7'
    unittest2; python_version < '2.7'

The separator is "; ". For convinience, ";" alone is also supported, but
no in URLs. The ";" character is a legit and common character in an URL.
Example of valid URL without markers::

    http://foo.com/?p=bar.git;a=snapshot;h=v0.1;sf=tgz

Example of URL with markers::

    http://foo.com/?p=bar.git;a=snapshot;h=v0.1;sf=tgz; python_version < '3.3'
@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner Oct 29, 2014

Contributor

I ran "tox -e pep8,docs,py27,py33" with success. Let's see if Travis agrees with me ;-)

Travis agrees with me. So what's the next step?

Contributor

vstinner commented Oct 29, 2014

I ran "tox -e pep8,docs,py27,py33" with success. Let's see if Travis agrees with me ;-)

Travis agrees with me. So what's the next step?

@dstufft dstufft added this to the 6.0 milestone Nov 20, 2014

@dstufft dstufft referenced this pull request Nov 20, 2014

Merged

Haypo req markers #2134

@dstufft dstufft closed this in #2134 Nov 20, 2014

@matrixise

This comment has been minimized.

Show comment
Hide comment

Thanks

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