Skip to content
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

PR: Fix issue where user environment variables with line endings were not parsed correctly on Unix platforms #20106

Merged
merged 5 commits into from
Feb 3, 2023

Conversation

mrclary
Copy link
Contributor

@mrclary mrclary commented Nov 22, 2022

Description of Changes

#20297 unsatisfactorily fixed #20097 by skipping environment variables that do not parse correctly. This PR remedies this.

  • For unix, use a clean subprocess environment, invoke a login shell, extract os.environ from a python process. This circumvents any parsing issues and should reflect exactly what a user would expect from a python process in a standard terminal.
  • For Windows, get environment variables directly from the system registry.
  • Add regression test for ValueError when trying to get environment variables on Linux #20097
    • Modify user environment variables in system registry or shell startup file
    • Restore user environment variables
    • Only run on CI to protect developers' local user environment variables

Issue(s) Resolved

Fixes #20637.
Fixes #20242.
See #20097

@mrclary mrclary self-assigned this Nov 22, 2022
@mrclary mrclary added this to the v5.4.1 milestone Dec 1, 2022
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks @mrclary for your work on this! Looks good to me, but I think it'd be nice if you could add a test for issue #20097, so we don't introduce a regression for it in the future.

@mrclary
Copy link
Contributor Author

mrclary commented Dec 2, 2022

@ccordoba12, this might be a good time to review what the get_user_environment_variables function should actually do.

Perhaps we need to define, for the purposes of Spyder, what "user environment variables" means. I have a working definition which is:

  • The exported environment variables present when a user starts a login shell.

For Windows users, this is very straight-forward: everything inherits from the registry, so these values can be collected directly from there.

For unix platforms, this is more complicated: it depends on the default shell and the profile startup scripts (e.g. /etc/bashrc, ~/.bash_profile, etc.). Ostensibly, os.environ has the "user environment variables", but it has additional variables used by Spyder, so cannot be relied on to be equivalent to "user environment variables". Additionally, the "user environment variables" in os.environ are set at Spyder launch, so Spyder would have to be restarted if these variables change.

For these reasons, I was motivated to use a subprocess to get the "user environment variables" rather than os.environ. However, there is an oversight here because the subprocess actually injects os.environ into the environment variables by default, so at present it does not achieve its intended goal. I was curious how the unit test for the pythonpath manager plugin worked when assigning os.environ['PYTHONPATH']; this is how I realized the oversight.

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Great work here @mrclary! I left some small suggestions for you, otherwise looks almost ready to me.

conftest.py Outdated Show resolved Hide resolved
spyder/utils/environ.py Outdated Show resolved Hide resolved
spyder/utils/environ.py Outdated Show resolved Hide resolved
spyder/utils/environ.py Outdated Show resolved Hide resolved
spyder/utils/environ.py Outdated Show resolved Hide resolved
spyder/utils/tests/test_environ.py Outdated Show resolved Hide resolved
conftest.py Outdated Show resolved Hide resolved
@ccordoba12 ccordoba12 changed the title PR: Fix issue where user environment variables with line endings were not parsed correctly on unix platforms PR: Fix issue where user environment variables with line endings were not parsed correctly on Unix platforms Feb 3, 2023
* from registry (Windows)
* from os.environ of python process in clean login shell
* Don't give impression that set_user_env can write environment variables for general purposes on Unix systems.
* More descriptive variable name in get_user_environment_variables
* Move restore_user_env to spyder/app/tests/conftest.py to avoid Spyder imports in top-level conftest.py
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Looks good to me now, thanks @mrclary!

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

Successfully merging this pull request may close these issues.

2 participants