Fail early when install path is not writable #7828

McSinyx · 2020-03-07T05:23:40Z

At the same time install location compatibility is checked.

This tries to fix GH-6762:

$ pip install --target / appdirs
ERROR: Cannot write to target directory: /
$ pip install --root / appdirs
ERROR: Cannot write to global site-packages.
$ pip install --prefix / appdirs
ERROR: Cannot write to prefix path: /

It also handles destination conflicts, e.g.

$ pip install --target / --user appdirs
ERROR: Different installation locations are implied by --target and --user.
$ pip install --user --root / --prefix / appdirs
ERROR: Different installation locations are implied by --user, --root and --prefix.

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 either python -s or PYTHONNOUSER, pip does not run the modified code at all. May I ask what causes this?

src/pip/_internal/commands/install.py

tests/unit/test_command_install.py

src/pip/_internal/commands/install.py

tests/unit/test_decide_user_install.py

tfolbrecht · 2020-03-13T01:56:19Z

SICK!!!

webknjaz · 2020-03-13T18:03:26Z

tests/unit/test_decide_user_install.py

+
+@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


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.

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.

tests/unit/test_decide_user_install.py

McSinyx · 2020-05-08T15:15:56Z

To move this forward, I'm unlinking GH-7829 by exclude --root out of the mutually exclusive group, and introduce no breaking changes.

McSinyx · 2020-05-09T04:18:25Z

After the latest commit, I think this PR no longer needs design discussion. I am ready for implementation reviews!

pradyunsg · 2020-07-29T20:04:50Z

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.

pradyunsg

I'm finding it exceedingly difficult to review this PR due to the code style. Here are some preliminary comments.

src/pip/_internal/commands/install.py

pradyunsg · 2020-07-29T20:10:01Z

src/pip/_internal/commands/install.py

+        logger.debug("Non-user install since user site-packages are disabled "
+                     "for security reasons or by an administrator.")


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:

Suggested change

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."

)

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.

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]".

Does 9306254 do what you mean?

src/pip/_internal/commands/install.py

McSinyx · 2020-07-30T04:29:13Z

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.

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 <wk@sydorenko.org.ua>

The use of the function is also emphasized.

Effectively user-site also need write permission checking.

pradyunsg · 2020-07-30T10:13:40Z

it's the most unpythonic way possible to style Python code

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.

McSinyx · 2020-07-30T10:22:55Z

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

McSinyx · 2020-09-20T10:45:22Z

Maintainers of pip,
Is it a time for a revisit?

webknjaz · 2020-09-20T12:10:20Z

@pypa/pip-committers ^

pfmoore

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.

pfmoore · 2020-09-20T12:19:19Z

src/pip/_internal/commands/install.py

@@ -5,8 +5,9 @@
 import operator
 import os
 import shutil
-import site
+from distutils.util import change_root


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.

pfmoore · 2020-09-20T12:19:53Z

src/pip/_internal/commands/install.py

@@ -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


Suggested change

# decide_user_install not only return the whether to use user

# decide_user_install not only returns whether to use user

pfmoore · 2020-09-20T12:20:02Z

src/pip/_internal/commands/install.py

@@ -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


Suggested change

# site-packages, but also validate compatibility of the input

# site-packages, but also validates compatibility of the input

pfmoore · 2020-09-20T12:51:55Z

src/pip/_internal/commands/install.py

+    home=None,  # type: Optional[str]
+    root=None,  # type: Optional[str]
+    isolated=False,  # type: bool
+    prefix=None,  # type: Optional[str]


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.

pfmoore · 2020-09-20T12:52:06Z

src/pip/_internal/commands/install.py

 ):
-    # type:(...) -> List[str]
+    # type: (...) -> List[str]


Same as above.

pfmoore · 2020-09-20T12:55:14Z

src/pip/_internal/commands/install.py

+    # type: (...) -> bool
+    return all(map(test_writable_dir, set(get_lib_location_guesses(
+        user=user, root=root, isolated=isolated, prefix=prefix,
+    ))))


-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.

pfmoore · 2020-09-20T13:02:18Z

src/pip/_internal/commands/install.py

    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.


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.

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.

+1 I totally relate to this and I normally request that submitters distill as many small and atomic changes into separate PRs as possible...

