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

urllib3 2.0.0 breaks twine #989

Closed
tra-github opened this issue Apr 28, 2023 · 12 comments
Closed

urllib3 2.0.0 breaks twine #989

tra-github opened this issue Apr 28, 2023 · 12 comments

Comments

@tra-github
Copy link

tra-github commented Apr 28, 2023

Your Environment

Thank you for taking the time to report an issue.

To more efficiently resolve this issue, we'd like to know some basic information about your system and setup.

  1. Your operating system: Ubuntu 20.04.5 LTS

  2. Version of python you are running: Python 3.8.10

  3. How did you install twine? Did you use your operating system's package manager or pip or something else?
    python3 -m pip install --upgrade twine

  4. Version of twine you have installed (include complete output of): twine-4.0.2

  5. Which package repository are you targeting? Nexus

The Issue

Excerpt from our Jenkins pipeline console log:

16:55:44 Successfully installed Pygments-2.15.1 SecretStorage-3.3.3 bleach-6.0.0 cffi-1.15.1 cryptography-40.0.2 docutils-0.19 importlib-metadata-6.6.0 importlib-resources-5.12.0 jaraco.classes-3.2.3 jeepney-0.8.0 keyring-23.13.1 markdown-it-py-2.2.0 mdurl-0.1.2 more-itertools-9.1.0 pkginfo-1.9.6 pycparser-2.21 readme-renderer-37.3 requests-toolbelt-0.10.1 rfc3986-2.0.0 rich-13.3.5 twine-4.0.2 typing-extensions-4.5.0 urllib3-2.0.0 webencodings-0.5.1 zipp-3.15.0
16:55:44 /usr/lib/python3/dist-packages/requests/init.py:89: RequestsDependencyWarning: urllib3 (2.0.0) or chardet (3.0.4) doesn't match a supported version!
16:55:44 warnings.warn("urllib3 ({}) or chardet ({}) doesn't match a supported "
16:55:44 Traceback (most recent call last):
16:55:44 File "/usr/local/lib/python3.8/dist-packages/requests_toolbelt/_compat.py", line 48, in
16:55:44 from requests.packages.urllib3.contrib import appengine as gaecontrib
16:55:44 ImportError: cannot import name 'appengine' from 'urllib3.contrib' (/usr/local/lib/python3.8/dist-packages/urllib3/contrib/init.py)
16:55:44
16:55:44 During handling of the above exception, another exception occurred:
16:55:44
16:55:44 Traceback (most recent call last):
16:55:44 File "/usr/local/bin/twine", line 8, in
16:55:44 sys.exit(main())
16:55:44 File "/usr/local/lib/python3.8/dist-packages/twine/main.py", line 33, in main
16:55:44 error = cli.dispatch(sys.argv[1:])
16:55:44 File "/usr/local/lib/python3.8/dist-packages/twine/cli.py", line 121, in dispatch
16:55:44 main = registered_commands[args.command].load()
16:55:44 File "/usr/local/lib/python3.8/dist-packages/importlib_metadata/init.py", line 210, in load
16:55:44 module = import_module(match.group('module'))
16:55:44 File "/usr/lib/python3.8/importlib/init.py", line 127, in import_module
16:55:44 return _bootstrap._gcd_import(name[level:], package, level)
16:55:44 File "", line 1014, in _gcd_import
16:55:44 File "", line 991, in _find_and_load
16:55:44 File "", line 975, in _find_and_load_unlocked
16:55:44 File "", line 671, in _load_unlocked
16:55:44 File "", line 848, in exec_module
16:55:44 File "", line 219, in _call_with_frames_removed
16:55:44 File "/usr/local/lib/python3.8/dist-packages/twine/commands/upload.py", line 26, in
16:55:44 from twine import settings
16:55:44 File "/usr/local/lib/python3.8/dist-packages/twine/settings.py", line 22, in
16:55:44 from twine import repository
16:55:44 File "/usr/local/lib/python3.8/dist-packages/twine/repository.py", line 18, in
16:55:44 import requests_toolbelt
16:55:44 File "/usr/local/lib/python3.8/dist-packages/requests_toolbelt/init.py", line 12, in
16:55:44 from .adapters import SSLAdapter, SourceAddressAdapter
16:55:44 File "/usr/local/lib/python3.8/dist-packages/requests_toolbelt/adapters/init.py", line 12, in
16:55:44 from .ssl import SSLAdapter
16:55:44 File "/usr/local/lib/python3.8/dist-packages/requests_toolbelt/adapters/ssl.py", line 16, in
16:55:44 from .._compat import poolmanager
16:55:44 File "/usr/local/lib/python3.8/dist-packages/requests_toolbelt/_compat.py", line 50, in
16:55:44 from urllib3.contrib import appengine as gaecontrib
16:55:44 ImportError: cannot import name 'appengine' from 'urllib3.contrib' (/usr/local/lib/python3.8/dist-packages/urllib3/contrib/init.py)

Steps to Reproduce

If the issue is predictable and consistently reproducible, please list the steps here.

@tra-github
Copy link
Author

tra-github commented Apr 28, 2023

In twine/setup.cfg:
urllib3 >= 1.26.0, < 2.0.0

@sigmavirus24
Copy link
Member

In twine/setup.cfg:
urllib3 >= 1.26.0, < 2.0.0

See urllib3/urllib3#2997 (comment)

No upper cap here is not an issue since requests has one itself. Something else is forcing your installation higher, not twine.

@lynshi
Copy link

lynshi commented Apr 28, 2023

Had the same problem today in Azure DevOps. Maybe the default urllib3 got bumped, but installing twine via pip install twine "urllib3>=1.26.0,<2.0.0" allows twine upload to keep working.

With that in mind, I think it makes sense to put an upper restriction from within twine. You shouldn't depend on a dependency to enforce the restriction for you.

bigcat88 added a commit to bigcat88/pillow_heif that referenced this issue Apr 30, 2023
Signed-off-by: bigcat88 <[email protected]>
@bhrutledge
Copy link
Contributor

Broken CI and dependency conflicts are a bummer, but I don't think adding urllib3>=1.26.0,<2.0.0 to Twine's setup.cfg will resolve your issues.

As @sigmavirus24 noted, there's already an implicit upper bound from requests: urllib3>=1.21.1,<1.27. In a fresh virtual environment, we can see that this behaves as expected.

% python3 -m venv twine-venv

% source twine-venv/bin/activate

% pip install --no-cache-dir twine

% pip freeze | grep urllib3
urllib3==1.26.15

% pip install pipdeptree

% pipdeptree -rp urllib3
urllib3==1.26.15
  - requests==2.29.0 [requires: urllib3>=1.21.1,<1.27]
    - requests-toolbelt==0.10.1 [requires: requests>=2.0.1,<3.0.0]
      - twine==4.0.2 [requires: requests-toolbelt>=0.8.0,!=0.9.0]
    - twine==4.0.2 [requires: requests>=2.20]
  - twine==4.0.2 [requires: urllib3>=1.26.0]

Since urllib3<1.27 is already part of Twine's dependency tree, pip install should respect that. In fact, I get an error if I try to install the latest versions of Twine and urllib3:

% pip install 'twine>=4' 'urllib3>=2'

ERROR: ResolutionImpossible: for help visit https://pip.pypa.io/en/latest/topics/dependency-resolution/#dealing-with-dependency-conflicts

That said, I'm surprised to see urllib3-2.0.0 in @tra-github's install output. Again, as @sigmavirus24 noted, it's likely that "something else is forcing your installation higher". I wonder if your environment has an outdated version of pip that's using the old dependency resolver, which would happily install conflicting versions.

@sigmavirus24
Copy link
Member

You shouldn't depend on a dependency to enforce the restriction for you.

By that logic, we shouldn't depend on pip to resolve dependencies either. Clearly that's not working.

Furthermore, @tra-github seems to be doing something very bizarre based on the output. For one thing, they're using pip to install ... something (they haven't given us what pip was trying to install in Jenkins) and they're executing twine from /usr/local/bin which may be a pre-existing installation via another package manager. They seem to have a mixture of system installed dependencies as well as non-system installed dependencies. This is a known bad pattern in python in general. I suspect, they would have their issue resolved with better Jenkins management (virtualenv usage, docker container, pretty much anything to make the build process repeatable and not dependent upon the state of the overall build system runner).

