Skip to content

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

Merged
jaraco merged 4 commits into
pypa:mainfrom
plumdog:do-not-error-if-use-2to3-is-false
Sep 8, 2021
Merged

Do not error if use_2to3 is set to a false value#2777
jaraco merged 4 commits into
pypa:mainfrom
plumdog:do-not-error-if-use-2to3-is-false

Conversation

@plumdog

@plumdog plumdog commented Sep 7, 2021

Copy link
Copy Markdown
Contributor

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

jimmyhli commented Sep 7, 2021

Copy link
Copy Markdown

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.
Comment thread setuptools/dist.py Outdated
Comment thread changelog.d/2777.misc.rst

@webknjaz webknjaz left a comment

Copy link
Copy Markdown
Member

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 <wk.cvs.github@sydorenko.org.ua>
@webknjaz

webknjaz commented Sep 7, 2021

Copy link
Copy Markdown
Member

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

@jaraco

jaraco commented Sep 8, 2021

Copy link
Copy Markdown
Member

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

jaraco commented Sep 8, 2021

Copy link
Copy Markdown
Member

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

jimmyhli commented Sep 8, 2021

Copy link
Copy Markdown

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!

@plumdog plumdog deleted the do-not-error-if-use-2to3-is-false branch September 8, 2021 05:54
@potiuk

potiuk commented Sep 8, 2021

Copy link
Copy Markdown

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

plumdog commented Sep 9, 2021

Copy link
Copy Markdown
Contributor Author

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

@potiuk

potiuk commented Sep 9, 2021

Copy link
Copy Markdown

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