@McSinyx if you were to learn just one important thing from this review it must be this ☝️

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.

pfmoore · 2020-09-20T13:03:28Z

tests/functional/test_install.py

@@ -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,


Why are you dropping this test? Again, it seems unrelated to the stated purpose of this PR.

pfmoore · 2020-09-20T13:03:45Z

tests/functional/test_install.py

@@ -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):


Same comment as above. No reason to drop this.

pfmoore · 2020-09-20T13:04:15Z

tests/functional/test_install_user.py

@@ -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 "


Cosmetic fix. Should be in a separate PR.

BrownTruck · 2021-02-04T16:01:21Z

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 master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

pradyunsg · 2021-03-05T16:41:07Z

Closing as this is significantly out of date, and has merge conflicts. Please do feel free to resubmit an updated PR for this! ^>^

McSinyx force-pushed the check-path branch 3 times, most recently from e4e0dd9 to 0f50737 Compare March 7, 2020 09:15

McSinyx marked this pull request as ready for review March 7, 2020 09:22

webknjaz reviewed Mar 7, 2020

View reviewed changes

src/pip/_internal/commands/install.py Outdated Show resolved Hide resolved

webknjaz reviewed Mar 7, 2020

View reviewed changes

src/pip/_internal/commands/install.py Outdated Show resolved Hide resolved

webknjaz reviewed Mar 7, 2020

View reviewed changes

src/pip/_internal/commands/install.py Show resolved Hide resolved

webknjaz reviewed Mar 7, 2020

View reviewed changes

src/pip/_internal/commands/install.py Outdated Show resolved Hide resolved

webknjaz reviewed Mar 7, 2020

View reviewed changes

src/pip/_internal/commands/install.py Show resolved Hide resolved

webknjaz reviewed Mar 7, 2020

View reviewed changes

tests/unit/test_command_install.py Outdated Show resolved Hide resolved

webknjaz reviewed Mar 7, 2020

View reviewed changes

tests/unit/test_command_install.py Outdated Show resolved Hide resolved

webknjaz reviewed Mar 7, 2020

View reviewed changes

tests/unit/test_command_install.py Outdated Show resolved Hide resolved

webknjaz reviewed Mar 7, 2020

View reviewed changes

tests/unit/test_command_install.py Outdated Show resolved Hide resolved

webknjaz reviewed Mar 7, 2020

View reviewed changes

src/pip/_internal/commands/install.py Outdated Show resolved Hide resolved

webknjaz reviewed Mar 7, 2020

View reviewed changes

src/pip/_internal/commands/install.py Show resolved Hide resolved

McSinyx mentioned this pull request Mar 8, 2020

Do not allow --target, --user, --root and --prefix together #7829

Open

pradyunsg added state: needs discussion This needs some more discussion type: enhancement Improvements to functionality labels Mar 8, 2020

webknjaz reviewed Mar 12, 2020

View reviewed changes

tests/unit/test_decide_user_install.py Outdated Show resolved Hide resolved

webknjaz reviewed Mar 12, 2020

View reviewed changes

tests/unit/test_decide_user_install.py Outdated Show resolved Hide resolved

webknjaz reviewed Mar 12, 2020

View reviewed changes

tests/unit/test_decide_user_install.py Outdated Show resolved Hide resolved

webknjaz reviewed Mar 13, 2020

View reviewed changes

tests/unit/test_decide_user_install.py Outdated Show resolved Hide resolved

webknjaz reviewed Mar 13, 2020

View reviewed changes

tests/unit/test_decide_user_install.py Outdated Show resolved Hide resolved

tfolbrecht mentioned this pull request Apr 14, 2020

Check install path #6801

Closed

McSinyx mentioned this pull request May 19, 2020

feat(): Add logs for pip environment when installing #8266

Merged

pradyunsg added the state: needs eyes Needs a maintainer/triager to take a closer look label May 31, 2020

BrownTruck added the needs rebase or merge PR has conflicts with current master label Jul 18, 2020

McSinyx force-pushed the check-path branch 2 times, most recently from b928ef1 to ad17477 Compare July 18, 2020 13:40

pypa-bot removed the needs rebase or merge PR has conflicts with current master label Jul 18, 2020

pradyunsg mentioned this pull request Jul 29, 2020

