Skip to content

gh-143429: Use compile-time NaN encoding detection for test_struct #143432

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
vstinner merged 6 commits into from
Jan 9, 2026
Merged

gh-143429: Use compile-time NaN encoding detection for test_struct #143432

vstinner merged 6 commits into from
Jan 9, 2026
+10 −7

Conversation

@chenx97
Copy link
Contributor

@chenx97 chenx97 commented Jan 5, 2026 •
edited
Loading

Description

This PR uses C macros to detect NaN encoding schemes for Lib/test/test_struct.py, instead of detecting the host machine.

math.nan depends on toolchain settings for a MIPS build, as setting -mnan=legacy and -mnan=2008 result in different __builtin_nan behavior for GCC.

Tests

  • Ran the official test suite: ./python -m test test_struct (Passed).

@python-cla-bot

This comment was marked as resolved.

Sign in to view
@bedevere-app bedevere-app bot added awaiting review tests Tests in the Lib/test dir labels Jan 5, 2026
@bedevere-app bedevere-app bot mentioned this pull request Jan 5, 2026
Closed
@bedevere-app

This comment was marked as resolved.

Sign in to view
@chenx97 chenx97 force-pushed the mips-skip-nan-checks branch from 8ff4af9 to 0bbf71e Compare January 5, 2026 09:49
skirpichev
skirpichev reviewed Jan 5, 2026
View reviewed changes
skirpichev
skirpichev previously approved these changes Jan 5, 2026
View reviewed changes
Copy link
Contributor

@skirpichev skirpichev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In principle, this looks good for me. But I would prefer a different solution, see issue thread.

CC @vstinner

skirpichev
skirpichev reviewed Jan 5, 2026
View reviewed changes
@skirpichev skirpichev added the needs backport to 3.14 bugs and security fixes label Jan 5, 2026
@chenx97 chenx97 changed the title gh-143429: Skip NaN encoding assertions for MIPS gh-143429: Use compile-time NaN encoding detection for test_struct Jan 7, 2026
@chenx97 chenx97 requested a review from skirpichev January 7, 2026 12:34
vstinner
vstinner reviewed Jan 7, 2026
View reviewed changes
vstinner
vstinner approved these changes Jan 7, 2026
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I just suggest moving the C change to Modules/_testcapi/float.c.

picnixz
picnixz reviewed Jan 7, 2026
View reviewed changes
picnixz
picnixz requested changes Jan 7, 2026
View reviewed changes
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to explicitly request changes.

@bedevere-app
Copy link

bedevere-app bot commented Jan 7, 2026

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

skirpichev
skirpichev reviewed Jan 7, 2026
View reviewed changes
@skirpichev
Copy link
Contributor

skirpichev commented Jan 8, 2026

@chenx97, could you check also test_pack_unpack_roundtrip_for_nans() in Lib/test/test_capi/test_float.py?

Is it enabled on your system? If so, it should fail too. If not, I suspect you could increase number of samples (10 for now) in this test.

chenx97 added a commit to AOSC-Tracking/cpython that referenced this pull request Jan 8, 2026
@chenx97
FROMLIST python#143432: Skip NaN encoding assertions for MIPS
16f7fad
@chenx97
Copy link
Contributor Author

chenx97 commented Jan 8, 2026

@chenx97, could you check also test_pack_unpack_roundtrip_for_nans() in Lib/test/test_capi/test_float.py?

Is it enabled on your system? If so, it should fail too. If not, I suspect you could increase number of samples (10 for now) in this test.

I think the tests are expected to pass on mips64, as the parisc handling logic only runs for the 32-bit environment. Legacy mips32 fails here, as you guessed.

@skirpichev
Copy link
Contributor

skirpichev commented Jan 8, 2026

the parisc handling logic only runs for the 32-bit environment

Ah, you are right. (I just did a quick grepping and didn't look on the function code.)

Still, I think we could use new module variable also here, instead of the platform check.

@chenx97
Move nan flag to Modules/_testcapi/float.c and use bool
376c04d 
... and migrate another parisc detection to the new nan flag
vstinner
vstinner approved these changes Jan 8, 2026
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. It's better with _testcapi.nan_msb_is_signaling.

@vstinner vstinner enabled auto-merge (squash) January 9, 2026 10:44
@vstinner vstinner merged commit dcdb23f into python:main Jan 9, 2026
82 of 85 checks passed
@miss-islington-app
Copy link

miss-islington-app bot commented Jan 9, 2026

Thanks @chenx97 for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 9, 2026
@chenx97 @miss-islington
pythongh-143429: Use compile-time NaN encoding detection for test_str…
2433baa 
…uct (pythonGH-143432)

(cherry picked from commit dcdb23f)

Co-authored-by: Henry Chen <chenx97@aosc.io>
@bedevere-app
Copy link

bedevere-app bot commented Jan 9, 2026

GH-143595 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label Jan 9, 2026
@vstinner
Copy link
Member

vstinner commented Jan 9, 2026

Mergerd, thank you for your fix. It's good that the fix is no longer to skip the tests :-)

vstinner pushed a commit that referenced this pull request Jan 9, 2026
@miss-islington @chenx97
[3.14] gh-143429: Use compile-time NaN encoding detection for test_st…
a299c1b 
…ruct (GH-143432) (#143595)

gh-143429: Use compile-time NaN encoding detection for test_struct (GH-143432)
(cherry picked from commit dcdb23f)

Co-authored-by: Henry Chen <chenx97@aosc.io>
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

@skirpichev skirpichev skirpichev approved these changes

@picnixz picnixz picnixz approved these changes

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.

4 participants

@chenx97 @skirpichev @vstinner @picnixz