Without other information, there's no way to guess at what others are running into.


Aside from that, I don't remember why we added urllib3 to the dependency list, but I do remember suggesting that something like this could happen and be construed as that being the cause - causing us a headache. I vaguely remember the justification being ideological rather than practical. I don't believe that removing the line will fix this, but it will ensure we don't get this bug hung on our doorstep again.

@lynshi
Copy link

lynshi commented Apr 30, 2023

You shouldn't depend on a dependency to enforce the restriction for you.

By that logic, we shouldn't depend on pip to resolve dependencies either. Clearly that's not working.

Sorry, let me clarify my point. If your library explicitly uses urllib3 for something and urllib3>=2.0.0 breaks the functionality you use directly, it makes sense to set an upper bound because you know that a later version breaks your package. If you are simply consuming urllib3 transitively through some other library (presumably requests-toolbelt), then yeah I agree, you don't need to set a bound :)

In fact, if you are not using urllib3 explicitly, I would agree that in principle you don't need to list it in setup.cfg at all (and based on the next section, this might be the best solution). At the same time, since twine is often used as an executable, it can make some sense to include explicit bounds for known breakages in order to improve the user experience. Of course there are some consequences. For example, if one of your pinned dependencies has a security vulnerability resolved in a newer version, someone may open an issue telling you to update your requirements list, a situation you avoid by simply not pinning.


