-
Notifications
You must be signed in to change notification settings - Fork 181
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
Only prefer envs owned by the current user #323
Conversation
in cases of a shared virtualenv, this path may not be writable, causing problems for e.g. juptyerlab which writes workspaces to the default config dir, which will fail with a permission error
This now has an ownership check, which falls back on W_OK if no owner can be established, which I think will happen on Windows. |
4fd1ab1
to
1ff7a74
Compare
If another user created the env, that means it's shared and we shouldn't write config to it
1ff7a74
to
776ab03
Compare
@jasongrout had brought up some concerns about special-casing the "writability" of the path here. |
@minrk - I'd love to hear your thoughts about the points I raised at #318 (comment). I agree with you that an ownership check is a good first check. I'm still a bit concerned with the complexity of this check, but given that now two adminstrators have raised the issue as a practical problem, I think it would be a good thing to solve, especially since Min is pointing out that it is likely to be a widespread problem when using JupyterHub. I think it is really important to be very transparent and helpful to users that want to understand why Jupyter is choosing its paths and how they can affect that choice. We try to give such guidance in |
@jasongrout thanks! I left comments in #318. I think shared env detection is too complex and fraught with wrong conclusions (e.g. admin vs user getting different results for the same env). I think the solution probably belongs in checks in jupyterlab whatever package is choosing to require writing. I do believe it shouldn't be on the user to solve, though, for standard persistence behavior like jupyterlab workspaces. |
Reopening now that it's switched to an ownership check, following discussion in #318 |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
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!
Check ownership of prefix to avoid misidentifying 'shared' environments that can't be.
Heuristic being to prefer user environments to general user config, but not shared environments that may be owned by e.g. root.
in cases of a shared virtualenv, the env may be owned by root and not writable, causing problems for e.g. juptyerlab which writes workspaces to the default config dir, which will fail with a permission error. It makes sense to prefer the env when they are my envs for projects, but not for e.g. creating a user env to be used in jupyterhub. I think an ownership
writabilitycheck is a good indicator of whether it's a 'shared' env.More info: jupyterlab/jupyterlab#13580
This would fix the above issue in the real cases I've encountered, but it's not a rigorous check for the writability of the config dir specifically.
Downside of just a writability check: if an admin has write access instead of requiring e.g. root, they will still get the shared env!
With an owner check, this can still happen if a 'shared' environment is owned by one of its users. That ought to be more rare than writable, at least.
closes #318