PR: Consistently handle PYTHONPATH and add Import feature to PYTHONPATH Manager

[mrclary]

Description of Changes

Explicitly excluded system PYTHONPATH paths from Editor completion paths, IPython Console sys.path, and IPython Console os.environ['PYTHONPATH'].
Added feature to import system PYTHONPATH paths into PYTHONPATH Manager.

The "Syncronize..." feature (Windows only) is changed to "Export" to better complement "Import" and comport with what it actually does. This remains a Windows only feature for the time being.

Resolved an issue where a path was added after canceling "Select directory" dialog.

Issue(s) Resolved

Fixes #17511

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:

[pep8speaks]

Hello @mrclary! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file external-deps/spyder-kernels/spyder_kernels/customize/spyderpdb.py:

Line 659:80: E501 line too long (80 > 79 characters)
Line 850:80: E501 line too long (80 > 79 characters)

• In the file spyder/utils/icon_manager.py:

Line 216:80: E501 line too long (88 > 79 characters)

Comment last updated at 2022-06-21 20:52:46 UTC

[mrclary]

We had been removing PYTHONPATH from env_vars and using SPY_PYTHONPATH instead. This is only used to set the sys.path paths in the IPython Console at launch. Eliminating SPY_PYTHONPATH in favor of PYTHONPATH will produce the same result while also providing os.environ['PYTHONPATH'] in the IPython Console and obviate set_spyder_pythonpath in spyder-kernels. This prevents kernel crashes at startup if PYTHONPATH Manager paths shadow standard libraries.

A companion PR spyder-ide/spyder-kernels#378 accommodates this change and updates IPython Console's os.environ['PYTHONPATH'] along with the sys.path update.

[ccordoba12]

Thanks for your work on this @mrclary! I'm leaving it for 5.3.1 because this is an important change and we need to carefully discuss about it.

[ccordoba12]

@mrclary, we discussed your PR with @CAM-Gerlach and @isabela-pf this week and we consider it requires some UI/UX improvements, so that users better understand how the PYTHONPATH manager works with the improvements you propose.

@isabela-pf will be posting more details about it soon.

[mrclary]

@mrclary, we discussed your PR with @CAM-Gerlach and @isabela-pf this week and we consider it requires some UI/UX improvements, so that users better understand how the PYTHONPATH manager works with the improvements you propose.

@isabela-pf will be posting more details about it soon.

Thanks, @ccordoba12 @CAM-Gerlach and @isabela-pf .

Another thing to discuss is whether we should intercept PYTHONPATH in Spyder at launch. In this PR I have only ensured that PYTHONPATH is handled consistently between platforms, launch mechanisms, and completions/IPython Console. @ccordoba12 implemented SPY_PYTHONPATH to intercept PYTHONPATH Manager paths and re-inject them into sys.path in IPython Console without crashing the kernel if the user shadowed standard libraries. This was a good idea and I'm wondering if we shouldn't implement something similar for Spyder at launch. Spyder does not need the user's system PYTHONPATH for anything, but it does get injected into Spyder's runtime sys.path, so if it does shadow standard libraries it could cause problems for Spyder. Although I don't think we've seen any issues posted, so maybe it's nothing to be concerned about, it seems a relatively cheap thing to do to ensure Spyder is robust against user settings.

[mrclary]

@isabela-pf, did you have some feedback on this PR?

[steff456]

It looks better!

[ccordoba12]

Agreed, it looks much nicer.

[ccordoba12]

Last minor details @mrclary, then this should be ready.

Also, since I just merged spyder-ide/spyder-kernels#378, please resync the spyder-kernels subrepo with the instructions in our Contributing guide.

[ccordoba12]

Looks good to me now, thanks @mrclary!

[dalthviz]