I wonder if your environment has an outdated version of pip

This is the case for me in Azure DevOps. pip --version outputs pip 20.0.2. For just pip install twine, the output is as follows:

Collecting urllib3>=1.26.0
  Downloading urllib3-2.0.1-py3-none-any.whl (123 kB)

This suggests that pip is only considering the version restrictions in twine and ignoring those from requests. Explicitly setting the urllib3 version via pip install twine "urllib3>=1.26.0,<2.0.0" results in

Collecting urllib3<2.0.0,>=1.26.0
  Downloading urllib3-1.26.15-py2.py3-none-any.whl (140 kB)

This might suggest that if you don't use urllib3 directly at all, removing it completely from setup.cfg would avoid this problem, because (the older) pip may then just transitively install the correct version of urllib3 specified in requests.


Since I don't know how your library is using urllib3, I can't say whether you should be setting an upper bound for urllib3. I only came here since I was searching up error strings to figure out why my build broke. As stated above, pip install twine "urllib3>=1.26.0,<2.0.0" is the easy solution to this 😄

@lynshi
Copy link

lynshi commented Apr 30, 2023

At the same time, since twine is often used as an executable, it can make some sense to include explicit bounds for known breakages in order to improve the user experience.

To emphasize the confusion that can be caused, I noticed this issue was already reported in #981. I don't mean to prescribe a solution, but I think omitting urllib3 completely or setting an upper bound are equally justifiable in the sense that both are logically sound and your usage of urllib3 should naturally gravitate you towards one or the other. But setting just the lower bound can definitely lead to user confusion, and either extreme of removing/upper-bounding will make it more clear what your intent is and reduce user confusion.

@sigmavirus24
Copy link
Member

So to be clear, a modern version of pip would solve this issue, but twine is responsible because some environments default to a version published over 3 years ago. Exactly how long is the ecosystem supposed to support these otherwise unsupported versions and their bad behavior?

@lynshi
Copy link

lynshi commented May 1, 2023

Sure, that's one way to think about it. But the core of the problem seems useful to address regardless? If you don't use urllib3 or your usage is not dependent on the urllib3 version, you can just say so and do whatever you want versioning wise. If you use it in a way that is dependent on an older version, then you should set an upper bound.

