Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-119049: Fix incorrect usage ofGET_WARNINGS_ATTR#119063

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.

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

Already on GitHub?Sign in to your account

Merged
serhiy-storchaka merged 8 commits intopython:mainfromEclips4:issue-119049
May 16, 2024

Conversation

Eclips4
Copy link
Member

@Eclips4Eclips4 commentedMay 15, 2024•
edited
Loading

GET_WARNINGS_ATTR is trying to get theATTR attribute of the Pythonwarnings module. If the third parameter is true, it tries to import thewarnings module if it hasn't been imported already.
All of the calls tocall_show_warning are made with thesource = NULL argument, so this statementshow_fn = GET_WARNINGS_ATTR(interp, _showwarnmsg, source != NULL); always becomesshow_fn = GET_WARNINGS_ATTR(interp, _showwarnmsg, 0);.
So it never triess to import thewarnings module if it has not already imported, which leads us to the issue described in the#119049.
This is whytest_io.test_check_encoding_warning works whenimport warnings is used inpathlib._local.py and fails when we remove this statement.

barneygale reacted with heart emoji
Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

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

On other hand, what happens when it is called at the late stage of Python shutdown, when import does not work? Does it emit any warning or raise an exception?

@Eclips4
Copy link
MemberAuthor

Eclips4 commentedMay 15, 2024•
edited
Loading

On other hand, what happens when it is called at the late stage of Python shutdown, when import does not work? Does it emit any warning or raise an exception?

No, it's just omit the source in the warning. Should we emit warning or raise a exception?

@serhiy-storchaka
Copy link
Member

How difficult is to write a test for this case?

@Eclips4
Copy link
MemberAuthor

How difficult is to write a test for this case?

It's not difficult.

Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Thank you for the new test. Even if its result was not affected by this change, it is nice to check this.

Eclips4and others added3 commitsMay 16, 2024 22:18
Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM.

@serhiy-storchakaserhiy-storchaka added needs backport to 3.12only security fixes needs backport to 3.13bugs and security fixes and removed skip news labelsMay 16, 2024
@serhiy-storchakaserhiy-storchakaenabled auto-merge (squash)May 16, 2024 20:05
@serhiy-storchaka
Copy link
Member

A NEWS entry is needed since this is a user visible change.

Eclips4 reacted with thumbs up emoji

@Eclips4
Copy link
MemberAuthor

Thanks for the review, Serhiy.

@serhiy-storchakaserhiy-storchaka merged commit100c7ab intopython:mainMay 16, 2024
@miss-islington-app
Copy link

Thanks@Eclips4 for the PR, and@serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestMay 16, 2024
…d by C API (pythonGH-119063)The source line was not displayed if the warnings module had not yetbeen imported.(cherry picked from commit100c7ab)Co-authored-by: Kirill Podoprigora <kirill.bast9@mail.ru>
@miss-islington-app
Copy link

Sorry,@Eclips4 and@serhiy-storchaka, I could not cleanly backport this to3.12 due to a conflict.
Please backport usingcherry_picker on command line.

cherry_picker 100c7ab00ab66a8c0d54582f35e38d8eb691743c 3.12

@bedevere-app
Copy link

GH-119106 is a backport of this pull request to the3.13 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.13bugs and security fixes labelMay 16, 2024
@Eclips4
Copy link
MemberAuthor

Eclips4 commentedMay 17, 2024•
edited
Loading

@serhiy-storchaka I'm trying to do backport to the 3.12 branch manually, and realize that this cannot be done without the backport ofe1d8c65.
In short: to get the sources we need a new version oflinecache which has_register_code function. This function is used inpythonrun.c which saves our code to thelinecache.cache.

@serhiy-storchaka
Copy link
Member

Is it because of passing the code via the-c option? Then I guess we can just write it into a file.

@bedevere-app
Copy link

GH-119119 is a backport of this pull request to the3.12 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.12only security fixes labelMay 17, 2024
@Eclips4
Copy link
MemberAuthor

Is it because of passing the code via the-c option? Then I guess we can just write it into a file.

Yes, you're right! Thanks for the hint. Check please the#119119.

@Eclips4Eclips4 deleted the issue-119049 branchMay 17, 2024 11:53
serhiy-storchaka pushed a commit that referenced this pull requestMay 17, 2024
…ed by C API (GH-119063) (GH-119106)The source line was not displayed if the warnings module had not yetbeen imported.(cherry picked from commit100c7ab)Co-authored-by: Kirill Podoprigora <kirill.bast9@mail.ru>
@serhiy-storchakaserhiy-storchaka removed their assignmentMay 17, 2024
estyxx pushed a commit to estyxx/cpython that referenced this pull requestJul 17, 2024
…d by C API (pythonGH-119063)The source line was not displayed if the warnings module had not yetbeen imported.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@serhiy-storchakaserhiy-storchakaserhiy-storchaka approved these changes

@vstinnervstinnerAwaiting requested review from vstinner

@erlend-aaslanderlend-aaslandAwaiting requested review from erlend-aasland

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@Eclips4@serhiy-storchaka
[8]ページ先頭
©2009-2025 Movatter.jp