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.

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

pip 21.0.1 fails when run with warnings converted to errors #9540

Closed
pfmoore opened this issue Jan 30, 2021 · 15 comments · Fixed by #9779
Closed

pip 21.0.1 fails when run with warnings converted to errors #9540

pfmoore opened this issue Jan 30, 2021 · 15 comments · Fixed by #9779
Labels
project: setuptools Related to setuptools type: bug A confirmed bug or unintended behavior type: maintenance Related to Development and Maintenance Processes

Comments

@pfmoore
Copy link
Member

pfmoore commented Jan 30, 2021

Environment

  • pip version: 21.0.1
  • Python version: 3.9.1
  • OS: Windows

Description
With the latest version of packaging (vendored in 21.0.1) a DeprecationWarning is issued when parsing a "legacy version". If pip is run with warnings converted to errors, this causes a failure.

Expected behavior
No error

How to Reproduce
py -wE -m pip --version

Or to pinpoint it further,

py -wE
>>> from pip._vendor import pkg_resources

This does not happen with setuptools 52.0.0, it appears to be related to the version of setuptools (44.0.0) that we vendor.

Output

Traceback (most recent call last):
  File "C:\Users\Gustav\AppData\Local\Programs\Python\Python39\lib\site-packages\pip\_vendor\packaging\version.py", line 57, in parse
    return Version(version)
  File "C:\Users\Gustav\AppData\Local\Programs\Python\Python39\lib\site-packages\pip\_vendor\packaging\version.py", line 298, in __init__
    raise InvalidVersion("Invalid version: '{0}'".format(version))
