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: Do not check for updates if Spyder is in a system or managed environment #22631

Merged
merged 3 commits into from
Oct 9, 2024

Conversation

mrclary
Copy link
Contributor

@mrclary mrclary commented Oct 3, 2024

Description of Changes

Check that sys.executable is not in /usr/bin or /usr/local/bin

Issue(s) Resolved

Fixes #22407

@mrclary mrclary self-assigned this Oct 3, 2024
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 for addressing this @mrclary!

spyder/plugins/updatemanager/widgets/update.py Outdated Show resolved Hide resolved
@mrclary
Copy link
Contributor Author

mrclary commented Oct 4, 2024

According to doc page:

  1. Is it running outside of a virtual environment? It can determine this by whether sys.prefix == sys.base_prefix.
  2. Is there an EXTERNALLY-MANAGED file in the directory identified by sysconfig.get_path("stdlib", sysconfig.get_default_scheme())?

On my machine, I have the following observations.

  • sys.prefix == sys.base_prefix evaluates True for the system Python (/usr/bin/python3), conda environments, and pyenv environments. It evaluates False for virtual environments created with python -m venv.
  • There is no EXTERNALLY-MANAGED file in the directory identified by sysconfig.get_path("stdlib", sysconfig.get_default_scheme()) for pyenv (didn't expect there to be) nor for the system. For my system, Python=3.9.6, sysconfig.get_default_scheme does not exist, but sysconfig.get_path("stdlib") works just the same for both 3.9 and 3.10.

So, I think we should do the following checks:

  • sys.executable.startswith(('/usr/bin/', '/usr/local/bin/'))
  • os.path.exists(os.path.join(sysconfig.get_path('stdlib'), 'EXTERNALLY-MANAGED'))

If either are true, then do not check for updates.

But we need to exclude conda envs from this check, otherwise we won't be able to update Spyder in them.

We should not have to worry about this with the above checks.

@ccordoba12
Copy link
Member

We should not have to worry about this with the above checks.

The problem is that EXTERNALLY-MANAGED can be added at some point to conda envs. The presence of that file prevents pip from managing packages for the corresponding env or installation, which applies to conda envs because they're managed by conda/mamba/micromamba.

@mrclary
Copy link
Contributor Author

mrclary commented Oct 4, 2024

The problem is that EXTERNALLY-MANAGED can be added at some point to conda envs. The presence of that file prevents pip from managing packages for the corresponding env or installation, which applies to conda envs because they're managed by conda/mamba/micromamba.

I still don't quite understand. If EXTERNALLY-MANAGED is added to a conda environment, then neither pip nor conda will be able to modify the environment, correct?
If this is the case, then we would not be able to update the environment, so we should exclude it from checking for updates.
If this is not the case, i.e. pip cannot modify the environment but conda can modify the environment, we could update the environment, but I think we should still respect the intent of the EXTERNALLY-MANAGED file and not update.

@ccordoba12
Copy link
Member

I still don't quite understand. If EXTERNALLY-MANAGED is added to a conda environment, then neither pip nor conda will be able to modify the environment, correct?

Nop, only pip won't be able to change the env and that's because conda is managing it.

If this is not the case, i.e. pip cannot modify the environment but conda can modify the environment, we could update the environment, but I think we should still respect the intent of the EXTERNALLY-MANAGED file and not update.

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.

@mrclary
Copy link
Contributor Author

mrclary commented Oct 9, 2024

Nop, only pip won't be able to change the env and that's because conda is managing it.

Okay, so then pip install ... would fail in a conda environment if EXTERNALLY-MANAGED is present, and the appropriate interpretation is that the environment allows conda to manage but not pip.

@ccordoba12
Copy link
Member

Okay, so then pip install ... would fail in a conda environment if EXTERNALLY-MANAGED is present, and the appropriate interpretation is that the environment allows conda to manage but not pip.

Exactly.

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.

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()
Copy link
Member

@ccordoba12 ccordoba12 Oct 9, 2024

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

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

Copy link
Member

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.

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!

@ccordoba12 ccordoba12 changed the title PR: Do not check for updates if the environment is a system or managed environment PR: Do not check for updates if Spyder is in a system or managed environment Oct 9, 2024
@ccordoba12 ccordoba12 merged commit f10e434 into spyder-ide:master Oct 9, 2024
17 checks passed
@ccordoba12
Copy link
Member

@meeseeksdev please backport to 6.x

meeseeksmachine pushed a commit to meeseeksmachine/spyder that referenced this pull request Oct 9, 2024
ccordoba12 pushed a commit that referenced this pull request Oct 9, 2024
…der is in a system or managed environment) (#22657)
@mrclary mrclary deleted the issue-22407 branch October 10, 2024 20:19
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.

Update mechanism not working for Spyder 6.0 installed from Fedora distro package
2 participants