-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Ensure writable paths are used for wheel installer #9014
base: main
Are you sure you want to change the base?
Conversation
This change fixes an issue where when system environment is used as a destination for wheel installation as a user without access to system site packages, installation fails with permission error. Functionally, this now follows the same behaviour as when a package is installed to user site. Resolves: python-poetry#8408
@@ -312,7 +349,9 @@ def run_pip(self, *args: str, **kwargs: Any) -> str: | |||
def run_python_script(self, content: str, **kwargs: Any) -> str: | |||
return self.run( | |||
self._executable, | |||
"-I", | |||
# if isolation is enabled for system env, user site would be |
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 suppose this reintroduces #6627, which was the reason for adding this flag
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.
Good catch thanks. Let me see if there is a better way.
"data", | ||
]: | ||
if not is_dir_writable(path=Path(paths[key]), create=True): | ||
overrides[key] = paths[key].replace( |
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.
Is it ok to end up potentially with a mix of system and user directories here? This is not something I have looked into but I would have guessed you'd want all or nothing
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.
pretty sure there is a feature request somewhere asking for --user
type installs, perhaps it makes sense to do that instead, have this be explicit
would it get messy if eg you provide --user
on install
but forget it on update
, or similar? I dont know.
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 suspect that would confuse things more from the user perspective. Especially because we are predominantly servicing virtual environments.
Such a flag would in theory only be used when the user knows they are running in a system environment and does not have write access to the system environment. Hence, I'm not sure how valuable it might be as a cli option. Maybe a config option, I do not know.
If I understood the issues requesting --user
, this would simply solve the root issue for them.
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.
Is it ok to end up potentially with a mix of system and user directories here? This is not something I have looked into but I would have guessed you'd want all or nothing
In practice it shouldn't matter. And in reality it is highly unlikely that parts of it is writable and others not.
Python will look everywhere by default.
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.
hmm. pip
checks all directories when making the same decision I think - https://github.com/pypa/pip/blob/f4a543917eac822a3b06ab0fcc4df7c4e37e387a/src/pip/_internal/commands/install.py#L652. And then once it has made that decision it is fully committed to a user install rather than a site-wide install.
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.
We can replicate that I guess.
base_path, paths["userbase"] | ||
) | ||
|
||
for k, v in overrides.items(): |
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.
paths.update(overrides)
?
@@ -242,6 +244,41 @@ def paths(self) -> dict[str, str]: | |||
|
|||
return self._paths | |||
|
|||
@property |
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.
We may use @cached_property
instead so we don't need _installer_scheme_dict
.
This change fixes an issue where when system environment is used as a destination for wheel installation as a user without access to system site packages, installation fails with permission error.
Functionally, this now follows the same behaviour as when a package is installed to user site.
Resolves: #8408
console.log