gh-154200: Use sysconfig.get_platform() in test_importlib.test_windows#154210
gh-154200: Use sysconfig.get_platform() in test_importlib.test_windows#154210SynaptSea wants to merge 2 commits into
Conversation
…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 onVSCMD_ARG_TGT_ARCHandsys.versionparsing. - Compute the expected
.pydplatform tag usingsysconfig.get_platform()and keep normalization viare.sub(..., '_', ...)forwin-amd64→win_amd64.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
chris-eibl
added
the
🔨 test-with-buildbots
|
🤖 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. |
bedevere-bot
removed
the
🔨 test-with-buildbots
|
Since version 22, clang-cl returns a much longer #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 . |
|
|
The following commit authors need to sign the Contributor License Agreement: |
|
I do not know why the CLA step got stuck. I've clicked "update branch" in the hope it works now ... |
|
Yupp, that did it :) @SynaptSea can you please sign the CLA? |
Fixes #154200
test_tagged_suffixcomputes its expected extension suffix with a vendored"Port of
distutils.util.get_platform()" that sniffssys.version.lower()for'amd64'.Python/getversion.ctruncates the compiler segment ofsys.versionat 80 characters(
"%.80s"). On clang-cl builds the banner is"[Clang " __clang_version__ "] 64 bit (AMD64) with MSC v.<n> CRT]", and official LLVMWindows binaries have an 86-character
__clang_version__, so the(AMD64)marker is alwayscut off. The helper falls through to
return sys.platform->'win32', and the test expects.cp316t-win32.pydwhile the interpreter correctly reports.cp316t-win_amd64.pyd:sysconfig.get_platform()no longer has this problem: since gh-145410 it does not parsesys.versionon Windows but delegates to the_sysconfigC accelerator, which derives theplatform 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_PLATFORMinModules/_sysconfig.candPYD_PLATFORM_TAGinPC/pyconfig.hare independent definitions in separate files, so the assertion stillcross-checks two separate derivations.
It also drops the
VSCMD_ARG_TGT_ARCHbranch, which trusted the ambient Visual Studiodeveloper-prompt target architecture over the interpreter actually executing the test.
That was backwards —
EXTENSION_SUFFIXESis compiled into the running binary — and it iswhy 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:Ran 11 tests ... FAILED (failures=2)(theFrozen_andSource_variants)Ran 11 tests ... OKAlso OK with
VSCMD_ARG_TGT_ARCH=x64, and withVSCMD_ARG_TGT_ARCH=x86deliberatelymismatched against this amd64 interpreter — the old code would have computed
'win32'andfailed there. Full
python -m test test_importlib:run=1229 skipped=23, SUCCESS.Note the
re.sub()is still required:sysconfig.get_platform()returnswin-amd64with ahyphen while
PYD_PLATFORM_TAGuseswin_amd64.osandreboth remain in use elsewherein the module, so no imports become unused.
NEWS
This is a test-only change, which the devguide lists as not requiring a
Misc/NEWS.dentry,so I have not added a blurb — could a maintainer apply
skip news? Happy to add one insteadif you would prefer, on the grounds that it unblocks a whole build configuration.
get_platform()misdetects clang-cl builds aswin32, failingtest_tagged_suffix#154200