Repository navigation
gh-154200: Use sysconfig.get_platform() in test_importlib.test_windows - #154210
chris-eibl merged 2 commits into
Conversation
Sorry, something went wrong.
…windows
test_tagged_suffix computed its expected extension suffix with a vendored port of
distutils.util.get_platform() that sniffs sys.version for 'amd64'. Python/getversion.c
truncates the compiler segment of sys.version at 80 characters, so on clang-cl builds --
whose banner carries a long __clang_version__ -- the marker is cut off, the helper falls
through to sys.platform ('win32'), and the test expects '.cp316t-win32.pyd' while the
interpreter correctly reports '.cp316t-win_amd64.pyd'.
sysconfig.get_platform() no longer parses sys.version on Windows (pythongh-145410); it uses the
_sysconfig C accelerator, which derives the platform from compile-time architecture
macros and is compiler-agnostic. Use it and drop the vendored copy, its only caller.
This also removes the VSCMD_ARG_TGT_ARCH branch, which trusted the ambient Visual Studio
target architecture over the interpreter actually running the test.
There was a problem hiding this comment.
Pull request overview
This PR fixes a Windows-only test failure in test_importlib.test_windows by replacing a local, sys.version-parsing platform detector with sysconfig.get_platform(), which is compiler-agnostic on Windows and avoids truncation-related mis-detection on clang-cl builds.
Changes:
- Remove the vendored get_platform() helper that relied on VSCMD_ARG_TGT_ARCH and sys.version parsing.
- Compute the expected .pyd platform tag using sysconfig.get_platform() and keep normalization via re.sub(..., '_', ...) for win-amd64 → win_amd64.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
|
🤖 New build scheduled with the buildbot fleet by @chris-eibl for commit d63344e 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F154210%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
Sorry, something went wrong.
|
Since version 22, clang-cl returns a much longer __clang_version__. I think it would make sense to switch to #if defined(__clang__)
#define COMPILER "[Clang " __clang_major__ "." __clang_minor__ "." __clang_patchlevel__ "]"
like suggested in #145410 (comment). This would also fix #154199. Nevertheless, this change here lgtm, too. Fwiw, during EP2026 sprints @zware setup a clang-cl buildbot. Cc @encukou . |
Sorry, something went wrong.
|
Sorry, something went wrong.
|
All commit authors signed the Contributor License Agreement. |
Sorry, something went wrong.
|
I do not know why the CLA step got stuck. I've clicked "update branch" in the hope it works now ... |
Sorry, something went wrong.
|
Yupp, that did it :) @SynaptSea can you please sign the CLA? |
Sorry, something went wrong.
I trust your judgement here. My main concern is that supported platforms don't break :)
Great news! Please keep using OS-unsupported tag on issues/PRs though: that'll make them (eventually) get listed in the project. If/when clang-cl gets a PEP-11 tier, we can add a clang-cl tag and put it on the older issues. |
Sorry, something went wrong.
m2, hence I've run the build bots in addition to CI to play safe: all 18 failures are unrelated (mostly test.test_curses.TestAscii.test_complexchar). So once CLA is signed I'd be inclined to approve.
cc @zooba
👍 |
Sorry, something went wrong.
|
The change in this PR is perfect, happy to see it. No real opinion on what to do about sys.version for Clang-cl, other than make sure it doesn't break tests like this one. |
Sorry, something went wrong.
|
@SynaptSea can you sign the CLA so that we can proceed here? Otherwise can you / we close this PR? |
Sorry, something went wrong.
Sorry for the delay. Signed |
Sorry, something went wrong.
Me2, CLA is signed, so merging. @SynaptSea Thanks for your contribution ❤️ |
Sorry, something went wrong.
…windows (python#154210) instead of a port of distutils.util.get_platform()
Fixes #154200
test_tagged_suffix computes its expected extension suffix with a vendored
"Port of distutils.util.get_platform()" that sniffs sys.version.lower() for 'amd64'.
Python/getversion.c truncates the compiler segment of sys.version at 80 characters
("%.80s"). On clang-cl builds the banner is
"[Clang " __clang_version__ "] 64 bit (AMD64) with MSC v.<n> CRT]", and official LLVM
Windows binaries have an 86-character __clang_version__, so the (AMD64) marker is always
cut off. The helper falls through to return sys.platform -> 'win32', and the test expects
.cp316t-win32.pyd while the interpreter correctly reports .cp316t-win_amd64.pyd:
sysconfig.get_platform() no longer has this problem: since gh-145410 it does not parse
sys.version on Windows but delegates to the _sysconfig C accelerator, which derives the
platform from compile-time architecture macros and is compiler-agnostic. So this uses it and
deletes the vendored copy, which that call site was the only consumer of.
This is not circular: SYSCONFIG_PLATFORM in Modules/_sysconfig.c and PYD_PLATFORM_TAG in
PC/pyconfig.h are independent definitions in separate files, so the assertion still
cross-checks two separate derivations.
It also drops the VSCMD_ARG_TGT_ARCH branch, which trusted the ambient Visual Studio
developer-prompt target architecture over the interpreter actually executing the test.
That was backwards — EXTENSION_SUFFIXES is compiled into the running binary — and it is
why the failure only reproduced outside a VS developer prompt.
Testing
On a local clang-cl free-threaded build, python -m unittest test.test_importlib.test_windows:
Also OK with VSCMD_ARG_TGT_ARCH=x64, and with VSCMD_ARG_TGT_ARCH=x86 deliberately
mismatched against this amd64 interpreter — the old code would have computed 'win32' and
failed there. Full python -m test test_importlib: run=1229 skipped=23, SUCCESS.
Note the re.sub() is still required: sysconfig.get_platform() returns win-amd64 with a
hyphen while PYD_PLATFORM_TAG uses win_amd64. os and re both remain in use elsewhere
in the module, so no imports become unused.
NEWS
This is a test-only change, which the devguide lists as not requiring a Misc/NEWS.d entry,
so I have not added a blurb — could a maintainer apply skip news? Happy to add one instead
if you would prefer, on the grounds that it unblocks a whole build configuration.