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

Do not error if use_2to3 is set to a false value #2777

Merged
merged 4 commits into from
Sep 8, 2021
Merged

Do not error if use_2to3 is set to a false value #2777

merged 4 commits into from
Sep 8, 2021

Conversation

plumdog
Copy link
Contributor

@plumdog plumdog commented Sep 7, 2021

Summary of changes

I think this slight change fixes an issue where setting use_2to3=False would fail fast when really it ought to be ignored.

I think this results in behaviour closer to what I had in mind with #2769.

Pull Request Checklist

  • Changes have tests - no because I can't see where one would go
  • News fragment added in changelog.d/.

@jimmyhli
Copy link

jimmyhli commented Sep 7, 2021

should 58.0.2 be yanked? I assume this will go into a 58.0.3

cfm added a commit to freedomofpress/securedrop that referenced this pull request Sep 7, 2021
The alternative appears to be to wait for setuptools > 58.0.2, per
pypa/setuptools#2777.  But this seems like the more-forward way.
setuptools/dist.py Outdated Show resolved Hide resolved
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

In general, I think it's a move in the right direction but could you also add a regression test for this scenario? I'd like to see both positive and negative test cases added.

Co-authored-by: Sviatoslav Sydorenko <[email protected]>
@webknjaz
Copy link
Member

webknjaz commented Sep 7, 2021

FTR somebody needs to evaluate this as a possible fix for #2776.

@jaraco
Copy link
Member

jaraco commented Sep 8, 2021

should 58.0.2 be yanked? I assume this will go into a 58.0.3

What value would there be in yanking 58.0.2?

@jaraco
Copy link
Member

jaraco commented Sep 8, 2021

In general, I think it's a move in the right direction but could you also add a regression test for this scenario? I'd like to see both positive and negative test cases added.

I'm happy to skip regression tests for these tweaks.

@jaraco jaraco merged commit 3b549e5 into pypa:main Sep 8, 2021
@jimmyhli
Copy link

jimmyhli commented Sep 8, 2021

should 58.0.2 be yanked? I assume this will go into a 58.0.3

What value would there be in yanking 58.0.2?

No, there wouldn't be, if a 58.0.3 is released. I think some were suggesting that 58.0.2 be yanked so the latest just points to 58.0.1 :)

Thanks for the fix!

@potiuk
Copy link

potiuk commented Sep 8, 2021

We have some packages as dependencies, which have 2_to_3 set to true (specifically FlaskOpenId which has not been updated since 2016, so this is likely it won't be updated. https://github.com/mitsuhiko/flask-openid/ Any chance setuptools can simply ignore 2_to_3 setting, rather than ignore when false ?

UPDATE: see below - I withdraw the proposal. It was bad idea

@plumdog
Copy link
Contributor Author

plumdog commented Sep 9, 2021

@potiuk See #2775 (comment) and #2769 for rationale behind not ignoring the flag - the installed package almost certainly won't be importable.

@potiuk
Copy link

potiuk commented Sep 9, 2021

Yeah. Absolutely. Actually we had an interesting case because the Flask-openid actually was working with just removing 2_to_3 - because it was really small, the changes were harmless (like removing u` from unicod strings) and the only non-compatible python 3 code was under "if python < 3" anyway. Thankfully we got response from the maintainers and we became maintainers ourselves and we released a new version of package (after manually converting it with 2_to_3 and making it python3 - only). We also released a .whl package which remove the need of using setuptools at all.

I agree it's not possible and tha approach you come up with with "false" is good.

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

Successfully merging this pull request may close these issues.

5 participants