Skip to content

Conversation

@bdarnell
Copy link
Contributor

Code depending on this conditional import could break if an old
version of Tornado is present in the environment, rendering pip
unusable.

Fixes #10020

Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

Thanks for taking this up!

@@ -0,0 +1 @@
The presence of an old version of Tornado (before 4.1) in the environment no longer breaks pip. No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

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

Describe the problem in past tense, and the fix in present tense (because that's what makes sense after the release is made). So maybe something like

Remove unused optional tornado import in vendored tenacity to avoid an old version or tornado installation from breaking pip.

import tornado
# Pip does not currently vendor Tornado, so this will take the
# ImportError path.
from pip._vendor import tornado
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to patch the entire block to tornado = None. If the import always fails, we don't need to try that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to make that change. It was my original plan, but I decided to use from pip._vendor import tornado so that in the (unlikely) event pip adds Tornado to its vendored dependencies things will just work and you won't be left with a copy of tenacity that mostly works but breaks if it interacts with tornado.

except ImportError:
tornado = None


No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

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

Need to fix this lack of trailing newline.

@bdarnell bdarnell force-pushed the develop branch 2 times, most recently from 17f2f7e to 3b655ef Compare May 29, 2021 15:52
@bdarnell
Copy link
Contributor Author

Branch is updated with all requested changes.

except ImportError:
tornado = None
# Pip does not currently vendor Tornado, so don't import it even if it
# happens to be installed in the environment.
Copy link
Member

Choose a reason for hiding this comment

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

If I read this comment for the first time, I’d think to myself “OK but that works without the patch as well, so is this unnecessary”?

I’d mention why the patch is needed instead to avoid this confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Code depending on this conditional import could break if an old
version of Tornado is present in the environment, rendering pip
unusable.
@uranusjr uranusjr merged commit 5554432 into pypa:main May 31, 2021
@sbidoul sbidoul added this to the 21.1.3 milestone May 31, 2021
@pradyunsg
Copy link
Member

Thank you @bdarnell! ^>^

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Vendoring of tenacity is leaky

5 participants