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

Environment markers don't support <, <= etc #380

Closed
bb-migration opened this Issue May 13, 2015 · 12 comments

Comments

Projects
None yet
1 participant
@bb-migration

bb-migration commented May 13, 2015

Originally reported by: rbtcollins (Bitbucket: rbtcollins, GitHub: rbtcollins)


PEP 427 specifies a range of comparison operators:
CMPOP: (==|!=|<|>|<=|>=|in|not in)

But when I tried to use <= while testing environment markers, I get an error about an invalid marker. The same exact expression with s/</=/ works.

If this is the wrong place to file marker bugs - please point me at the right place :)


@bb-migration

This comment has been minimized.

bb-migration commented May 22, 2015

Original comment by jamezpolley (Bitbucket: jamezpolley, GitHub: jamezpolley):


I believe you mean PEP-426 rather than PEP-427

This seems to come from https://bitbucket.org/pypa/setuptools/src/b6e4e89191a674855e06114e8072f2449ef53347/pkg_resources/__init__.py?at=default#cl-1481

@bb-migration

This comment has been minimized.

bb-migration commented May 27, 2015

Original comment by jamezpolley (Bitbucket: jamezpolley, GitHub: jamezpolley):


I've created https://bitbucket.org/pypa/setuptools/pull-request/134/expand-the-range-of-valid-operators-to/diff which attempts to address this issue.

It certainly avoids the invalid marker error message, but I'm not certain if this does everything we need to make sure that the markers get fully parsed and requirements installed

@bb-migration

This comment has been minimized.

bb-migration commented Jun 7, 2015

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


Add tests capturing expectation for range comparison operators (Ref #380).

@bb-migration

This comment has been minimized.

bb-migration commented Jun 7, 2015

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


I accepted Pull Request #134, but it causes a test to fail:

    >>> print(im("'x' < 'y'"))
    '<' operator not allowed in environment markers

I don't know why this test was added. Was it there to describe the behavior as implemented or was it there to prescribe some known violation with the spec? Since there's no comment in the code, I'm going to assume the former and simply remove the failing test.

@bb-migration

This comment has been minimized.

bb-migration commented Jun 7, 2015

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


I should note also that one's expectation is probably violated with a PEP426 marker such as "full_python_version > 2.7.3" as "2.7.10" will fail for that. I'll add that as a test.

@bb-migration

This comment has been minimized.

bb-migration commented Jun 7, 2015

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


Add test capturing possible violation in expectation due to new Python 2.7.10 release. Ref #380.

@bb-migration

This comment has been minimized.

bb-migration commented Jun 7, 2015

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


Remove now deprecated test capturing failure of range comparison operators (Ref #380).

@bb-migration

This comment has been minimized.

bb-migration commented Jun 7, 2015

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


Released as 17.1.

@bb-migration

This comment has been minimized.

bb-migration commented Jun 9, 2015

Original comment by jamezpolley (Bitbucket: jamezpolley, GitHub: jamezpolley):


Hrm.

I'm sorry. I said above that "I'm not certain this does everything we need".

I probably should have come back back to say that I'm now certain it doesn't. It does, in my testing, avoid the error being raised (although it seems other people don't even see that) - but it doesn't result in requirements that have these markers being processed and the requirements being installed. I'm still trying to track down why that's the case.

@bb-migration

This comment has been minimized.

bb-migration commented Jun 9, 2015

Original comment by jamezpolley (Bitbucket: jamezpolley, GitHub: jamezpolley):


The old behaviour at least warned people about env markers that wouldn't work; the new behaviour silently drops requirements that look like they should work, which is arguably worse.

If we can't find a way to make this work properly, perhaps it'd be better to revert to the old behaviour?

@bb-migration

This comment has been minimized.

bb-migration commented Jun 9, 2015

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


I'd rather not back out the change. I do see the argument that it doesn't fail fast, and could, but I'd rather work toward an implementation that does what you want. I've successfully used the markers feature for selectively and declaratively requiring a dependency. See backports.unittest_mock for an example. Through the 'extras', mock will be required, but only on Python 2. When I can rely on Setuptools 17.1, I can change that dependency to python_version < "3.3". Can you open a new ticket describing what is happening and how that differs from your expectation?

@bb-migration

This comment has been minimized.

bb-migration commented Jun 10, 2015

Original comment by jamezpolley (Bitbucket: jamezpolley, GitHub: jamezpolley):


I defer to your experience re: backing out the change.

Happily, a colleague seems to have tracked down the source of the problems I was seeing - and it seems that the fix is to apply the same patch as pull request 134 to the pkg_resources vendored into pip. So there doesn't seem to be any need to do anything more here :)

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