-
Notifications
You must be signed in to change notification settings - Fork 72
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
sysconfig: use get_makefile_filename() from stdlib sysconfig #92
Conversation
1091c82
to
83da08d
Compare
@0-wiz-0 can you confirm this does not break anything for NetBSD? |
I do see that pkgsrc already patches sysconfig for similar purposes, so I expect that deferring to sysconfig is the right thing to do. |
distutils/sysconfig.py
Outdated
# Allow this value to be patched by pkgsrc. Ref pypa/distutils#16. | ||
_makefile_tmpl = 'config-{python_ver}{build_flags}{multiarch}' | ||
|
||
|
||
def get_makefile_filename(): | ||
"""Return full pathname of installed Makefile from the Python build.""" | ||
if python_build: |
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.
I also notice that the sysconfig version has a _PYTHON_BUILD with similar logic. Can the entire function be delegated to 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.
I wanted to keep this low-impact. But yes, I agree that Python probably knows better where its files are when run uninstalled. I can make that change if wanted.
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.
What if I add a CI job running the test suite with an uninstalled CPython? (this would require building CPython, ensurepip, install pytest, run tests)
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.
I don’t feel strongly about my suggestion. Happy to take baby steps too.
Regarding CI, I’d be on board if slightly reluctant due to the slowness and maintenance burden. At some point, all tests should be migrated to Setuptools, so I dread slightly added complication for that migration.
That said, better to have something useful to migrate than no protection at all. If you’re willing to contribute it, I’d accept it.
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, thanks. I went with delegating everything to sysconfig now.
I tried running the tests uninstalled, but there is one (unrelated) error I don't understand yet, so I'll keep this in mind but wont pursue for now.
83da08d
to
5726e0c
Compare
Ah, thanks. I've updated the commit message accordingly, linking to the upstream patch, |
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.
I’m ready to merge this anytime.
Instead of guessing the filename just refer to the stdlib. This also removes the "_makefile_tmpl" hook added in pypa#16, but 1) It was never implemented by the distro requesting it from what I can see: https://github.com/NetBSD/pkgsrc/blob/586097714897b1b4d4a9/devel/py-setuptools/Makefile#L28 2) The stdlib version should return a proper result as it is patched by the distro requesting the change: https://github.com/NetBSD/pkgsrc/blob/6efa5763ec447864a7d4/lang/python38/patches/patch-Lib_sysconfig.py Also adds a small test checking that the file exists on Unix platforms
5726e0c
to
62ed952
Compare
thanks! |
Instead of guessing the filename just refer to the stdlib.
This also removes the "_makefile_tmpl" hook added in #16, but
https://github.com/NetBSD/pkgsrc/blob/586097714897b1b4d4a9/devel/py-setuptools/Makefile#L28
https://github.com/NetBSD/pkgsrc/blob/6efa5763ec447864a7d4/lang/python38/patches/patch-Lib_sysconfig.py
Also adds a small test checking that the file exists on Unix platforms