gh-111650: Generate pyconfig.h on Windows #112179
Conversation
colesbury
added
type-feature
colesbury
force-pushed
the
generate-pyconfig-win
branch
2 times, most recently
from
e4c3520
to
912e169
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
!buildbot nogil |
|
🤖 New build scheduled with the buildbot fleet by @colesbury for commit b2920c4 🤖 The command will test the builders whose names match following regular expression: The builders matched are:
|
Prior to this change, the Py_NOGIL macro was not defined when building C API extensions with the `--disable-gil` build on Windows. `Py_NOGIL` was only defined as a pre-processor definition in pyproject.props, but that is not used by C-API extensions. This instead generates the `pyconfig.h` header on Windows as part of the build process. For now, `Py_NOGIL` is the only macro that may be conditionally defined in the generated file.
colesbury
force-pushed
the
generate-pyconfig-win
branch
from
b2920c4
to
25c6028
Compare
| @@ -94,6 +96,11 @@ | |||
| <Projects2 Include="venvlauncher.vcxproj;venvwlauncher.vcxproj" /> | |||
| </ItemGroup> | |||
|
|
|||
| <Target Name="PreBuild" BeforeTargets="Build"> | |||
| <!-- Stick a _PLACEHOLDER=1 after $(PyConfigArgs) to handle both trailing commas and empty $(PyConfigArgs) --> | |||
| <Exec Command="powershell.exe $(PySourcePath)PCbuild\generate_pyconfig.ps1 -File $(PySourcePath)PC\pyconfig.h -define $(PyConfigArgs)_PLACEHOLDER=1" /> | |||
There was a problem hiding this comment.
Would rather have this happen in pythoncore.vcxproj if possible, or _freeze_module.vcxproj if necessary (or both). That way we don't have to rely on building through this file to make sure it happens.
It probably also belongs in the regen.targets file, and can then be referenced from the others.
| pyconfig_h = os.path.join(srcdir, 'PC', 'pyconfig.h') | ||
| self.assertTrue(os.path.exists(pyconfig_h), pyconfig_h) | ||
| if os.name == 'nt': | ||
| # <srcdir>/PC/pyconfig.h only exists on Windows. |
There was a problem hiding this comment.
Honestly, it's probably a better long-term move to get rid of PC/pyconfig.h completely and generate it in Include where the POSIX one would be.
Eventually (though I suspect it's "likely never") we'll get a combined build system. At that point, it'll be much more sensible to have a coherent set of include files.
The code that tries to include header files in installers already assumes it can pick it up directly from a repository checkout (Tools/msi/dev and PC/layout, IIRC). That will need additional logic to pick it up from somewhere else - I'd suggest the build directory $(Py_OutDir) along with every other generated file that we include in the distribution.
| foreach ($i in 0..($lines.Length - 1)) { | ||
| if ($lines[$i] -match "^#undef (\w+)$") { | ||
| $key = $Matches[1] | ||
| if ($definedValues.ContainsKey($key)) { | ||
| $value = $definedValues[$key] | ||
| $lines[$i] = "#define $key $value".TrimEnd() | ||
| } else { | ||
| $lines[$i] = "/* #undef $key */" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
| foreach ($i in 0..($lines.Length - 1)) { | |
| if ($lines[$i] -match "^#undef (\w+)$") { | |
| $key = $Matches[1] | |
| if ($definedValues.ContainsKey($key)) { | |
| $value = $definedValues[$key] | |
| $lines[$i] = "#define $key $value".TrimEnd() | |
| } else { | |
| $lines[$i] = "/* #undef $key */" | |
| } | |
| } | |
| } | |
| $lines = $lines | %{ | |
| if ($_ -match "^#undef (\w+)$") { | |
| $key = $Matches[1]; | |
| if ($definedValues.ContainsKey($key)) { | |
| "#define $key ${definedValues[$key]}".TrimEnd() | |
| } else { | |
| "/* #undef $key */" | |
| } | |
| } else { | |
| $_ | |
| } | |
| } |
There was a problem hiding this comment.
Okay, that was a bit of a self-indulgent rewrite that didn't actually make it shorter, simpler, or probably faster 😆
But if you want, we already assume that a copy of Python is available for bootstrapping on Windows (and I believe we require 3.10), so you could make this a Python script. IIRC, $(HostPython) has the location - it should be used elsewhere in regen.targets if you need examples.
| @@ -14,6 +15,7 @@ | |||
| <IncludeSSL Condition="'$(IncludeSSL)' == ''">true</IncludeSSL> | |||
| <IncludeTkinter Condition="'$(IncludeTkinter)' == ''">true</IncludeTkinter> | |||
| <IncludeUwp Condition="'$(IncludeUwp)' == ''">false</IncludeUwp> | |||
| <PyConfigArgs Condition="'$(DisableGil)' == 'true'">Py_GIL_DISABLED=1,$(PyConfigArgs)</PyConfigArgs> | |||
There was a problem hiding this comment.
Semicolon separated is the usual for MSBuild files. I know I'll think this is wrong every time I see it - can we maybe $(PyConfigArgs.Replace(`;`, `,`)) later if it's really needed?
Prior to this change, the
Py_NOGILmacro was not defined when building C API extensions with the--disable-gilbuild on Windows.Py_NOGILwas only defined as a pre-processor definition in pyproject.props, but that is not used by C-API extensions.This instead generates the
pyconfig.hheader on Windows as part of the build process. For now,Py_NOGILis the only macro that may be conditionally defined in the generated file.