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 getting user environment variables on Posix systems and pass them to the IPython console #21065

Merged
merged 6 commits into from
Jun 25, 2023

Conversation

mrclary
Copy link
Contributor

@mrclary mrclary commented Jun 23, 2023

Description of Changes

  • Use a shell script in Spyder's config directory to retriev user environment variables on posix systems.
  • The shell script is created on Spyder startup and only for posix systems.

To get user environment variables from login and interactive startup files on posix, both -l and -i flags must be used. Parsing the environment list is problematic if the variable has a newline character, but Python's os.environ can do this for us. However, executing Python in an interactive shell in a subprocess causes Spyder to hang on Linux. Using a shell script resolves the issue. Note that -i must be in the sha-bang line; if -i and -l are swapped, Spyder will hang.

Also note that for environment variables for KernelSpec objects are first set to the user's environment variables, then updated with os.environ. Thus user login and interactive variables are ensured to be present, but values in os.environ may override user variable values.

Issue(s) Resolved

Part of #3891

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct:

@mrclary mrclary added this to the v6.0alpha2 milestone Jun 23, 2023
@mrclary mrclary requested a review from ccordoba12 June 23, 2023 16:45
@mrclary mrclary self-assigned this Jun 23, 2023
@mrclary mrclary removed the request for review from ccordoba12 June 23, 2023 16:45
… and interactive starup files.

I think that proc.communicate is the only real source for errors, so I eliminated the outer-most try block.
Added a timeout to proc.communicate to handle possible hangs (interactive input requests in startup files?)
Interactive startup files, e.g. ~/.bashrc, will not be sourced.
@mrclary mrclary force-pushed the issue-3891-env-var branch 3 times, most recently from 6a65f83 to 9c58cc6 Compare June 24, 2023 03:41
…ython consoles).

Do not need to get user variables for macOS app startup.
Create shell script on module import based on user's shell
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!

spyder/utils/environ.py Outdated Show resolved Hide resolved
spyder/utils/environ.py 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 Show resolved Hide resolved
spyder/utils/environ.py Show resolved Hide resolved
@ccordoba12
Copy link
Member

Note: This doesn't completely solve issue #3891 because that would require adding a tab to the IPython console preferences so that users can add env vars without editing their bashrc or similar files.

@mrclary
Copy link
Contributor Author

mrclary commented Jun 24, 2023

Note: This doesn't completely solve issue #3891 because that would require adding a tab to the IPython console preferences so that users can add env vars without editing their bashrc or similar files.

I suppose this is a UX issue and maybe we could discuss it more at one of our dev meetings. I see a few additional ideas:

  1. We could modify the user's ~/.bashrc, similar to how we add the aliases for the Linux installer. We could just delimit a section like conda does, e.g. >>> Spyder Injected Variables >>> ... <<< Spyder Injected Variables <<<, or similar. This would then provide similar functionality to the user environment variables widget as we have for Windows.
  2. Alternatively, we could treat the user environment variables similar to how we manage the PYTHONPATH. We could show what is provided by the system, allow the user to add and/disable variables, but these don't get applied upstream to the system, only to downstream processes. The current Windows functionality does not conform to this, so maybe we consider changing it in this scenario.

spyder/utils/environ.py Outdated Show resolved Hide resolved
spyder/utils/environ.py Outdated Show resolved Hide resolved
spyder/utils/environ.py Show resolved Hide resolved
@ccordoba12
Copy link
Member

I suppose this is a UX issue and maybe we could discuss it more at one of our dev meetings

Agreed. We need to solve that in another PR.

Alternatively, we could treat the user environment variables similar to how we manage the PYTHONPATH.

I like this idea. That way the env vars managed by us would be propagated to the LSP, IPython console and all other things we run in external processes.

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 tested this locally and it's working as expected.

@ccordoba12 ccordoba12 changed the title PR: Fix issue getting user environment variables on posix systems PR: Fix issue getting user environment variables on Posix systems and pass them to the IPython console Jun 25, 2023
@ccordoba12 ccordoba12 merged commit 7ff772f into spyder-ide:master Jun 25, 2023
@mrclary mrclary deleted the issue-3891-env-var branch June 26, 2023 14:03
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