-
Notifications
You must be signed in to change notification settings - Fork 0
Sysconfig merge #2
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
Conversation
0538371 to
da88e63
Compare
|
Well, this seems to be almost complete and I'd like to merge it before I get to other parts of distutils we need to merge to sysconfig. In points:
My plan was to not touch tests at all to prove that the change of the deprecated module has no impact on its functionality but this plan failed. The main reason is that sysconfig uses Another change in a test (test_venv) is related to a fact that pip uses What I think is left to be done:
|
|
The description is awesome. Plese let the PEP discussion know that you are working on this (even if you are not yet ready to share the code). |
zooba
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for working on this!
Lib/distutils/sysconfig.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strictly, sys.platform == "win32" is the correct check for this case (sys.platform is "how it was built", while os.name is "which POSIX emulation are we using" - they just happen to line up right now ;) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The functions _fix_pcbuild is defined in sysconfig only if os.name == 'nt' so I can either keep the condition as is or change it in both modules. What do you think would be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Long term I'd rather it was changed in sysconfig. I don't mind what happens in distutils.sysconfig.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
os.name == "nt" is used in sysconfig in many places so I'd rather keep it as TODO.
Lib/sysconfig.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s possible that re was imported in this function rather than at module scope because building CPython (and the _sre modules) needs sysconfig.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll add try/except to handle situation when re module won't be available. Or do you have a better idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That may not work if the problem is circular imports. Better to just not use re until it's needed (and there's probably nothing gained from re.compile here anyway; the regular re methods will cache well enough).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would mean import re and define some of the regexes in three places:
- expand_makefile_vars function in sysconfig
- _parse_makefile function in sysconfig
- globally in distutils.sysconfig because they're imported from there in read_setup_file function in distutils.extension (not future proof)
I'd rather keep them in one place. I've checked and there seems to be no circular import. Do you think there might be a problem which the fresh builds in CI cannot spot?
|
FTR I found some tickets about differences between the two modules: |
d9f9d92 to
182712e
Compare
Thank you. Both seem to be better fixed in |
|
Doc status:
|
|
My plan is to get back to this now and open an upstream PR once I solve the problem on macOS. |
…ONFIG_VARS might be modified
This reverts commit fef432d1b75d98bb68ec98dd92d3de1aacca437c.
…edtogether with the module.
74ff716 to
6d29b50
Compare
6d29b50 to
a2d8e4d
Compare
|
Upstream PR: python#23142 |
No description provided.