Blacken changed function McSinyx/pip#2

Closed

pradyunsg reviewed Jul 29, 2020

View reviewed changes

McSinyx and others added 6 commits July 30, 2020 12:00

Fail early when install path is not writable

3b70f49

At the same time install location compatibility is checked.

Add unit tests for user install decision

f6dce7e

Functional tests for conflict install locations are removed in favor of the unittest which handle more cases.

Drop redundant explicit bool checks and clarify site.ENABLE_USER_SITE

57d1305

Co-authored-by: Sviatoslav Sydorenko <wk@sydorenko.org.ua>

Move decide_user_install to a separate unit test

33eb545

The use of the function is also emphasized.

Allow --root to be used with other install dest options

a7cd38b

Effectively user-site also need write permission checking.

Fix commands.install.site_packages_writable signature

e5ddcc7

McSinyx force-pushed the check-path branch from ad17477 to 669f2e8 Compare July 30, 2020 05:01

Blacken decide_user_install and clarify log output

90a1357

McSinyx force-pushed the check-path branch from 669f2e8 to 90a1357 Compare July 30, 2020 09:33

Improve log/error messages esp. ones involve ENABLE_USER_SITE

d7472a8

This is to be squashed with the commit above

McSinyx force-pushed the check-path branch from 9306254 to d7472a8 Compare August 3, 2020 17:15

McSinyx mentioned this pull request Sep 8, 2020

Prepare for the future of distutils-in-setuptools #8851

Closed

pfmoore requested changes Sep 20, 2020

View reviewed changes

BrownTruck added the needs rebase or merge PR has conflicts with current master label Feb 4, 2021

pradyunsg closed this Mar 5, 2021

github-actions bot locked as resolved and limited conversation to collaborators Oct 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fail early when install path is not writable #7828

Fail early when install path is not writable #7828

McSinyx commented Mar 7, 2020 •

edited

tfolbrecht commented Mar 13, 2020

webknjaz Mar 13, 2020

McSinyx Mar 14, 2020

McSinyx commented May 8, 2020 •

edited

McSinyx commented May 9, 2020

pradyunsg commented Jul 29, 2020

pradyunsg left a comment

pradyunsg Jul 29, 2020

McSinyx Jul 30, 2020

pradyunsg Jul 30, 2020

McSinyx Jul 30, 2020

McSinyx commented Jul 30, 2020

pradyunsg commented Jul 30, 2020

McSinyx commented Jul 30, 2020

McSinyx commented Sep 20, 2020

webknjaz commented Sep 20, 2020

pfmoore left a comment

pfmoore Sep 20, 2020

pfmoore Sep 20, 2020

pfmoore Sep 20, 2020

pfmoore Sep 20, 2020

pfmoore Sep 20, 2020

pfmoore Sep 20, 2020

pfmoore Sep 20, 2020

pradyunsg Sep 20, 2020

webknjaz Sep 21, 2020

webknjaz Sep 21, 2020

McSinyx Sep 21, 2020

pfmoore Sep 20, 2020

pfmoore Sep 20, 2020

pfmoore Sep 20, 2020

BrownTruck commented Feb 4, 2021

pradyunsg commented Mar 5, 2021

		logger.debug("Non-user install since user site-packages are disabled "
		"for security reasons or by an administrator.")

	# decide_user_install not only return the whether to use user
	# decide_user_install not only returns whether to use user

	# site-packages, but also validate compatibility of the input
	# site-packages, but also validates compatibility of the input

		@@ -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):

Fail early when install path is not writable #7828

Fail early when install path is not writable #7828

Conversation

McSinyx commented Mar 7, 2020 • edited

tfolbrecht commented Mar 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

McSinyx commented May 8, 2020 • edited

McSinyx commented May 9, 2020

pradyunsg commented Jul 29, 2020

pradyunsg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

McSinyx commented Jul 30, 2020

pradyunsg commented Jul 30, 2020

McSinyx commented Jul 30, 2020

McSinyx commented Sep 20, 2020

webknjaz commented Sep 20, 2020

pfmoore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrownTruck commented Feb 4, 2021

pradyunsg commented Mar 5, 2021

McSinyx commented Mar 7, 2020 •

edited

McSinyx commented May 8, 2020 •

edited