I'm not sure why you are being so combative. Even though this issue was caused by out-of-date environments, that's tangential to whether your versioning can or should be improved from a purely principled perspective. Frankly, it doesn't matter to me what you decide to do since finding this issue was enough for me to figure out how to fix my pipeline, but it is a little odd that the tone for this discussion is so negative.

I'm definitely sorry for initially suggesting that the lack of upper bound is your fault without any insight into how you use urllib3, but surely that's a minor misunderstanding, and you can easily clarify one way or the other to resolve the discussion more amicably.

Edit: I mainly started commenting because the initial response was that versioning in requests should have prevented this, which didn't make sense to me so I just remarked on it. If you said something like "twine doesn't have an explicit upper requirement so we chose not to list it", I would've bought that as an explanation and left the versioning statement out of my comment. Sorry if it felt too much like an accusation :)

@bhrutledge
Copy link
Contributor

@lynshi Out of curiosity, I tried installing Twine in fresh venv with an old version of pip:

% pip install pip==20.0.2

% pip install twine
...
Collecting urllib3>=1.26.0
  Using cached urllib3-2.0.1-py3-none-any.whl (123 kB)
...
ERROR: requests 2.29.0 has requirement urllib3<1.27,>=1.21.1, but you'll have urllib3 2.0.1 which is incompatible.

Upgrading pip resolves the error:

% pip install -U pip
...
Successfully installed pip-23.1.2

% pip install twine
...
Installing collected packages: urllib3
  Attempting uninstall: urllib3
    Found existing installation: urllib3 2.0.1
    Uninstalling urllib3-2.0.1:
      Successfully uninstalled urllib3-2.0.1
Successfully installed urllib3-1.26.15

I don't know if it's documented anywhere, but I think it's generally a good practice to pip install -U pip whenever you create a fresh Python runtime environment (venv, CI, Docker, etc.)


It looks like Twine only uses urllib3 directly for retries:

def _make_adapter_with_retries() -> adapters.HTTPAdapter:
retry = urllib3.Retry(
allowed_methods=["GET"],
connect=5,
total=10,
status_forcelist=[500, 501, 502, 503],
)
return adapters.HTTPAdapter(max_retries=retry)

I'm guessing that functionality isn't broken in urllib3>=2, so from Twine's perspective, the lack of an upper bound in setup.cfg is semantically correct. Ultimately, I think the behavior you're seeing could be considered a bug in pip, that has been resolved in newer versions. I'm also still not convinced that specifying an upper bound in Twine's setup.cfg would resolve the issue.

Furthermore, the root cause of this issue seems to be a bug in requests-toolbelt, that is resolved and is pending release.

@lynshi
Copy link

lynshi commented May 1, 2023

It looks like Twine only uses urllib3 directly for retries:
I'm guessing that functionality isn't broken in urllib3>=2, so from Twine's perspective, the lack of an upper bound in setup.cfg is semantically correct

Thanks for the clarification! Totally agree with you then :)

I was just making an offhand comment because it didn't sound right to me that the responsibility for setting an upper bound was punted to requests without any reasoning, but with this context it doesn't make sense for twine to set the upper bound.

I don't know if it's documented anywhere, but I think it's generally a good practice to pip install -U pip whenever you create a fresh Python runtime environment (venv, CI, Docker, etc.)

Yep, thanks for the suggestion! It's just that the pipeline I had was a tiny one so I didn't bother going for total correctness which is completely my fault 😅

@pquentin
Copy link

pquentin commented May 1, 2023

requests-toolbelt 1.0.0 is out with urllib3 2.0 support: https://pypi.org/project/requests-toolbelt/1.0.0/ Fun fact: I uploaded the release with twine 4.0.2, requests 2.29.0, requests-toolbelt 1.0.0 and urllib3 2.0.1.

I also agree that twine should not enforce the upper bound of urllib3, but I made the release quickly anyway because requests will allow urllib3 2.0 soon.

I believe this issue can be closed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants