GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-110119: Fix test_importlib `--disable-gil` Windows test failures by colesbury · Pull Request #110422 · python/cpython

Conversation

@colesbury

@colesbury colesbury commented Oct 5, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Use "t" in the expected tag for --disable-gil builds in test_tagged_suffix.

  • Issue: --disable-gil test failures due to ABI flag #110119

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@colesbury
pythongh-110119: Fix test_importlib --disable-gil Windows test fail…
2ebec1f 
…ures

Use "t" in the expected tag for `--disable-gil` builds in test_tagged_suffix.
@colesbury
colesbury requested review from brettcannon, ericsnowcurrently, ncoghlan and warsaw as code owners October 5, 2023 18:53
@bedevere-app bedevere-app Bot added tests Tests in the Lib/test dir awaiting review labels Oct 5, 2023
@bedevere-app bedevere-app Bot mentioned this pull request Oct 5, 2023
--disable-gil test failures due to ABI flag #110119
Closed
@colesbury colesbury added the skip news label Oct 5, 2023
@colesbury

colesbury commented Oct 5, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

cc @itamaro - this fixes one of the two Windows --disable-gil buildbot failures. I'm still trying to reproduce the test_peg_generator failures.

(EDIT: s/test_importlib/test_peg_generator)
itamaro reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@colesbury
colesbury requested a review from vstinner October 5, 2023 18:54
vstinner
vstinner reviewed Oct 5, 2023
View reviewed changes
Comment thread Lib/test/test_importlib/test_windows.py Outdated
Comment on lines 115 to 118
threading_tag = "t" if sysconfig.get_config_var("Py_NOGIL") else ""
expected_tag = ".cp{0.major}{0.minor}{1}-{2}.pyd".format(sys.version_info,
threading_tag,
re.sub('[^a-zA-Z0-9]', '_', get_platform()))

@vstinner vstinner Oct 5, 2023

Copy link
Copy Markdown
Member

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.

This code looks too complicated for what it is. I suggest:

        abi_flags = "t" if sysconfig.get_config_var("Py_NOGIL") else ""
        ver = sys.version_info
        platform = re.sub('[^a-zA-Z0-9]', '_', get_platform())
        expected_tag = f".cp{ver.major}{ver.minor}{abi_flags}-{platform}.pyd"

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
colesbury reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@colesbury
Use suggestion from vstinner
c6b791b
@colesbury
colesbury requested a review from vstinner October 5, 2023 19:20
vstinner
vstinner approved these changes Oct 5, 2023
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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.

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_importlib/test_windows.py
suffixes = self.machinery.EXTENSION_SUFFIXES
expected_tag = ".cp{0.major}{0.minor}-{1}.pyd".format(sys.version_info,
re.sub('[^a-zA-Z0-9]', '_', get_platform()))
abi_flags = "t" if sysconfig.get_config_var("Py_NOGIL") else ""

@vstinner vstinner Oct 5, 2023

Copy link
Copy Markdown
Member

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.

Now I'm confused. On Windows, sys.abiflags doesn't exist? It doesn't look practical that sys.abiflags doesn't exist to look for PYD files :-(

Maybe we should now add this flag?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@colesbury colesbury Oct 5, 2023

Copy link
Copy Markdown
Contributor Author

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.

The awkward bit is that debug libraries are indicated as a _d prefix like, foo_d.cp313t-win_amd64.pyd instead of as part of the abiflags. Still might be useful.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Oct 5, 2023

Copy link
Copy Markdown
Member

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.

We can maybe change that in Python 3.13 to make it consistent.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting core review and removed awaiting review labels Oct 5, 2023
@vstinner
vstinner enabled auto-merge (squash) October 5, 2023 19:43
@vstinner
vstinner merged commit 3c2f321 into python:main Oct 5, 2023
@bedevere-app bedevere-app Bot removed the awaiting core review label Oct 5, 2023
@colesbury
colesbury deleted the disable_gil_win branch October 6, 2023 15:40
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@colesbury @Glyphack
pythongh-110119: Fix test_importlib --disable-gil Windows test fail…
d2501d2 
…ures (python#110422)

Use "t" in the expected tag for `--disable-gil` builds in test_tagged_suffix.
@SynaptSea SynaptSea mentioned this pull request Jul 19, 2026
test_importlib.test_windows: vendored get_platform() misdetects clang-cl builds as win32, failing test_tagged_suffix #154200
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes
@brettcannon brettcannon Awaiting requested review from brettcannon brettcannon is a code owner
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner
@ncoghlan ncoghlan Awaiting requested review from ncoghlan ncoghlan is a code owner
@warsaw warsaw Awaiting requested review from warsaw warsaw is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news tests Tests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@colesbury @vstinner
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.