Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d79297a
In Version, convert version to string before applying the regex.
vphilippon Mar 24, 2017
34e8b5b
Markers: Use PEP 440 version comparison only when both sides are vali…
vphilippon Mar 24, 2017
6c2e77c
In Version, catch TypeError from the regex, and raise InvalidVersion.
vphilippon Apr 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ Changelog

.. note:: This version is not yet released and is under active development.

* Fix a bug where ``packaging.markers.Marker.evaluate`` used PEP 440
version comparison when the left side is not a valid version specifier.
* Drop support for python 2.6 and 3.2


Expand Down
6 changes: 5 additions & 1 deletion packaging/markers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

from ._compat import string_types
from .specifiers import Specifier, InvalidSpecifier
from .version import Version, InvalidVersion


__all__ = [
Expand Down Expand Up @@ -182,8 +183,11 @@ def _format_marker(marker, first=True):

def _eval_op(lhs, op, rhs):
try:
# PEP 508: Use the PEP-440 version comparison rules
# when both sides have a valid version specifier.
spec = Specifier("".join([op.serialize(), rhs]))
except InvalidSpecifier:
lhs = Version(lhs)
except (InvalidSpecifier, InvalidVersion):
pass
else:
return spec.contains(lhs)
Expand Down
11 changes: 9 additions & 2 deletions packaging/version.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ def parse(version):
return LegacyVersion(version)


class InvalidVersion(ValueError):
class InvalidVersion(Exception):
"""
An invalid version was found, users should refer to PEP 440.
"""
Expand Down Expand Up @@ -198,7 +198,14 @@ class Version(_BaseVersion):

def __init__(self, version):
# Validate the version and parse it into pieces
match = self._regex.search(version)
try:
match = self._regex.search(version)
except TypeError:
raise InvalidVersion(
"Invalid type: '{0}' of type '{1}' is not a string".format(
version, type(version)
)
)
if not match:
raise InvalidVersion("Invalid version: '{0}'".format(version))

Expand Down
40 changes: 40 additions & 0 deletions tests/test_markers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,46 @@ def test_extra_with_no_extra_in_environment(self):
{"extra": "security"},
True,
),
(
"extra == 'security'",
{"extra": None},
False,
),
(
"extra == '3.7.0'",
{"extra": None},
False,
),
(
"extra == '3.7.0'",
{"extra": "foobar"},
False,
),
(
"extra == '3.7.0'",
{"extra": "3.7.0"},
True,
),
(
"extra == '3.7.0'",
{"extra": "3.7"},
True,
),
(
"extra == '3.7.0'",
{"extra": "3.7.0.0"},
True,
),
(
"extra == '3.7.0'",
{"extra": "3.7.0+youaregreat"},
True,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

),
(
"extra == '3.7.0'",
{"extra": "3.7.1"},
False,
),
],
)
def test_evaluates(self, marker_string, environment, expected):
Expand Down
18 changes: 18 additions & 0 deletions tests/test_version.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
[
("1.0", Version),
("1-1-1", LegacyVersion),
(None, LegacyVersion),
],
)
def test_parse(version, klass):
Expand Down Expand Up @@ -53,6 +54,23 @@ def test_valid_versions(self, version):
@pytest.mark.parametrize(
"version",
[
# None is not a valid version
None,

# Numbers (not strings) are not valid versions
0,
1,
1.0,
1.1,

# Object that are not string are not valid versions
[],
[1],
{},
{'1': 1},
tuple(),
(1, 2),

# Non sensical versions should be invalid
"french toast",

Expand Down