Skip to content

gh-46927: Prevent readline from overriding environment#133585

Open
mbriand wants to merge 4 commits into
python:mainfrom
mbriand:fix-issue-46927
Open

gh-46927: Prevent readline from overriding environment#133585
mbriand wants to merge 4 commits into
python:mainfrom
mbriand:fix-issue-46927

Conversation

@mbriand

@mbriand mbriand commented May 7, 2025

Copy link
Copy Markdown

Readline library will set the LINES and COLUMNS environment variables during initialization. One might expect these variables to be updated later on SIGWINCH, but this is not the case with cpython.

As a consequence, when the readline module is imported, any process launched from cpython with the default environment will have LINES and COLUMNS variables set, potentially to a wrong value.

Use the rl_change_environment global variable to disable this initial setup of the environment variables.

@python-cla-bot

python-cla-bot Bot commented May 7, 2025

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

CLA signed

@bedevere-app

bedevere-app Bot commented May 7, 2025

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Comment thread Modules/readline.c
/* The name must be defined before initialization */
rl_readline_name = "python";

#if !defined(__APPLE__)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'm a bit surprised I had to rely on the __APPLE__ define here. As I understand apple OS is using libedit, so I would have thought relying on WITH_EDITLINE would be enough. But the macOS pipeline proved me wrong...

@ZeroIntensity ZeroIntensity 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.

Please add a test case. The file you're looking for is test_readline.

Comment thread Misc/NEWS.d/next/Core_and_Builtins/2025-05-07-18-31-31.gh-issue-46927.sF02gj.rst Outdated
@mbriand mbriand requested a review from ZeroIntensity May 14, 2025 06:26
Comment thread Lib/test/test_readline.py
@mbriand mbriand force-pushed the fix-issue-46927 branch from 6325feb to a91fe5d Compare May 15, 2025 06:18
@mbriand mbriand requested a review from ZeroIntensity May 16, 2025 05:32
@mbriand

mbriand commented Jul 10, 2025

Copy link
Copy Markdown
Author

Hi!

Any news about this PR? Is there anything I can do to help the this modification being accepted?

Readline library will set the LINES and COLUMNS environment variables
during initialization. One might expect these variables to be updated
later on SIGWINCH, but this is not the case with cpython.

As a consequence, when the readline module is imported, any process
launched from cpython with the default environment will have LINES and
COLUMNS variables set, potentially to a wrong value.

Use the rl_change_environment global variable to disable this initial
setup of the environment variables.
@mbriand

mbriand commented Feb 13, 2026

Copy link
Copy Markdown
Author

I rebased on current main branch. I confirm the issue is still present today.

Again, if there is anything I can to to help getting this merged, please tell me.

@github-actions

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 24, 2026
@serhiy-storchaka

Copy link
Copy Markdown
Member

Verified locally — without it LINES/COLUMNS get set and your test fails; with it they stay unset and it passes.

The #if !defined(__APPLE__) guard also keeps the assignment on non-Apple libedit builds that lack the symbol. I opened mbriand#1 against your branch replacing it with a HAVE_RL_CHANGE_ENVIRONMENT configure probe (like the existing rl_resize_terminal check) — feel free to merge it in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting review stale Stale PR or inactive for long period of time.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants