Skip to content

gh-127873: Only check sys.flags.ignore_environment for PYTHON* env vars #127877

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
Jan 21, 2025

Conversation

hugovk
Copy link
Member

@hugovk hugovk commented Dec 12, 2024

When -E is passed (and therefore sys.flags.ignore_environment is true), we should only ignore only env vars matching PYTHON*; in this case, only PYTHON_COLORS.

We should not ignore the other env vars: FORCE_COLOR/NO_COLOR/TERM.

Re: https://docs.python.org/3/using/cmdline.html#cmdoption-E

Fixing this revealed some tests that failed because of inserted colour codes. For example, in "ZeroDivisionError: division by zero", the parts "ZeroDivisionError", ":" and "division by zero" are all different colours.

For these test cases, I wrapped them with the @force_not_colorized decorator, or, where there were a number of them, I wrapped the test class in the new force_not_colorized_test_class decorator. This was a suggestion from #127223 (comment). And from that PR, I replaced some of the numerous function decorators with the class decorator.

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @hugovk for commit 80422ff 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 13, 2024
@hugovk

This comment was marked as resolved.

@hugovk
Copy link
Member Author

hugovk commented Dec 17, 2024

@pablogsal Please could you have a look at this? Thanks!

@brettcannon brettcannon removed their request for review January 13, 2025 18:21
@serhiy-storchaka
Copy link
Member

Why do we use names without the PYTHON prefix? Are FORCE_COLOR and NO_COLOR the part of some (de-facto?) standard?

@hugovk
Copy link
Member Author

hugovk commented Jan 20, 2025

Why do we use names without the PYTHON prefix? Are FORCE_COLOR and NO_COLOR the part of some (de-facto?) standard?

Yes, see https://force-color.org/ and https://no-color.org.

@hugovk hugovk marked this pull request as draft January 20, 2025 16:10
@hugovk
Copy link
Member Author

hugovk commented Jan 21, 2025

Please add tests for these environment variables when run Python with the -E option.

Updated to use make_clean_env() from the PyREPL tests (and moved it to test.support for re-use), which removes the PYTHON* env vars to isolate the tests and simulate the -E option.

@hugovk hugovk marked this pull request as ready for review January 21, 2025 14:16
Copy link
Member

@serhiy-storchaka serhiy-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. Although you can use support.force_not_colorized_test_class in few more files.

@hugovk hugovk enabled auto-merge (squash) January 21, 2025 15:54
@hugovk
Copy link
Member Author

hugovk commented Jan 21, 2025

Thank you for the reviews!

@hugovk hugovk merged commit 05d12ee into python:main Jan 21, 2025
38 checks passed
@hugovk hugovk deleted the 3.14-fix-colour-env-var-check branch January 21, 2025 16:10
@miss-islington-app
Copy link

Thanks @hugovk for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Sorry, @hugovk, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 05d12eecbde1ace39826320cadf8e673d709b229 3.13

hugovk added a commit to hugovk/cpython that referenced this pull request Jan 21, 2025
… `PYTHON*` env vars (pythonGH-127877)

(cherry picked from commit 05d12ee)

Co-authored-by: Hugo van Kemenade <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Jan 21, 2025

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

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jan 21, 2025
hugovk added a commit that referenced this pull request Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants