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

Fix "Permission denied"/"Access refused" error when installing or updating packages (solves #9527, #9566, #9395) #10454

Closed
wants to merge 3 commits into from

Conversation

baterflyrity
Copy link

Add check for write permissions for non user installs.

Add check for write permissions for non user installs.
@baterflyrity baterflyrity mentioned this pull request Sep 9, 2021
@DiddiLeija
Copy link
Member

Hi @baterflyrity, before going ahead, I recommend you to create a news file for your pull request (in this case, it should be news/10454.bugfix.rst, I supose). See the reference here:

https://pip.pypa.io/en/latest/development/contributing/#news-entries

Clear user site installation error message.
Describe bug fix.
@baterflyrity
Copy link
Author

@DiddiLeija , done.

@baterflyrity baterflyrity changed the title Solve #9527 Solve #9527, #9566 Sep 10, 2021
@baterflyrity baterflyrity mentioned this pull request Sep 10, 2021
1 task
@baterflyrity baterflyrity changed the title Solve #9527, #9566 Fix "Permission denied"/"Access refused" error when installing or updating packages (solves #9527, #9566) Sep 10, 2021
@baterflyrity baterflyrity changed the title Fix "Permission denied"/"Access refused" error when installing or updating packages (solves #9527, #9566) Fix "Permission denied"/"Access refused" error when installing or updating packages (solves #9527, #9566, ##9395) Sep 10, 2021
@baterflyrity baterflyrity changed the title Fix "Permission denied"/"Access refused" error when installing or updating packages (solves #9527, #9566, ##9395) Fix "Permission denied"/"Access refused" error when installing or updating packages (solves #9527, #9566, #9395) Sep 10, 2021
@baterflyrity
Copy link
Author

Also notice that this fix just prevents destroying of packages instead of installing them. To install write protected packages one is considered to use sudo/gsudo/administrator shell.

@pfmoore
Copy link
Member

pfmoore commented Sep 10, 2021

From my reading of this PR, it handles one specific case of permission problems, where

  1. User installs are disabled.
  2. System installs require elevated permissions.
  3. The user is not running with those elevated permissions.

This comes down to improving the error when pip has no actual way of performing an install. That's a worthy improvement, but it's not at all clear to me that it's the root of all the linked issues.

Can you please:

  1. Update the news item to be clearer about precisely what is changing. (I'd be fine with something like "Improve the error which occurs when pip does not have permission to install in any available location").
  2. Add a test that verifies the new behaviour (a unit test that patches site_packages_writable to simulate a read-only site location would be sufficient here, IMO).
  3. Check whether the linked issues actually are encountering this specific situation (I suspect it's unlikely - disabling user installs is pretty unusual for a start, so not likely to be a common problem) and if not, remove the claims from this PR that it fixes them, so we don't end up closing issues that aren't actually fixed when we merge this PR. If we're not sure this fixes an issue, assume it doesn't - it's much easier to just drop a note in the issue saying "we've just merged a change in this area, can you check if it fixes your issue" than to find out that the issue wasn't fixed after the fact and have to reopen it.

@pfmoore
Copy link
Member

pfmoore commented Sep 10, 2021

Also notice that this fix just prevents destroying of packages instead of installing them.

You keep stating this, but I don't see why it would do so. All this PR does is check if site-packages is writeable before falling back to doing a site install. But if pip tries to install to something that's not writeable, it will just fail, not "destroy packages".

The reality here is, I think, that

  1. The site_packages_writable check isn't catching every form of "not being writeable". This is definitely true, it's only intended as a broad check (for example it doesn't handle race conditions where the permissions get changed while pip is running). That's fine as long as we consider (3) below.
  2. People are ignoring the advice to upgrade pip using python -m pip. That's an unavoidable problem we know we can't fix, so we document a safe approach. I'm not worried about this, we can point out to people that they need to follow the docs.
  3. Pip's logic to correctly roll back in cases where an error happens is not working properly. This is an entirely valid issue, and we should fix it if it's happening. But we need a demonstrated (reproducible) example of this in order to identify the issue and develop a fix. At the moment, we have no such reproducible examples, just rather too much emotion and a lot of rhetoric.

If you have a reproducible example of pip "destroying packages", please provide instructions so that the pip maintainers can reproduce the problem. Just saying "upgrade pip and it fails, see my output here" is no help. I upgrade pip using itself regularly, and it always works. So there's something else involved, and you haven't yet told me what...

@baterflyrity
Copy link
Author

baterflyrity commented Sep 10, 2021

@pfmoore , just to be clear, real problem is here:

    # If user installs are not enabled, choose a non-user install
    if not site.ENABLE_USER_SITE:
        logger.debug("Non-user install because user site-packages disabled")
        return False # ←←←←←←←← no check of write access at all

Hence we want to e.g. upgrade global package pylint (pip install --upgrade pylint) → no user flag is provided → no check is performed (we just hope we have access) → we delete (write != delete, wtf???) old package version → actually we don't have access → we cant install any version while the current is lost.

Thus i added this check here plus do small refactoring to reduce code amount (combined two cases). The old logic was:

                                                               [no] → Install globally. → Opps...we destroy package.
Use has not defined site/prefix/target. → Is user site enabled? ↨                                     [no] → Install locally.
                                                               [yes] → Do we have global write access? ↨
                                                                                                     [yes] → Install globally.

and the new one is:

                                                                        [yes] → Install globally.                                                           
Use has not defined site/prefix/target. → Do we have global write access? ↨                        [no] → Raise error, don't destroy package.
                                                                        [no] → Is user site enabled? ↨ 
                                                                                                   [yes] → Install locally.

p.s.
Is mermaid enabled here?)

People are ignoring the advice to upgrade pip using python -m pip.

I suggest disabling self-upgrade if you so hate it. As for me i always upgrade with sudo pip install --upgrade pip and it always works.

Just saying "upgrade pip and it fails, see my output here" is no help.

Can't understand why it is so hard to reproduce pip install --upgrade pip in default Windows environment with default non-administrator permissions. What do u mean under reproducable? Docker container?

If we're not sure this fixes an issue, assume it doesn't - it's much easier to just drop a note in the issue saying "we've just merged a change in this area, can you check if it fixes your issue" than to find out that the issue wasn't fixed after the fact and have to reopen it.

I'm sure but you as maintainer/contributor/manager/e.t.c can decide whether to close or note linked issues. It does not depend on me - need to ask others and perform tests. Ive already dropped notes there. Despite i provide this PR i wont perform any tests for side issues because this problem is several years old and no activity from anyone else has been done.

@pfmoore
Copy link
Member

pfmoore commented Sep 10, 2021

no user flag is provided

That's not what the check if not site.ENABLE_USER_SITE does. Are you sure you understand what your PR is actually doing here?

Is mermaid enabled here?

That's a github question, I don't think they support that.

I suggest disabling self-upgrade if you so hate it.

I don't. I use it (as it's documented) all the time.

As for me i always upgrade with sudo pip install --upgrade pip and it always works.

sudo isn't a Windows command, and every Unix distro strongly discourages using sudo pip. Use it if you want, but no-one will support you.

Can't understand why it is so hard to reproduce pip install --upgrade pip

That command isn't supported, and the documented py -m pip install --upgrade pip works fine for me (in either a venv or a default python.org install). It's hard to reproduce because you give no details of how to set things up to reproduce the issue.

We're getting nowhere here, you are just repeating the same statements without adding anything new, I'm afraid :-(

What do u mean under reproducable?

Tell me exactly what I need to do in order to step by step generate the error you are seeing.

Docker container?

🤷 If that's the easiest way for you to give me precise instructions, then sure. But it's not necessary - all that's necessary is you tell me what you do. So far all you've said is pip install --upgrade pip and I've told you "that's not documented to work". And indeed I've tried it and it doesn't work for me (on Windows) but that's not a bug if we've said it doesn't work and you shouldn't use it. And even so, all it does (again for me) is fail to replace the pip.exe wrapper (with a new version likely to be exactly the same in most cases) - it certainly doesn't "destroy pip". So it's not even a massive problem in practice.

Despite i provide this PR i wont perform any tests for side issues because this problem is several years old and no activity from anyone else has been done.

OK, that's fine. Just remove the claims that this PR fixes issues that you haven't confirmed it fixes, to avoid confusing people.

@pradyunsg
Copy link
Member

Is pip install --upgrade pip not straight up blocking running on Windows, with an error message saying "use py -m pip install --upgrade pip?

@pradyunsg
Copy link
Member

Or is this not on Windows?

@pfmoore
Copy link
Member

pfmoore commented Sep 10, 2021

It's not at all clear to me what system this is on (the OP quoted sudo at one point). Other posts by @baterflyrity have output quoted that seems to suggest that maybe they are using a mingw64 or msys2 environment, which would be another possibility for something different - the mingw64/msys2 builds of Python are customised and not supported by python.org, so if the problem only occurs with those distributions, it needs to be raised with that project.

@baterflyrity these are the sorts of questions you need to address in providing a reproducible set of instructions for demonstrating the issue.

@baterflyrity
Copy link
Author

@pfmoore ,

no user flag is provided

That's not what the check if not site.ENABLE_USER_SITE does.

    # ...
    # If we are here, user installs have not been explicitly requested/avoided
    assert use_user_site is None
    # user install incompatible with --prefix/--target
    if prefix_path or target_dir:
        logger.debug("Non-user install due to --prefix or --target option")
        return False
    # here comes my patch...

@baterflyrity
Copy link
Author

baterflyrity commented Sep 10, 2021

@pfmoore , @pradyunsg , windows 10. It's special program called gsudo to grant admin access in the current session. Simple python3.9 installation with the official installer.

@pfmoore
Copy link
Member

pfmoore commented Sep 10, 2021

That's not what the check if not site.ENABLE_USER_SITE does.

See here.

That check says "if the Python interpreter does not have the user site location enabled". That's almost never the case in real systems, and nothing in any of the examples you've given suggests you're configuring things that way.

Moving a block of code that will never be executed in a particular situation will make no difference. Your PR only changes pip's behaviour if PYTHONNOUSERSITE is set (or similar mechanisms to disable the user site location). The more I review this, the less I see how this PR addresses any of the problems being discussed 🙁

@chrullrich
Copy link

pfmoore wrote:

So far all you've said is pip install --upgrade pip and I've told you "that's not documented to work". And indeed I've tried it and it doesn't work for me (on Windows) but that's not a bug if we've said it doesn't work and you shouldn't use it. And even so, all it does (again for me) is fail to replace the pip.exe wrapper (with a new version likely to be exactly the same in most cases) - it certainly doesn't "destroy pip".

Please note that in the case of my #9395 (venv on different volume from %TEMP%) it does indeed "destroy pip".

@pfmoore
Copy link
Member

pfmoore commented Sep 10, 2021

Please note that in the case of my #9395 (venv on different volume from %TEMP%) it does indeed "destroy pip".

Yes, that's a different situation (and thanks for your clear explanation over there of what the issue is that you're encountering).

@baterflyrity
Copy link
Author

@pfmoore well... i disagree with you that this is useless PR as far as it corrects logical error (see logical scheme above) anyway. So lets firstly review this as "add global installation write access check".

Oppositely, i think relatively to #9527, #9566, #9395 i need to go deeper, open IDE, write some tests and even use debugger) But it need much more time then editing text in browser.

Also tell me please where can i find pip's logs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants