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

bump idna has version 3.0 was released #5711

Merged
merged 3 commits into from Jul 7, 2021
Merged

Conversation

naorlivne
Copy link
Contributor

@naorlivne naorlivne commented Jan 1, 2021

Closes #5710

@nateprewitt
Copy link
Member

Hi @naorlivne,

Thanks for the PR. I think this looks fine but there's some mild concern about users on Python 2. idna 3.0 drops Python 2 support and has added an appropriate python_requires specifier so in most cases this change will work fine. Users still on Python 2 though tend to have more outdated versions of pip, potentially earlier than 9.0 which may make this breaking.

I'm wondering if we need to add an environment marker splitting out the dependency range for Python 2 and 3.

@sigmavirus24
Copy link
Contributor

@nateprewitt the real solution here is for @naorlivne to not try to force dependencies to upgrade when they're untested with other projects (as you can see in the original report, they upgraded idna without regard for their other dependencies).

@naorlivne
Copy link
Contributor Author

@sigmavirus24 what do you suggest? the alternative is that idna can never be upgraded when using requests from this moment on which opens a whole range of issues as no future bugfixes ( including security fixes) in it will be possible to implement when using requests

This change doesn't force anyone to use the latest version as it simply increase the range of accepted versions of idna, it doesn't lock the version to 3.0 but simply allows it to be used.

@nanonyme
Copy link
Contributor

nanonyme commented Jan 2, 2021

@nateprewitt note that the real thing to ponder upon here is that requests dependencies are dropping Python 2 support. Requests needs to start preparing for a future where it also must drop Python 2 support. On the whole I don't see harm with @naorlivne's change though assuming there's no API incompatibilities.

@sigmavirus24
Copy link
Contributor

This change doesn't force anyone to use the latest version as it simply increase the range of accepted versions of idna, it doesn't lock the version to 3.0 but simply allows it to be used.

Semantically speaking, that range isn't correct for all users of Requests though. Which is why @nateprewitt is pondering using python_version to manage the dependencies which will be a nightmare for maintenance.

note that the real thing to ponder upon here is that requests dependencies are dropping Python 2 support. Requests needs to start preparing for a future where it also must drop Python 2 support.

That's called Requests 3.0

On the whole I don't see harm with @naorlivne's change though assuming there's no API incompatibilities.

The scant notes don't imply any API incompatibilities: https://github.com/kjd/idna/blob/master/HISTORY.rst#30-2021-01-01 which doesn't guarantee there aren't any

@naorlivne
Copy link
Contributor Author

Semantically speaking, that range isn't correct for all users of Requests though. Which is why @nateprewitt is pondering using python_version to manage the dependencies which will be a nightmare for maintenance.

It will happen more and more, this is just the first but a lot of packages are deprecating support for Python 2.x so this problem will keep pop up with other dependencies as well so the options are:

  1. That maintenance nightmare (which I think we all agree is indeed a nightmare) will happen on more and more packages
  2. Requests stops supporting Python 2.x
  3. Every package that drops support to Python 2.x gets version locked which will cause serious security issues as no fixes to them will be made

That's called Requests 3.0

It's possible this will need to be merged to Requests 3.0 if the decision is that's the version where python 2.x support will be dropped, however I noticed in #5660 that there's planned to drop support for Python 3.5 as it's EOL and I do find it weird we are keeping version 2.x support and a much newer version support is dropped, I also think that unless Requests 3.0 is right around the corner it might not be a good idea to wait a long time with this as the problem will only worsen as more time pass.

On the whole I don't see harm with @naorlivne's change though assuming there's no API incompatibilities.

The scant notes don't imply any API incompatibilities: https://github.com/kjd/idna/blob/master/HISTORY.rst#30-2021-01-01 which doesn't guarantee there aren't any

The only guarantees in life is death and taxes, we can only work on what's known (including known unknowns) and given that there hasn't been any declared API changes & all tests passed I don't think a worry of a "what if" possibly caused by an unknown unknown should be factored in.

@sigmavirus24
Copy link
Contributor

The only guarantees in life is death and taxes, we can only work on what's known (including known unknowns) and given that there hasn't been any declared API changes & all tests passed I don't think a worry of a "what if" possibly caused by an unknown unknown should be factored in.

Ah, yes. What a package trying to provide stability to governments, corporations, and hobbyists alike needs - flippant attitudes towards functionality

@nanonyme
Copy link
Contributor

nanonyme commented Jan 2, 2021

Regarding request 3, what is the right place to follow the roadmap?

@naorlivne
Copy link
Contributor Author

@sigmavirus24 What do you suggest instead of upgrading then? this change has passed all tests and there is no documented API changes in idna anywhere official or unofficial... do you suggest we version lock all packages forever as we may never know 100% noting broke at any change?

@sigmavirus24
Copy link
Contributor

do you suggest we version lock all packages forever as we may never know 100% noting broke at any change?

No I don't. I suggest taking greater care

@naorlivne
Copy link
Contributor Author

Can you give suggestion to what greater care you think is needed then? Aside from making sure all tests pass and going through the release data of the new package version I'm not sure what else can be done.

setup.py Outdated Show resolved Hide resolved
Co-authored-by: Mickaël Schoentgen <contact@tiger-222.fr>
@naorlivne
Copy link
Contributor Author

@BoboTiG didn't know it's possible to have a python version if in the requirements,txt... guess you learn something every day right?

Anyway the suggested edit been made, is there anything else needed before this can be merged?

@BoboTiG
Copy link
Contributor

BoboTiG commented Jan 5, 2021

FTR @naorlivne here are all markers you can use in requirements files: PEP 496 ;)

@naorlivne
Copy link
Contributor Author

Is there anything else needed for me to do regarding this ticket before this can be approved & merged?

@jschlyter
Copy link

It would be most useful if these could be merged for the next release, we're also stuck with issues due to idna dependencies.

@maqp
Copy link

maqp commented Jan 20, 2021

I'm also looking forward to this ticket being fixed. It's the only requirement blocking maintenance update. I of course totally understand if you're busy! :)

@jayaddison
Copy link

@sigmavirus24 Can you provide any guidance for things that people should look out for in the upgrade from idna version 2.10 to 3.1?

@sethmlarson
Copy link
Member

@jschlyter It's likely this PR will land in the next release. Pinging this PR doesn't change anything except generate noise to 1400 people. Locking this discussion to contributors to avoid this happening again.

@jayaddison Requests maintainers won't provide any guidance here, instead you should read the release notes for idna. From my reading there are unlikely to be functional changes for most users.

@psf psf locked as off-topic and limited conversation to collaborators Jan 20, 2021
@nateprewitt nateprewitt added this to the 2.26.0 milestone May 20, 2021
Copy link
Member

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

I'm happy with this PR in it's current state.

@sethmlarson sethmlarson merged commit 33cf965 into psf:master Jul 7, 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.

idna 3.0 version package conflict
9 participants