In Markers, use PEP 440 version comparison only if both sides are valid version specifier. #101

vphilippon · 2017-03-24T04:21:02Z

This adresses the bug mentionned here: pypa/pip#4356.

This parses the lhs to make sure it's a valid PEP 440 version specifier before attempting to use PEP 440 version comparison.
Do to so, the behaviour of Version(None) (or rather Version(<not a string>)) was changed: It now raises an InvalidVersion exception instead of relying on the regex raising a TypeError.

vphilippon · 2017-03-24T04:36:38Z

CI failling, pretty sure it's related to #95.
Seems like we'll need pip 9.0.2 once it gets released.

xavfernandez · 2017-03-26T17:47:23Z

@vphilippon hello, could you rebase on master ?

Passing something that is not a string (ex: None) to Version.__init__ caused a TypeError, thrown by the regex. Now, it will be stringified first. In the case of None, it will now raise an InvalidVersion exception instead.

…d version specifier.

vphilippon · 2017-03-27T14:31:14Z

@xavfernandez Done! Thanks for the CI fix, and for your reply on th pip repo too.

dstufft · 2017-04-07T00:45:09Z

packaging/version.py

@@ -198,7 +198,7 @@ class Version(_BaseVersion):

    def __init__(self, version):
        # Validate the version and parse it into pieces
-        match = self._regex.search(version)
+        match = self._regex.search(str(version))


Is this required? I would kind of prefer not to str() everything passed into version since that may mask things like doing Version(1) that should still fail.

That's what make sure that the regex doesn't throw a TypeError if version is not a string (like, None)
I could change to having a check like:

if not isinstance(version, str): raise InvalidVersion("Invalid version: '{0}' is not a string".format(version))

but I should probably look for buffer or unicode and all "string-like types" too.

The issue is that Version(1), Version(None) and Version(MyWeirdKlass()) should raise InvalidVersion and not TypeError from the regex.

EDIT:
And I just realised I could catch TypeError from the regex and then raise InvalidVersion instead, which seems like a decent idea.

Yea that seems like a good idea to me.

I'll get on this tonight or during the weekend.

vphilippon · 2017-04-08T17:34:43Z

packaging/version.py

