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.

Sign up for GitHub

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

Jump to bottom

#219 Use sysconfig.get_config_h_filename() to locate pyconfig.h #220

Merged
merged 3 commits into from
Feb 12, 2024
Merged

#219 Use sysconfig.get_config_h_filename() to locate pyconfig.h #220

merged 3 commits into from
Feb 12, 2024

Conversation

zooba
Copy link
Contributor

@zooba zooba commented Dec 12, 2023 •
edited
Loading

I've used the minimal change here. It would be totally valid to always make this call and only add the path if it differs from {prefix}\include, but it still only affects the case where we're in a CPython build directory so I don't see any reason to touch code outside of the checks for that.

Fixes #219

@zooba zooba changed the title Fixes #219 Use sysconfig.get_config_h_filename() to locate pyconfig.h #219 Use sysconfig.get_config_h_filename() to locate pyconfig.h Dec 12, 2023
@zooba zooba mentioned this pull request Dec 12, 2023
Merged
@zooba
Copy link
Contributor Author

zooba commented Dec 12, 2023

Those failures seem to be due to setuptools not being installed by default in 3.12. I'd suggest that may indicate that earlier versions are testing the wrong distutils/setuptools versions, or at least missing the opportunity to test with the current code.

The tests impacted by this change passed. It's unfortunate that it aborts the parallel jobs so we can't see their results.

@lazka
Copy link
Contributor

lazka commented Dec 17, 2023

lgtm, but there is one more place where the old path is used:

distutils/distutils/sysconfig.py

Lines 362 to 371 in 35069ad

def get_config_h_filename():
"""Return full pathname of installed pyconfig.h file."""
if python_build:
if os.name == "nt":
inc_dir = os.path.join(_sys_home or project_base, "PC")
else:
inc_dir = _sys_home or project_base
return os.path.join(inc_dir, 'pyconfig.h')
else:
return sysconfig.get_config_h_filename()

@lazka
Copy link
Contributor

lazka commented Dec 17, 2023

The tests impacted by this change passed. It's unfortunate that it aborts the parallel jobs so we can't see their results.

I've created #222 for this

@zooba
Copy link
Contributor Author

zooba commented Dec 18, 2023

there is one more place where the old path is used

Is there any reason why this shouldn't just call sysconfig all the time? (Or just import the function from sysconfig instead of redefining it?)

@lazka
Copy link
Contributor

lazka commented Dec 18, 2023

Not to my knowledge. I skipped the build-python part in 676e669 back then because I wasn't sure if the logic was equal after things diverged with CPython and I didn't want to risk it.

@lazka
Copy link
Contributor

lazka commented Dec 18, 2023

lgtm

@zooba
Copy link
Contributor Author

zooba commented Jan 12, 2024

@jaraco Are you still merging stuff for this project? You'll want this change before 3.13 ships :)

@zooba
Copy link
Contributor Author

zooba commented Feb 12, 2024

Ping on this issue.

@jaraco jaraco merged commit 6da7ebb into pypa:main Feb 12, 2024
18 checks passed
jaraco added a commit that referenced this pull request Feb 12, 2024
@jaraco
Merge pull request #220 from zooba/issue-219
8ae78da
@zooba zooba deleted the issue-219 branch February 12, 2024 22:15
@zooba
Copy link
Contributor Author

zooba commented Feb 12, 2024

Thanks Jason!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FFY00 FFY00 FFY00 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

_get_python_inc_nt should use sysconfig.get_config_h_filename()
4 participants
@zooba @lazka @FFY00 @jaraco