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

Replace distutils usage with sysconfig #396

Merged
merged 2 commits into from
Apr 23, 2021

Conversation

uranusjr
Copy link
Member

@uranusjr uranusjr commented Feb 5, 2021

See #349 for discussion on this.

Copy link
Member

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

Approving for the code, but still waiting for us to reach agreement as to whether this is what we want.

@brettcannon
Copy link
Member

So I'm +1 on taking this PR at this point.

@puetzk
Copy link

puetzk commented Apr 20, 2021

Nudge: PEP 632 - Deprecate distutils module is accepted for Python 3.10, so doesn't this need to get landed (ideally before beta 1 in 2 weeks)? We seemed to have consensus for this solution per the discussion in #349...

There are merge conflicts vs the type annotation changes in #378 (nothing interesting, just changes on adjacent lines). I don't think I can push to @uranusjr's branch since I'm neither him nor a maintainer for this repo. But I have a rebase (fixing the conflicts) up at main...puetzk:distutils-to-sysconfig if @uranusjr wants to pull them in.

@brettcannon
Copy link
Member

There isn't a rush since this only come into play if there's a planned release which we don't have one explicitly (although maybe pip has one coming up?).

@uranusjr
Copy link
Member Author

pip has one coming up this month, but pip can always just vendor distutils and patch packaging so there’s no rush for the fix there either. I think this only needs to go in before major redistributors (Linux distributions) start shipping Python 3.10 by default, so there’s still time, but definitely sooner is better than later.

@pradyunsg
Copy link
Member

I'm happy for this to be rebased and merged, for the next packaging release.

@brettcannon
Copy link
Member

OK, then @uranusjr , when you are able to resolve the conflicts we can merge this.

@brettcannon brettcannon merged commit c800826 into pypa:main Apr 23, 2021
@brettcannon
Copy link
Member

Thanks!

@puetzk
Copy link

puetzk commented Apr 30, 2021

pip has one coming up this month, but pip can always just vendor distutils and patch packaging so there’s no rush for the fix there either.

See discussion at pypa/pip#9761 (comment): pip is only going to take it for 21.1.1 if packaging makes a release; they're not going to patch it in. Would be nice to hit 3.8.10 since it was a 3.7->3.8 regression and that's the last bugfix patch to for 3.8, but it has admittedly not become a crisis just because of that opportunity.

@pradyunsg
Copy link
Member

Since it's not worked for the entire life of 3.8, and seemingly no one bothered anyone about it, I don't feel to inclined to hurry up for that.

@puetzk
Copy link

puetzk commented Apr 30, 2021

Fair. Just wanted to link the discussion.

@puetzk
Copy link

puetzk commented Apr 30, 2021

Someone did bother: Ryan filed BPO38989 for the regression after 3.8.0 but before 3.8.1, and I figured out the root cause shortly after, then @JesseMaurais filed pypa/pip#8649 and figured out it was the same thing, and that turned into #327 here. But not all that many someones chimed in "me toos" on any of these 3 issues: apparently using python in MSVC build scripts must be rarer than I'd have thought (or most windows folks just install their tools in the system python manually instead of trying to automatically set up a venv automatically).

But then it got all tangled up with PEP 632 and the decision not to remove distutils rather than fix the regression, so it was a long time figuring out what fix was going to land and where. Which has now happened (and thank you for that!).

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

Successfully merging this pull request may close these issues.

4 participants