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: Remove check_path to allow running in debugger #20091

Merged
merged 1 commit into from
Nov 21, 2022

Conversation

maurerle
Copy link
Contributor

@maurerle maurerle commented Nov 19, 2022

Description of Changes

PYTHONPATH entries are removed on start, resulting in a the warning shown on check_path in requirements - program does not start.
Removing pythonpath was added to disable shadowed base libraries, but I think thats exactly how the PYTHONPATH could and would behave.
Still it makes sense to remove them so that it does not create problems.

Yet the resulting warning
"Spyder must be installed properly (e.g. from source: 'python setup.py install'), or directory 'dir' must be in PYTHONPATH environment variable."
is not helpful and is removed through this PR

I would therefore not remove entries on PYTHONPATH from PATH

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: Florian Maurer

@maurerle maurerle force-pushed the feature/fix_pythonpath_loading branch from cfe5fad to eede2d1 Compare November 19, 2022 09:42
@ccordoba12
Copy link
Member

ccordoba12 commented Nov 19, 2022

Hey @maurerle, I don't agree with this change. We remove PYTHONPATH in start.py to protect Spyder from crashing at startup when, for instance, a user adds to PYTHONPATH a module named random.py or string.py, which is not uncommon.

You also said:

PYTHONPATH entries are removed on start, resulting in a the warning shown on check_path in requirements - program does not start.

The way you need to start Spyder when developing it is by running python bootstrap.py in terminal, as explained in our Contributing guide. That makes a lot of adjustments necessary for development (like installing Spyder and our subrepos in external-deps in development mode). So, if you're starting Spyder in a different way, please resort back to this mechanism, which is used by the entire development team.

@maurerle
Copy link
Contributor Author

maurerle commented Nov 19, 2022

So I sometimes debug Spyder (like with the actual debugger) using VSCode, that way:

conda create -n spyder python=3.10
cd spyder
pip install -e .[test]

Then select the newly created python interpreter from spyder environment and hit F5 to debug the bootstrap.py command.
This runs /usr/bin/env /opt/conda/envs/spyder/bin/python /home/maurer/.vscode-o ss/extensions/ms-python.python-2022.18.2/pythonFiles/lib/python/debugpy/adapter/../../debugpy/launcher 43327 -- /home/maurer/gitea/spyder/bootstrap.py

And in that situation I am getting here:

image

So you might say that that way is not supported and i should rather run something like python -m debugpy --listen 43327 bootstrap.py and attach my debugger to that process but in this case the message is not very helpful, as my PYTHONPATH is correct?

@ccordoba12
Copy link
Member

Ok, I see. Then please check what happens if you remove the call to check_path in mainwindow.py. It shouldn't be necessary anymore.

@maurerle maurerle force-pushed the feature/fix_pythonpath_loading branch from eede2d1 to 12a6bdf Compare November 21, 2022 07:36
@maurerle
Copy link
Contributor Author

Yes, that works too, great.
I updated my PR accordingly.

The check was introduced in 1f73f2f - I did not find which case it was meant to handle, but it is already 12 years ago.

@maurerle maurerle changed the title PR: Feature/fix pythonpath loading PR: Remove check_path to allow running in debugger Nov 21, 2022
@ccordoba12 ccordoba12 added this to the v5.4.1 milestone Nov 21, 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.

Looks good to me now, thanks @maurerle!

@ccordoba12 ccordoba12 changed the base branch from master to 5.x November 21, 2022 16:03
@ccordoba12 ccordoba12 merged commit d784691 into spyder-ide:5.x Nov 21, 2022
ccordoba12 added a commit that referenced this pull request Nov 21, 2022
@ccordoba12
Copy link
Member

ccordoba12 commented Nov 21, 2022

The check was introduced in 1f73f2f - I did not find which case it was meant to handle, but it is already 12 years ago.

Until a few months ago we injected the git clone Spyder directory into sys.path, but now we install Spyder in development mode. So, that check is not necessary anymore.

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.

None yet

2 participants