Thanks @mrclary ! Checking this locally on Windows seems like is working correctly although to import exported paths you need to restart Spyder and/or run it from a new console/cmd. Seems like the changes done to PYTHONPATH are not being taken into account without that (I think that's something that was happening before this PR though, right?). So for example, if I use Add a path, use Export, remove the path from the manager with Remove path and then try to use Import I see the empty PYTHONPATH while in the consoles I can get the PYTHONPATH update:

If that is expected then this LGTM 👍

[mrclary]

Thanks @mrclary ! Checking this locally on Windows seems like is working correctly although to import exported paths you need to restart Spyder and/or run it from a new console/cmd. Seems like the changes done to PYTHONPATH are not being taken into account without that (I think that's something that was happening before this PR though, right?). So for example, if I use Add a path, use Export, remove the path from the manager with Remove path and then try to use Import I see the empty PYTHONPATH while in the consoles I can get the PYTHONPATH update:

The "import" feature currently uses the existing os.environ['PYTHONPATH'] in Spyder's runtime environment which is established at launch. We could change this with one of the following options:

• Upon exporting PPM to the system PYTHONPATH (Windows only), we could also appropriately update Spyder's runtime os.environ['PYTHONPATH']. Spyder does not use this internally for any reason, so this should be safe.
• Rather than relying on os.environ['PYTHONPATH'], we could retrieve the system environment variable directly. This should be straightforward on Windows by querying the registry (I think this is already done for the purpose of exporting anyway). On unix, a login shell subprocess would be needed to extract the environment variable; this would require a little more work to setup properly as it depends on the user's default shell etc., but is certainly feasible.

The second option may be more robust and future-proof. For example, @CAM-Gerlach has alluded to pending updates to Python which may use new -P flags by default which will preclude use of PYTHON* environment variables. If/when this is the case, os.environ['PYTHONPATH'] may no longer be available by default, so a different mechanism may be prudent.

If that is expected then this LGTM 👍

Technically, it is expected. Nevertheless, it may not be desirable. I think I'll include a fix for this in this PR. Unless @ccordoba12, @dalthviz, or @CAM-Gerlach have anything other recommendations, I'll proceed with the second option.

[dalthviz]

Sounds good to me 👍 Although, since the current behavior is expected maybe a new issue could be open to track the missing improvements (probably using the comment above #17512 (comment) as an starting point). In that way we could check a little bit more ideas and other missing things regarding the PYTHONPATH manager without that being a blocker to merge this

[mrclary]

Sounds good to me 👍 Although, since the current behavior is expected maybe a new issue could be open to track the missing improvements (probably using the comment above #17512 (comment) as an starting point). In that way we could check a little bit more ideas and other missing things regarding the PYTHONPATH manager without that being a blocker to merge this

Good idea. I'll proceed with the second option in another pull request in order to allow more in-depth feedback without blocking this PR.

[mrclary]

I'm glad @CAM-Gerlach was able to clarify! I don't want to block this PR with my limited time, so please don't feel the need to wait for me.

@isabela-pf, based on this comment, I'll remove you from the reviewer list. If you still wish to review, please feel free to add yourself back. Thanks for the previous feedback.

[steff456]

Thanks for working on this! I think it is ready to merge 😬

[CAM-Gerlach]

The second option may be more robust and future-proof. For example, @CAM-Gerlach has alluded to pending updates to Python which may use new -P flags by default which will preclude use of PYTHON* environment variables. If/when this is the case, os.environ['PYTHONPATH'] may no longer be available by default, so a different mechanism may be prudent.

To clarify, all -P does is disable inserting the current working directory (for -c, -m and the interactive interpreter) or the script directory (for scripts) at sys.path[0]. We hope to someday make it the default, but there is no concrete plan and that day would be a ways off if it comes. Therefore, this has no effect on PYTHONPATH. It is -E that ignores all environment variables, including PYTHONPATH, which is in turn implied by -I (along with -s and, crucially, not inserting the cwd/script dir on the path). However, even then, that is only for constructing the initial sys.path for the Python interpreter (which Spyder already ignores with this PR), it certainly doesn't mean that manually running os.environ["PYTHONPATH"] doesn't work.

To the contrary, I would think that relying on Python's mature, battle tested and widely used facilities for OS interaction are much more robust and reliable than anything we try to hack on our own, and if we want to address this here rather than in a separate PR, the first approach would make more sense and be more consistent (as well as much simpler to implement).

[CAM-Gerlach]

I confess I have not done another round of actual testing or detailed code analysis, to which I defer to the others, but I'm ✔️ on the high-level design and UI/UX (pending optional resolution of the concern above). Thanks @mrclary !

[mrclary]

The second option may be more robust and future-proof. For example, @CAM-Gerlach has alluded to pending updates to Python which may use new -P flags by default which will preclude use of PYTHON* environment variables. If/when this is the case, os.environ['PYTHONPATH'] may no longer be available by default, so a different mechanism may be prudent.

To clarify, all -P does is disable inserting the current working directory (for -c, -m and the interactive interpreter) or the script directory (for scripts) at sys.path[0]. We hope to someday make it the default, but there is no concrete plan and that day would be a ways off if it comes. Therefore, this has no effect on PYTHONPATH. It is -E that ignores all environment variables, including PYTHONPATH, which is in turn implied by -I (along with -s and, crucially, not inserting the cwd/script dir on the path). However, even then, that is only for constructing the initial sys.path for the Python interpreter (which Spyder already ignores with this PR), it certainly doesn't mean that manually running os.environ["PYTHONPATH"] doesn't work.

To the contrary, I would think that relying on Python's mature, battle tested and widely used facilities for OS interaction are much more robust and reliable than anything we try to hack on our own, and if we want to address this here rather than in a separate PR, the first approach would make more sense and be more consistent (as well as much simpler to implement).

Thanks for clarifying. I had the mistaken impression that these would affect os.environ and they do not.

Nevertheless, I still recommend addressing this and any other possible issues in #18397, even if they may be trivial, in order to not prolong this PR.