GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-111650: Generate pyconfig.h on Windows by colesbury · Pull Request #112179 · python/cpython Conversation
|
|
!buildbot nogil |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
🤖 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:
|
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
b2920c4 to
25c6028
Compare
|
|
||
| <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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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 { | |
| $_ | |
| } | |
| } |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
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.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| <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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
3 participants
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.