FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-119049: Fix incorrect usage of ``GET_WARNINGS_ATTR`` by Eclips4 · Pull Request #119063 · python/cpython · GitHub

/ cpython Public

gh-119049: Fix incorrect usage of GET_WARNINGS_ATTR - #119063

Merged
serhiy-storchaka merged 8 commits into
python:mainfrom
Eclips4:issue-119049
May 16, 2024
ConversationCommits (8)ChecksFiles changed
Merged

gh-119049: Fix incorrect usage of GET_WARNINGS_ATTR#119063
serhiy-storchaka merged 8 commits into
python:mainfrom
Eclips4:issue-119049

Conversation

Eclips4 commented May 15, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

GET_WARNINGS_ATTR is trying to get the ATTR attribute of the Python warnings module. If the third parameter is true, it tries to import the warnings module if it hasn't been imported already.
All of the calls to call_show_warning are made with the source = NULL argument, so this statement show_fn = GET_WARNINGS_ATTR(interp, _showwarnmsg, source != NULL); always becomes show_fn = GET_WARNINGS_ATTR(interp, _showwarnmsg, 0);.
So it never triess to import the warnings module if it has not already imported, which leads us to the issue described in the #119049.
This is why test_io.test_check_encoding_warning works when import warnings is used in pathlib._local.py and fails when we remove this statement.

bedevere-app Bot mentioned this pull request May 15, 2024
Closed
serhiy-storchaka reviewed May 15, 2024
View reviewed changes
Comment thread Modules/_testcapimodule.c Outdated
serhiy-storchaka approved these changes May 15, 2024
View reviewed changes

serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM.

Comment thread Modules/_testcapimodule.c Outdated
serhiy-storchaka reviewed May 15, 2024
View reviewed changes

serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented May 15, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

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 commented May 16, 2024

Copy link
Copy Markdown
Member

How difficult is to write a test for this case?

Eclips4 commented May 16, 2024

Copy link
Copy Markdown
Member Author

How difficult is to write a test for this case?

It's not difficult.

Eclips4 requested a review from serhiy-storchaka May 16, 2024 10:32
serhiy-storchaka reviewed May 16, 2024
View reviewed changes

serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread Lib/test/test_capi/test_exceptions.py Outdated
serhiy-storchaka reviewed May 16, 2024
View reviewed changes
Comment thread Lib/test/test_capi/test_exceptions.py Outdated
Comment thread Lib/test/test_capi/test_exceptions.py Outdated
Comment thread Lib/test/test_capi/test_exceptions.py Outdated
Eclips4 and others added 3 commits May 16, 2024 22:18
serhiy-storchaka approved these changes May 16, 2024
View reviewed changes

serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM.

Comment thread Lib/test/test_capi/test_exceptions.py Outdated
serhiy-storchaka added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes and removed skip news labels May 16, 2024
serhiy-storchaka enabled auto-merge (squash) May 16, 2024 20:05

serhiy-storchaka commented May 16, 2024

Copy link
Copy Markdown
Member

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

Eclips4 commented May 16, 2024

Copy link
Copy Markdown
Member Author

Thanks for the review, Serhiy.

serhiy-storchaka merged commit 100c7ab into python:main May 16, 2024

miss-islington-app Bot commented May 16, 2024

Copy link
Copy Markdown

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 request May 16, 2024
pythongh-119049: Fix incorrect display of warning which is constructe…
0de65e5 
…d by C API (pythonGH-119063)

The source line was not displayed if the warnings module had not yet
been imported.
(cherry picked from commit 100c7ab)

Co-authored-by: Kirill Podoprigora <kirill.bast9@mail.ru>

miss-islington-app Bot commented May 16, 2024

Copy link
Copy Markdown

Sorry, @Eclips4 and @serhiy-storchaka, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 100c7ab00ab66a8c0d54582f35e38d8eb691743c 3.12

bedevere-app Bot commented May 16, 2024

Copy link
Copy Markdown

GH-119106 is a backport of this pull request to the 3.13 branch.

bedevere-app Bot removed the needs backport to 3.13 bugs and security fixes label May 16, 2024

Eclips4 commented May 17, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

@serhiy-storchaka I'm trying to do backport to the 3.12 branch manually, and realize that this cannot be done without the backport of e1d8c65.
In short: to get the sources we need a new version of linecache which has _register_code function. This function is used in pythonrun.c which saves our code to the linecache.cache.

serhiy-storchaka commented May 17, 2024

Copy link
Copy Markdown
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 Bot commented May 17, 2024

Copy link
Copy Markdown

GH-119119 is a backport of this pull request to the 3.12 branch.

bedevere-app Bot removed the needs backport to 3.12 only security fixes label May 17, 2024

Eclips4 commented May 17, 2024

Copy link
Copy Markdown
Member Author

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.

Eclips4 deleted the issue-119049 branch May 17, 2024 11:53
serhiy-storchaka pushed a commit that referenced this pull request May 17, 2024
[3.13] gh-119049: Fix incorrect display of warning which is construct…
ced71d3 
…ed by C API (GH-119063) (GH-119106)

The source line was not displayed if the warnings module had not yet
been imported.
(cherry picked from commit 100c7ab)

Co-authored-by: Kirill Podoprigora <kirill.bast9@mail.ru>
serhiy-storchaka removed their assignment May 17, 2024
JelleZijlstra mentioned this pull request May 28, 2024
Closed
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
pythongh-119049: Fix incorrect display of warning which is constructe…
ba0c110 
…d by C API (pythonGH-119063)

The source line was not displayed if the warnings module had not yet
been imported.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

serhiy-storchaka serhiy-storchaka approved these changes
vstinner Awaiting requested review from vstinner
erlend-aasland Awaiting 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

Back | FazBrowse Home | New Git URL