-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Fail early when install path is not writable #7828
Conversation
e4e0dd9
to
0f50737
Compare
SICK!!! |
|
||
@mark.parametrize('kw', ({'use_user_site': False}, {'root_path': 'baz'})) | ||
def test_global_site_nonwritable(kw, nonwritable_global, virtualenv_global): | ||
"""Test error handling and user site-packages decision |
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.
FYI this docstring is incompatible w/ PEP 257 that says that the "title" line of a docstring should fit 72 chars and end with a period. Then, you should add an empty line if you want to follow-up with a description part.
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, I forgot about that. Though, since this is a test module, unless you feel really a strong against it, I'll probably leave it as-is.
To move this forward, I'm unlinking GH-7829 by exclude |
After the latest commit, I think this PR no longer needs design discussion. I am ready for implementation reviews! |
b928ef1
to
ad17477
Compare
I filed McSinyx#2, to blacken the decide_user_install function, since I find the code style used in this PR extremely difficult to follow. |
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'm finding it exceedingly difficult to review this PR due to the code style. Here are some preliminary comments.
logger.debug("Non-user install since user site-packages are disabled " | ||
"for security reasons or by an administrator.") |
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.
Assuming/implying "for security reasons or by an administrator" here is incorrect.
I'd prefer we avoid guessing why the variable has a given value and simplify this logic to say:
logger.debug("Non-user install since user site-packages are disabled " | |
"for security reasons or by an administrator.") | |
logger.debug( | |
"User site-packages is disabled. Will use environment's default site-packages." | |
) |
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.
This is taken straight from the docs. Since this is debug level and there's a distinction between what an user can do (when ENABLE_USER_SITE is False) and what perse probably can't (when ENABLE_USER_SITE is None), I think it might be reasonable to keep this information. I'll rephrase this a little to remove the awkward Non-user install
thing though.
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.
Even so, I'd much rather we log the value of ENABLE_USER_SITE rather than reproducing that information incompletely.
"None means it was disabled for security reasons (mismatch between user or group id and effective id) [snip]" reads very differently from "user site-packages are disabled for security reasons or [snip]".
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.
Does 9306254 do what you mean?
Thanks, I'll cherry pick it here along with your other suggestions. I'm really happy that this is moving forward! Regarding the style, to this day I'm still wondering where black took inspiration to put the closing parentheses on a separate line: it's the most unpythonic way possible to style Python code (in contrast to the exclusion of braces to format code). I don't even recall seeing it in C/C++ code: at least Linux/K&R and GNU style guide both recommend against it. I have no problem following it in my more recent PRs though, given consistency would save me from make meaningless rants like this 😄 |
At the same time install location compatibility is checked.
Functional tests for conflict install locations are removed in favor of the unittest which handle more cases.
Co-authored-by: Sviatoslav Sydorenko <[email protected]>
The use of the function is also emphasized.
Effectively user-site also need write permission checking.
Off topic, advice around code style discussions -- I suggest trying to avoid of invoking "pythonic" and similar purely-subjective heuristics when talking about code style, which always pushes the discussion into an unproductive direction. It's almost always better to try to state reasons for preferring a certain style in terms of what benefits you get from it / in terms of the difference in tradeoffs. For example, folks have called f-strings and the walrus operator "unpythonic", and yet those are effective tools for writing code that more clearly expresses its logic. |
Thanks, to be fair, my preference is purely for the look/the way my eyes scan the code, and discussion about such esthetic, much like discussion about editors' keybindings, never lead the anywhere as you suspected. I think I broke my promise about the last time mentioning the line break thing in pip, so I'll stop now to avoid wasting the time of mine and others on something I already agreed. To anyone got notified because of this, please take my apologies. |
This is to be squashed with the commit above
Maintainers of pip, |
@pypa/pip-committers ^ |
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 think @pradyunsg's comment from here still applies - this PR is way too hard to review. It should be refactored into smaller pieces, cosmetic and unrelated changes should be removed, and the functionality should be added by making minimal changes to the existing code, not by wholesale rewrites.
I would have expected a PR entitled "fail early when install path is not writeable" to consist of a check being added at the point where we determine what the install location is, that says "is this location writeable? If not, fail". If that's not possible, maybe we should start with a refactoring that introduces a point where we log "Installing to XXX" - that can serve as a checkpoint for where we'd introduce the test. Then a second PR could add the check, once the first PR has landed.
@@ -5,8 +5,9 @@ | |||
import operator | |||
import os | |||
import shutil | |||
import site | |||
from distutils.util import change_root |
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'd suggest we don't use change_root
here, as distutils is likely to be deprecated at some point, and at that point this function may no longer be available.
@@ -243,8 +241,13 @@ def run(self, options, args): | |||
install_options = options.install_options or [] | |||
|
|||
logger.debug("Using %s", get_pip_version()) | |||
|
|||
# decide_user_install not only return the whether to use user |
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.
# decide_user_install not only return the whether to use user | |
# decide_user_install not only returns whether to use user |
@@ -243,8 +241,13 @@ def run(self, options, args): | |||
install_options = options.install_options or [] | |||
|
|||
logger.debug("Using %s", get_pip_version()) | |||
|
|||
# decide_user_install not only return the whether to use user | |||
# site-packages, but also validate compatibility of the input |
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.
# site-packages, but also validate compatibility of the input | |
# site-packages, but also validates compatibility of the input |
home=None, # type: Optional[str] | ||
root=None, # type: Optional[str] | ||
isolated=False, # type: bool | ||
prefix=None, # type: Optional[str] |
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.
These are purely style changes, not related to the functionality change in this PR. Ideally, I'd rather see them removed (or made into a separate PR) as they distract when trying to review, but I'm not going to bother too much now that I've noted and ignored them. But please keep in mind for future PRs, we avoid including pure style changes in functionality PRs.
): | ||
# type:(...) -> List[str] | ||
# type: (...) -> List[str] |
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.
Same as above.
# type: (...) -> bool | ||
return all(map(test_writable_dir, set(get_lib_location_guesses( | ||
user=user, root=root, isolated=isolated, prefix=prefix, | ||
)))) |
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.
-1 on changing from a comprehension to map()
- It makes the logic more difficult to follow. That's something of a matter of opinion, agreed.
- It obscures the difference between the old and new versions, making it harder to review.
prefix_path=None, # type: Optional[str] | ||
target_dir=None, # type: Optional[str] | ||
root_path=None, # type: Optional[str] | ||
isolated_mode=False, # type: bool | ||
): | ||
# type: (...) -> bool | ||
"""Determine whether to do a user install based on the input options. | ||
"""Determine whether to do a user 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.
This appears to be a complete rewrite of decide_user_install
- I can't follow how the logic changes from the old to the new version, the differences are too major to find any obvious points of similarity. It would be much easier to review this PR if it was possible to understand what actually changed here.
Also, given that the PR is "fail early when install path is not writeable", I don't understand why you're changing this function in any case. The changes here seem unrelated to the purpose of the PR, and if they are necessary, they should be rewritten to make the reasoning clearer.
I'm not going to review this function in any further detail, as I don't believe I can say anything meaningful.
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.
the differences are too major to find any obvious points of similarity. It would be much easier to review this PR if it was possible to understand what actually changed here.
I strongly agree. Part of the reason that I've not revisited this PR personally is that working through those changes (and the effects of them) is pretty complicated, and not something I'm comfortable doing in even a 20 minute sitting, due to how nuanced the logic around this entire area is and how drastic the changes in this PR are.
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.
+1 I totally relate to this and I normally request that submitters distill as many small and atomic changes into separate PRs as possible...
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.
@McSinyx if you were to learn just one important thing from this review it must be this ☝️
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.
Thank you all for the suggestions and sorry for requesting reviews and running away 🙈 Given I'm more experienced with submitting code for peer review now, I'll try to redo this one for it to make some sense, likely within the next few days.
@@ -887,22 +887,6 @@ def test_install_package_with_target(script, with_wheel): | |||
result.did_update(singlemodule_py) | |||
|
|||
|
|||
@pytest.mark.parametrize("target_option", ['--target', '-t']) | |||
def test_install_package_to_usersite_with_target_must_fail(script, |
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.
Why are you dropping this test? Again, it seems unrelated to the stated purpose of this PR.
@@ -1075,21 +1059,6 @@ def test_install_editable_with_prefix(script): | |||
result.did_create(install_path) | |||
|
|||
|
|||
def test_install_package_conflict_prefix_and_user(script, data): |
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.
Same comment as above. No reason to drop this.
@@ -98,7 +98,7 @@ def test_install_user_venv_nositepkgs_fails(self, virtualenv, | |||
expect_error=True, | |||
) | |||
assert ( | |||
"Can not perform a '--user' install. User site-packages are not " | |||
"Can not perform a '--user' install. User site-packages is not " |
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.
Cosmetic fix. Should be in a separate PR.
Hello! I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the |
Closing as this is significantly out of date, and has merge conflicts. Please do feel free to resubmit an updated PR for this! ^>^ |
At the same time install location compatibility is checked.
This tries to fix GH-6762:
It also handles destination conflicts, e.g.
I would also love to be given an explanation for 2e1959a: Why do we need to check for against virtualenv non-global in a user site install?
Edit: while doing manual testing, I notice that if I set
site.ENABLE_USER_SITE
using eitherpython -s
orPYTHONNOUSER
,pip
does not run the modified code at all. May I ask what causes this?