Skip to content

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

Merged
jaraco merged 3 commits intopypa:mainfrom
zooba:issue-219
Feb 12, 2024
Merged

#219 Use sysconfig.get_config_h_filename() to locate pyconfig.h#220
jaraco merged 3 commits intopypa:mainfrom
zooba:issue-219

Conversation

@zooba
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

lazka commented Dec 18, 2023

lgtm

@zooba
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

zooba commented Feb 12, 2024

Ping on this issue.

@jaraco jaraco merged commit 6da7ebb into pypa:main Feb 12, 2024
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
Copy Markdown
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