+            match = self._regex.search(version)
+        except TypeError:
+            raise InvalidVersion(
+                "Invalid type: '{0}' is not a string".format(type(version))


@dstufft I went ahead and changed to catch TypeError, but something bugs me a bit:
InvalidVersion inherits from ValueError.
So raising something of the ValueError familly on a TypeError feels weird.
I could make InvalidVersion inherit from both ValueError and TypeError,
or make a seperate exception and add it's usage all around.
Or, if you feel this is ok and I'm being too nitpicky with my things, that's ok.

We could also just make InvalidVersion descend directly from Exception instead.

Giving the version's type is nice but I think version's repr should also be given to help the debugging.

@xavfernandez I agree.
I tried to make a short, clear and nice message with all that info, but tell me if it's weird (english is not my native language, and I end up having some weird ways to say stuff).

This means no more str() of the version argument. Instead, we catch errors straight from the regex. Version(<not-a-string>) will raise InvalidVersion now.

vphilippon · 2017-04-11T01:01:35Z

Amended my last commit (not sure it notifies the reviewers).

xavfernandez · 2017-04-11T08:10:51Z

tests/test_markers.py

+            (
+                "extra == '3.7.0'",
+                {"extra": "3.7.0+youaregreat"},
+                True,


Do we really want that ? :-/
(and same question for "extra": "3.7.0.0)

In PEP-440 3.7.0==3.7.0+anything, and 3.7.0==3.7.0.0 too.
That's how PEP-440 does the comparison, and how PEP-508 defines it, as it says it respects PEP-440 comparison for valid version specifier.
So,the marker extra == '3.7.0' with extra=3.7.0+youaregreat should evaluate as true.

We can open another issue about the PEP-508 spec and the changes we might want, but that's the "how PEP-508 is supposed to be right now" implementation.

Personnaly, I'm ok with that, and I'm eagerly waiting for the PEP-440 comparison to be fixed in the extras for pip, as I'd like to define some things like:

extras_require={ [';extra~=3.7']: ['adependency'] [';extra~=3.8']: ['anotherdependency'] }

Now, if I all my extras are valid version specifier, but I want 3.7.0 and 3.7.0.0 to be different, I could define those using ===:

extras_require={ [';extra===3.7.0']: ['adependency'] [';extra===3.7.0.0']: ['anotherdependency'] }

One sad thing, if you have extras like foobar, 3.7.0 and 3.7.0.0, you can't differentiate them all with PEP-508, as the === operator is not defined in python, so you can't use it here. That means you're back with 3.7.0==3.7.0.0.
Like I said, that's a whole other discussion IMO.

I agree, this PR follows the PEP-508, specifically:

Comparisons in marker expressions are typed by the comparison operator. The <marker_op> operators that are not in <version_cmp> perform the same as they do for strings in Python. The <version_cmp> operators use the PEP-440 version comparison rules when those are defined (that is when both sides have a valid version specifier).

but again, I don't think the PEP-508 wanted to allow "non-version" markers (that is os_name, sys_platform, platform_machine, platform_python_implementation, platform_system, implementation_name and IMHO extra) to behave like version numbers.
cc @rbtcollins

I see what you mean. That means that the <marker_op> operators behaviour would not only be determined by the types of both sides of the expression, but rather the expected types of both sides of the operation. Or rather, the expected type of the "variable".

That situation isn't really problematic usually, as each "variable" has an almost predifined type (python_version will be a version, sys_platform will be a string). With extra, it can be anything, really, and that's why we end up with a "it'll be evaluated based on what it looks like", which can bring some confusion (like the one I had before reading PEP-508).

The thing is, I'm not sure how we can change that behaviour, as when we're evaluationg a marker, we don't know what the "variable" was, we only have it's value. That is, unless we change the implementation quite a bit, and start associating a "type" to every "variable", and keeping that information for evaluation time.

I have a feeling that this is because extra might not fit so well as an environment marker, as it's a user provided value. This leads to a few odd behaviour (like this one), and some corner cases (like pypa/pip/issues/4086).

(Note that while I spent quite some time lately reading (and re-reading) the PEPs and the various codebase, as well as trying to analyze and understand a lot of this, I don't have the previous and historical knowledge that most of you seem to have, so thanks for taking the time to explain. I promise I'll keep trying to make it worthwile.)

After looking more into the code, I realized that having "Variable Types" wouldn't be as hard as I thought. I have a general idea of how I could implement that.

Here's my suggestion:

Merge this PR, as it fixes a bug that is live, which is that if a package provides extras that are valid version specifiers, the package is uninstalable (pointing back at DistInfoDistribution._compute_dependencies fails if the package has extras_require that are PEP440 versions. pip#4356).
This PR does not actually bring the change that introduced the questionable extra behaviour: If someone, right now with the previous code, creates a Marker object like extra == 3.7.0+youaregreat and evaluates it with an environment like {'extra': '3.7.0'}, it would have the behaviour I simply explicited in the tests. This simply never really occured "in the wild" before as the principal (if not only?) use of that is through setuptools/pip, which crashed before that by passing a None, which is now fixed.

Open an issue and re-launch the discussion on "Is that really the behaviour we intended with PEP 508?", as this PR pointed out there was an edge case with the extras possible values. We can then make the required changes/clarifications to PEP 508 and make the appropriate change to the code to respect the newly defined and written behaviour, if needed. I'll gladly join that discussion, and I'll most likely be ready to help by contributing with a PR for the changes.

If we could come to a conclusion really fast on the previously intended PEP 508 behaviour, I would gladly put it in this PR right away, but as we probably all have lots of things to do, I feel like that discussion will take quite some time, and it would be unfortunate to block a fix to a live bug for that.

Thanks again for your time!
@dstufft @xavfernandez

vphilippon · 2017-05-26T22:21:51Z

Comming back on this. I don't mind looking for a a way to have extras behave "not as version" and update the PEP 508, or even just remove the test that outline that behaviour before we look into the question. I'd just need a direction regarding what changes are required.

I still believe I only outlined an existing behaviour, but if you want it clarified and fixed as we are on the subject, that's a good point too.

As long as I can give a bit of traction to this, or fix the original pip issue by some other way.

brettcannon · 2022-08-14T22:51:09Z

Apologies for this slipping through the cracks! I just tried out some of the test cases, like markers.Marker("extra == '3.7.0'").evaluate({"extra": "foobar"}) and they returned False. So I think this is fixed? If not, then please let us know.

vphilippon added 2 commits March 27, 2017 09:59

In Version, convert version to string before applying the regex.

d79297a

Passing something that is not a string (ex: None) to Version.__init__ caused a TypeError, thrown by the regex. Now, it will be stringified first. In the case of None, it will now raise an InvalidVersion exception instead.

Markers: Use PEP 440 version comparison only when both sides are vali…

34e8b5b

…d version specifier.

vphilippon force-pushed the extra-pep440 branch from 2156649 to 34e8b5b Compare March 27, 2017 14:26

dstufft reviewed Apr 7, 2017

View reviewed changes

vphilippon commented Apr 8, 2017

View reviewed changes

In Version, catch TypeError from the regex, and raise InvalidVersion.

6c2e77c

This means no more str() of the version argument. Instead, we catch errors straight from the regex. Version(<not-a-string>) will raise InvalidVersion now.

vphilippon force-pushed the extra-pep440 branch from 9fe0312 to 6c2e77c Compare April 10, 2017 14:27

xavfernandez reviewed Apr 11, 2017

View reviewed changes

xavfernandez mentioned this pull request May 29, 2017

Make ordered comparisons work sanely for environment markers pypa/interoperability-peps#5

Open

brettcannon added the packaging.markers label Oct 17, 2020

Base automatically changed from master to main January 21, 2021 19:20

brettcannon added the bug label Jul 1, 2021

brettcannon closed this Aug 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

In Markers, use PEP 440 version comparison only if both sides are valid version specifier. #101

In Markers, use PEP 440 version comparison only if both sides are valid version specifier. #101

vphilippon commented Mar 24, 2017

vphilippon commented Mar 24, 2017 •

edited

xavfernandez commented Mar 26, 2017

vphilippon commented Mar 27, 2017

dstufft Apr 7, 2017

vphilippon Apr 7, 2017 •

edited

dstufft Apr 7, 2017

vphilippon Apr 7, 2017

vphilippon Apr 8, 2017

dstufft Apr 9, 2017

xavfernandez Apr 10, 2017

vphilippon Apr 10, 2017 •

edited

vphilippon commented Apr 11, 2017

xavfernandez Apr 11, 2017 •

edited

vphilippon Apr 11, 2017

xavfernandez Apr 11, 2017

vphilippon Apr 11, 2017 •

edited

vphilippon Apr 21, 2017 •

edited

vphilippon commented May 26, 2017 •

edited

brettcannon commented Aug 14, 2022

In Markers, use PEP 440 version comparison only if both sides are valid version specifier. #101

In Markers, use PEP 440 version comparison only if both sides are valid version specifier. #101

Conversation

vphilippon commented Mar 24, 2017

vphilippon commented Mar 24, 2017 • edited

xavfernandez commented Mar 26, 2017

vphilippon commented Mar 27, 2017

Choose a reason for hiding this comment

vphilippon Apr 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vphilippon Apr 10, 2017 • edited

Choose a reason for hiding this comment

vphilippon commented Apr 11, 2017

xavfernandez Apr 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vphilippon Apr 11, 2017 • edited

Choose a reason for hiding this comment

vphilippon Apr 21, 2017 • edited

Choose a reason for hiding this comment

vphilippon commented May 26, 2017 • edited

brettcannon commented Aug 14, 2022

vphilippon commented Mar 24, 2017 •

edited

vphilippon Apr 7, 2017 •

edited

vphilippon Apr 10, 2017 •

edited

xavfernandez Apr 11, 2017 •

edited

vphilippon Apr 11, 2017 •

edited

vphilippon Apr 21, 2017 •

edited

vphilippon commented May 26, 2017 •

edited