pip._vendor.packaging.version.InvalidVersion: Invalid version: 'pip'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\Gustav\AppData\Local\Programs\Python\Python39\lib\site-packages\pip\_vendor\pkg_resources\__init__.py", line 3252, in <module>
    def _initialize_master_working_set():
  File "C:\Users\Gustav\AppData\Local\Programs\Python\Python39\lib\site-packages\pip\_vendor\pkg_resources\__init__.py", line 3235, in _call_aside
    f(*args, **kwargs)
  File "C:\Users\Gustav\AppData\Local\Programs\Python\Python39\lib\site-packages\pip\_vendor\pkg_resources\__init__.py", line 3264, in _initialize_master_working_set
    working_set = WorkingSet._build_master()
  File "C:\Users\Gustav\AppData\Local\Programs\Python\Python39\lib\site-packages\pip\_vendor\pkg_resources\__init__.py", line 574, in _build_master
    ws = cls()
  File "C:\Users\Gustav\AppData\Local\Programs\Python\Python39\lib\site-packages\pip\_vendor\pkg_resources\__init__.py", line 567, in __init__
    self.add_entry(entry)
  File "C:\Users\Gustav\AppData\Local\Programs\Python\Python39\lib\site-packages\pip\_vendor\pkg_resources\__init__.py", line 623, in add_entry
    for dist in find_distributions(entry, True):
  File "C:\Users\Gustav\AppData\Local\Programs\Python\Python39\lib\site-packages\pip\_vendor\pkg_resources\__init__.py", line 2061, in find_on_path
    path_item_entries = _by_version_descending(filtered)
  File "C:\Users\Gustav\AppData\Local\Programs\Python\Python39\lib\site-packages\pip\_vendor\pkg_resources\__init__.py", line 2034, in _by_version_descending
    return sorted(names, key=_by_version, reverse=True)
  File "C:\Users\Gustav\AppData\Local\Programs\Python\Python39\lib\site-packages\pip\_vendor\pkg_resources\__init__.py", line 2032, in _by_version
    return [packaging.version.parse(part) for part in parts]
  File "C:\Users\Gustav\AppData\Local\Programs\Python\Python39\lib\site-packages\pip\_vendor\pkg_resources\__init__.py", line 2032, in <listcomp>
    return [packaging.version.parse(part) for part in parts]
  File "C:\Users\Gustav\AppData\Local\Programs\Python\Python39\lib\site-packages\pip\_vendor\packaging\version.py", line 59, in parse
    return LegacyVersion(version)
  File "C:\Users\Gustav\AppData\Local\Programs\Python\Python39\lib\site-packages\pip\_vendor\packaging\version.py", line 127, in __init__
    warnings.warn(
DeprecationWarning: Creating a LegacyVersion has been deprecated and will be removed in the next major release
@pfmoore
Copy link
Member Author

pfmoore commented Jan 30, 2021

This causes the CPython test suite to fail (at least prior to 3.10, I'm not entirely sure why the tests don't fail in cpython master, but ensurepip changed in that version).

This is blocking the backport upgrading ensurepip to 21.0.1 in CPython 3.8 and 3.9 🙁 Not upgrading ensurepip isn't a showstopper, though. I'm fine with leaving this fix for 21.1.

Possible fixes - remove the pin on setuptools in our vendoring (does this have any risk, or was it just pinned for Python 2 support?) or get on with removing our dependency on pkg_resources.

@pfmoore
Copy link
Member Author

pfmoore commented Jan 30, 2021

... or a 3rd option, we could just override the Python warning settings in __main__.py. (Following the "pip is an application" principle, an environment variable like PYTHONWARNINGS shouldn't affect us anyway...)

@uranusjr
Copy link
Member

What is causing the warning in the first place? Presumably there is something in the environment that uses non-PEP-440 versions? If that's the case, it would again be a problem when packaging drop LegacyVersion. We need to either drop support for non-PEP-440 versions altogether, or invent an alternative to the class at that point. If the former, the warning is relevant and the target environment should work to eliminate whatever is using a non-PEP-440 version. And if the latter… we'll probably start inventing something now.

@pfmoore
Copy link
Member Author

pfmoore commented Jan 30, 2021

I haven't had the time to dig that deeply. Note that the invalid "version" is "pip", which suggests that something parsed the wrong part of a filename, or something like that. As it doesn't happen when importing pkg_resources from the latest setuptools, I'm inclined to suspect a setuptools bug that has since been fixed.

@uranusjr
Copy link
Member

uranusjr commented Jan 31, 2021

I took a look at setuptools, it seems that significant has changed between the versions on this code path. setuptools does not emit a warning simply because it vendors packaging 20.4, before the deprecation warning was introduced 😞

So I guess the most reasonable approach is to either silence the warning (probably by patching pkg_resources; I think it’d be better to still emit that deprecation warning on other usages of LegacyVersion in pip’s code base).

@pradyunsg
Copy link
Member

does this have any risk, or was it just pinned for Python 2 support?

It was pinned for Python 2, although setuptools has moved forward a lot and we'd likely want to update what all we trim from their source code in the vendoring configuration.

@uranusjr
Copy link
Member

Upgrading the vendored setuptools will not resolve the issue. The difference between pip and setuptools is setuptools vendors a version of packaging that does not deprecate LegacyVersion.

@pfmoore
Copy link
Member Author

pfmoore commented Jan 31, 2021

The difference between pip and setuptools is setuptools vendors a version of packaging that does not deprecate LegacyVersion.

Ah. I hadn't spotted that.

@pfmoore
Copy link
Member Author

pfmoore commented Jan 31, 2021

OK, I had some time to debug. Setuptools is the issue, in _by_version_descending, the nested function _by_version parses every part of the filename, to make a sort key.

TBH, I'm not even sure how the code achieves what the docstring says it does. I think it's relying on the fact that LegacyVersion objects sort like normal strings, but it may well also be trying to handle non-PEP 440 versions somehow.

    def _by_version(name):
        """
        Parse each component of the filename
        """
        name, ext = os.path.splitext(name)
        parts = itertools.chain(name.split('-'), [ext])
        return [packaging.version.parse(part) for part in parts]

The following fixes it:

    def _by_version(name):
        """
        Parse each component of the filename
        """
        name, ext = os.path.splitext(name)
        parts = itertools.chain(name.split('-'), [ext])
        def safe_parse(s):
            try:
                return packaging.version.parse(s)
            except (packaging.version.InvalidVersion, DeprecationWarning):
                return s
        return [safe_parse(part) for part in parts]

but I don't like (1) that it's not obvious to me that it does the same as the original in edge cases, and (2) that we have to catch DeprecationWarning for the "warnings as errors" case.

I've submitted pypa/setuptools#2547 to let them know about the issue, but it's not likely to hit them soon, so I think we need a local solution for now.

@pradyunsg
Copy link
Member

I think our local solution should be to switch to importlib.metadata. 🙃

@pfmoore
Copy link
Member Author

pfmoore commented Jan 31, 2021

Agreed, but is anyone actively working on that?

A short term fix (which I just tested, briefly) is to add

warnings.filterwarnings("ignore", category=DeprecationWarning, module=".*packaging\\.version")

to __main__.py.

It might also be worth our while adding a test to the test suite, just to check that python -We -m pip --version doesn't fail. I don't suggest that we support running pip with -We as a general thing, but not having the CPython test suite fail would be good. I'm not sure we want to go any further (actually doing an install with -We) though.

@pfmoore
Copy link
Member Author

pfmoore commented Jan 31, 2021

As I say, though, I don't think this is a big enough deal to do a bugfix release, so there's no rush to fix it before 21.1. I'm fine with 21.0.1 not being backported into 3.8 and 3.9.

It's worth noting, though, that there are scheduled Python 3.8 and 3.9 releases on 2021-04-19, with the RC on 2021-04-05, so getting pip 21.1 into those will be a bit tight. And that 3.8 release is the final bugfix release, so if we miss that, 3.8 will never get a newer version of pip than 20.2.3. I'm going to count that as a problem for the 21.1 RM, though 🙂

@uranusjr
Copy link
Member

uranusjr commented Jan 31, 2021

I think our local solution should be to switch to importlib.metadata. 🙃

Agreed, but is anyone actively working on that?

Things are blocked by #8114, I’m waiting for a good timing to merge it. But even after merging the PR, the transition likely won’t finish for 21.1 (unless significant resources being suddenly made available), so better not count on that being the solution.

@pradyunsg pradyunsg added project: setuptools Related to setuptools type: bug A confirmed bug or unintended behavior type: maintenance Related to Development and Maintenance Processes labels Jan 31, 2021
@hexagonrecursion
Copy link
Contributor

Related #9250

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
project: setuptools Related to setuptools type: bug A confirmed bug or unintended behavior type: maintenance Related to Development and Maintenance Processes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants
@uranusjr @pfmoore @pradyunsg @hexagonrecursion and others