-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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: Do not check for updates if Spyder is in a system or managed environment #22631
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing this @mrclary!
According to doc page:
On my machine, I have the following observations.
So, I think we should do the following checks:
If either are true, then do not check for updates.
We should not have to worry about this with the above checks. |
The problem is that |
I still don't quite understand. If |
Nop, only pip won't be able to change the env and that's because conda is managing it.
EXTERNALLY-MANAGED was created for pip (not for conda) so that it doesn't touch envs that are managed by a different package manager (not only conda, but brew, apt, dnf, etc). Furthermore, if that file is added by default to conda envs in the future, then we wouldn't be able to update our own installers. |
Okay, so then |
Exactly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last comment, then this should be ready.
Also, you need to rebase on top of master so that this PR runs our new Remote client tests.
SKIP_CHECK_UPDATE = ( | ||
sys.executable.startswith(('/usr/bin/', '/usr/local/bin/')) | ||
or ( | ||
not is_anaconda() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we use is_conda_env
here? You can import it from Spyder-kernels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question.
We use is_anaconda
throughout Spyder's source code and it does essentially the same thing, so it does duplicate code, which is not ideal. I would suggest that we use is_conda_env
from spyder-kernels
exclusively and remove is_anaconda
from the Spyder source code. Is there a compelling reason to keep is_anaconda
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, I recommend removing is_anaconda
in a separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest that we use is_conda_env from spyder-kernels exclusively and remove is_anaconda from the Spyder source code. Is there a compelling reason to keep is_anaconda?
is_anaconda
came first and it's a simpler check because it doesn't depend on an interpreter. But I agree, we could remove it and leave is_conda_env
instead, to make things simpler.
To be clear, I recommend removing is_anaconda in a separate PR
Ok, I think that's a good idea.
On a second thought, I think this is the right check in this case because we need to run it only for Spyder's interpreter. So, sorry for the noise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mrclary!
@meeseeksdev please backport to 6.x |
…is in a system or managed environment
…der is in a system or managed environment) (#22657)
Description of Changes
Check that
sys.executable
is not in/usr/bin
or/usr/local/bin
Issue(s) Resolved
Fixes #22407