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

Pipx managing itself is broken on windows #1203

Closed
guahki opened this issue Jan 12, 2024 · 20 comments · Fixed by #1231
Closed

Pipx managing itself is broken on windows #1203

guahki opened this issue Jan 12, 2024 · 20 comments · Fixed by #1231
Labels
bug Something isn't working windows

Comments

@guahki
Copy link
Contributor

guahki commented Jan 12, 2024

Describe the bug

When pipx manages itself (pipx is installed as a pipx app) on Windows, it is no longer able to uninstall itself (since #1168 was merged).

The following error message is shown:

Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File "C:\Users\not_of_interest\.local\bin\pipx.exe\__main__.py", line 7, in <module>
  File "C:\Users\not_of_interest\.local\pipx\venvs\pipx\Lib\site-packages\pipx\main.py", line 914, in cli
  File "C:\Users\not_of_interest\.local\pipx\venvs\pipx\Lib\site-packages\pipx\main.py", line 274, in run_pipx_command
  File "C:\Users\not_of_interest\.local\pipx\venvs\pipx\Lib\site-packages\pipx\commands\reinstall.py", line 53, in reinstall
  File "C:\Users\not_of_interest\.local\pipx\venvs\pipx\Lib\site-packages\pipx\commands\uninstall.py", line 151, in uninstall
  File "C:\Users\not_of_interest\.local\pipx\venvs\pipx\Lib\site-packages\pipx\util.py", line 57, in rmdir
  File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.12_3.12.496.0_x64__qbz5n2kfra8p0\Lib\shutil.py", line 808, in rmtree
    return _rmtree_unsafe(path, onexc)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.12_3.12.496.0_x64__qbz5n2kfra8p0\Lib\shutil.py", line 631, in _rmtree_unsafe
    _rmtree_unsafe(fullname, onexc)
  File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.12_3.12.496.0_x64__qbz5n2kfra8p0\Lib\shutil.py", line 636, in _rmtree_unsafe
    onexc(os.unlink, fullname, err)
  File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.12_3.12.496.0_x64__qbz5n2kfra8p0\Lib\shutil.py", line 634, in _rmtree_unsafe
    os.unlink(fullname)
PermissionError: [WinError 5] Access denied: 'C:\\Users\\not_of_interest\\.local\\pipx\\venvs\\pipx\\Scripts\\python.exe'

The venv folder is not deleted, as the Scripts\python.exe is still inside. This also causes following installs to break (one has to delete the remaining folder manually).

How to reproduce

On Windows:

  1. Install pipx in pipx
  2. pipx uninstall pipx

Expected behavior

The locked (as in use) python.exe should be moved to the trash, as introduced in line 64-76 of 6e1907b#diff-c69ab827b9b12c8c732a2cfaf1056304894e775d94224f94e56313bf1f7d299f.

Analysis

In #1168, the os.system(f'rmdir /S /Q "{str(path)}"') to delete the folder was replaced by shutil.rmtree(path). The first one printed an error to the stdout it still fell through to the code introduced by #718 linked in the "Expected behavior" section, which then moved the undeletable executable and thus resolved the issue. The latter one (now) just fails with the stated error.

If any of the maintainers give feedback, how to solve it (restore the os.system-call?), I'm happy to provide a PR (shouldn't be too hard 😆).

Personal note

Tbh #1168 is a huge disaster imho. Both (little) changes introduced had bad impact on me. One is discribed here, for the other change see my comment #1186 (review). I do not want to blame anyone and hugely appreciate the time of everyone involved here, but this PR really did cost me some sanity now. And as the issue #1195 arising from it shows, I'm not the only one.

Thank you all for a great tool, which I really don't want to miss.

@chrysle
Copy link
Contributor

chrysle commented Jan 13, 2024

Please note, though, that we don't recommend managing pipx via itself (also documented on the front page). That's what the pipx-in-pipx project is for. Maybe they can work around that issue there.

@guahki
Copy link
Contributor Author

guahki commented Jan 13, 2024

I know, that it's not recommend to let pipx manage itself and I'm very aware of the "Sharp edges" mentioned in the pipx-in-pipx project (https://github.com/mattsb42-meta/pipx-in-pipx#sharp-edges). However, pipx-in-pipx is more or less unattended since 2-4 years (depending of what measure you use) and does not really solve any issues or problems, it was just a convenient way of bootstrapping pipx into pipx. And tbh: I would count most of the sharp edges as benefits and some of them just don't hold true (at least on windows in my experience, e.g. "If you uninstall your pipx-managed pipx (pipx uninstall pipx), all of the tools that you installed using that pipx will stop working because their Pythons suddenly point to nothing.").

However, huge efforts were taken, to make it possible (I understood "theoretically support for those who want") to manage pipx with itself under windows. E.g. the already mentioned #718. I really liked this back then and it was the key for adopting pipx for me. So I won't resist a "it was never recommended", but I want to resist a "it was never supported". Since if pipx would really drop support for managing itself (on Windows), I would be very sad.

For me the bottomline stays: a person had a issue with the Microsoft-Store Python and pipx (#1164; I can not really relate to the issue, as I use Microsoft-Store Python for some time now and never had problems with pipx). He tried to fix that and got it working for his specific usecase. He opened #1168 with the fix and broke the --python flag on Linux (#1195), on Windows (#1205) and (just as a sideeffect) the self-managing capability of pipx on Windows.

As I said, I don't want to blame anyone and I would even agree, that my usecases might be similarly edge-cases as his were. But breaking working use-cases, which were not recommended, but deliberatly made possible, seems a bad choice from my point of perspective. And as I also said: I'm happy to make another PR fixing this, I somebody tells me in which direction this should be going. I see (at least) the following options:

  1. Revert the deleting of the folder to use the os.system call on windows
  2. Ignore errors in shutil.rmtree using ignore_errors=True (I didn't test this yet, but it should work imho)
  3. Catch the specific PermissionError and ignore it (I didn't test this yet, but it should work imho)

@chrysle
Copy link
Contributor

chrysle commented Jan 13, 2024

I know, that it's not recommend to let pipx manage itself and I'm very aware of the "Sharp edges" mentioned in the pipx-in-pipx project (https://github.com/mattsb42-meta/pipx-in-pipx#sharp-edges).

I wasn't sure about that, so I just wanted to point you towards it ;-)

But breaking working use-cases, which were not recommended, but deliberatly made possible, seems a bad choice from my point of perspective.

I see your point and agree, since you aren't really asking for a breaking behavioural change. A PR would be welcome, I guess.

  1. Revert the deleting of the folder to use the os.system call on windows

That sounds like a good option to me, since it's well-tested.

  1. Catch the specific PermissionError and ignore it (I didn't test this yet, but it should work imho)

Or you could use the onerorr argument to shutil.rmtree()?

@guahki
Copy link
Contributor Author

guahki commented Jan 14, 2024

I know, that it's not recommend to let pipx manage itself and I'm very aware of the "Sharp edges" mentioned in the pipx-in-pipx project (https://github.com/mattsb42-meta/pipx-in-pipx#sharp-edges).

I wasn't sure about that, so I just wanted to point you towards it ;-)

All good. I'm sorry for my rant, but after spending some time investigating this and building a good knowledge of the issue just being pointed to "this is not recommended" was a little unsatisfying. Lets shake hands 🤝

But breaking working use-cases, which were not recommended, but deliberatly made possible, seems a bad choice from my point of perspective.

I see your point and agree, since you aren't really asking for a breaking behavioural change. A PR would be welcome, I guess.

I would even say, I'm asking to revert a (most likly accidental) breaking behavioural change 😎

  1. Revert the deleting of the folder to use the os.system call on windows

That sounds like a good option to me, since it's well-tested.

  1. Catch the specific PermissionError and ignore it (I didn't test this yet, but it should work imho)

Or you could use the onerorr argument to shutil.rmtree()?

I will have a look as soon as I have some time to do so and at least test the different ideas and report back and/or open a PR.

@guahki
Copy link
Contributor Author

guahki commented Jan 25, 2024

I added #1231 implemeting solution 2. See there for details about the implementation.

A short reasoning about the other solutions:

  1. this is (as was mentioned) well-tested, but it is an unnecessary system call. Solution 2 should do exactly the same.
  2. this was implemented.
  3. this (or the use of onerror, which was suggested) imho does not improve on solution 2. One would have (most likely "complex") checks about the error thrown or the path and only reject some exceptions and reraise others, which does not seem to improve the situation imho. If any error occurs, either the directory is removed anyways or the remainings would be moved to the trash and deleted next time around. If any kind of permission error or something like this happens, it is most likely raised when trying to move afterwards.

@dechamps
Copy link
Contributor

dechamps commented Jan 28, 2024

I just stumbled upon this thread after being pinged on #1205.

For me the bottomline stays: a person had a issue with the Microsoft-Store Python and pipx (#1164; I can not really relate to the issue, as I use Microsoft-Store Python for some time now and never had problems with pipx).

To be clear, I still believe #1164 was quite severe and did warrant a fix. At the time I was able to reproduce it on four separate Windows machines including a fresh Windows 11 install in an empty VM. Like, literally: create a VM, install latest Windows 11 from official ISO, install latest Python from Microsoft Store, install pipx, pipx doesn't work. To me that qualifies as "software is completely broken" in this particular software configuration that a substantial number of Windows users are likely to use.

I'm quite surprised you didn't hit that problem if you are using MS Store Python (again, I was able to reproduce in an empty fresh install VM). I'm now wondering if this might be dependent on Windows version, or perhaps MS Store Python version. Or perhaps you're using some kind of non-default config that affects how MS Store package path redirection works.

He tried to fix that and got it working for his specific usecase. He opened #1168 with the fix and broke the --python flag on Linux (#1195), on Windows (#1205) and (just as a sideeffect) the self-managing capability of pipx on Windows.

Apologies for the trouble I caused. I'll admit I dropped the ball when I wrote #1168 as I was not aware of all the various ways these code paths were used in the wild (e.g. I did not expect relative paths would make it to these functions). I was also hoping that if there were subtleties there then they would either be caught by the (seemingly extensive, but apparently not extensive enough) test suite, or by reviewers on the PR. Looks like all these defensive layers failed, and I'm sorry about that. Hopefully the additional tests being added in the aftermath of this trainwreck will make mistakes like these less likely in the future.

In #1168, the os.system(f'rmdir /S /Q "{str(path)}"') to delete the folder was replaced by shutil.rmtree(path).

Yeah. Interestingly, the original version of #1168 did not include this change because I deemed it out of scope for the PR - I only made that change following this discussion with @gaborbernat. I did try to understand why this code shelled out to rmdir (which seemed odd to me), but couldn't find any information in comments, docs nor code history. If this code needs to be fixed again, then I would strongly advise adding a detailed code comment that clearly explains to the reader the subtleties around this code and why it can't just be simplified to shutil.rmtree().

@Gitznik
Copy link
Contributor

Gitznik commented Jan 28, 2024

@dechamps while this change caused issues, as you said there is an extensive test suite, there were reviews and the workings of python on windows are hard to test and somewhat unpredictable IMO. No reason to beat yourself up about it.

Thanks for staying active on these issues and helping us find a version that works for everyone 💪

@guahki
Copy link
Contributor Author

guahki commented Jan 28, 2024

@dechamps Even without the time to review anything in detail right now, I want to immediately make sure to reiterate: I do not want to blame you for anything! Of course, your changes broke some use-cases for me and the debugging did cost (and costs) me some time. But

  1. As you (and @Gitznik, thanks for that!) say: you are not making decisions alone, the whole system (contributor, reviewer, unit-test, ...) did not work to it's best here
  2. my use-cases are partially edge-case (especially the use-case discussed in this issue)
  3. It's the risk and beauty of OSS: the risk is, somebody might break it. The beauty is, we can join and together come to a solution which hopefully is better than anything before. I'm very thankful for your humble and concerned reply and hope very much, you did not take anything I said personal (especially not the passage you quoted).

So, overall, let's just work on finding the best solution.

All of that said, I will review all of the comments from different sides asap, but it might take some time, as real life (unfortunately) exists.

@guahki
Copy link
Contributor Author

guahki commented Jan 28, 2024

For me the bottomline stays: a person had a issue with the Microsoft-Store Python and pipx (#1164; I can not really relate to the issue, as I use Microsoft-Store Python for some time now and never had problems with pipx).

To be clear, I still believe #1164 was quite severe and did warrant a fix. At the time I was able to reproduce it on four separate Windows machines including a fresh Windows 11 install in an empty VM. Like, literally: create a VM, install latest Windows 11 from official ISO, install latest Python from Microsoft Store, install pipx, pipx doesn't work. To me that qualifies as "software is completely broken" in this particular software configuration that a substantial number of Windows users are likely to use.

I'm quite surprised you didn't hit that problem if you are using MS Store Python (again, I was able to reproduce in an empty fresh install VM). I'm now wondering if this might be dependent on Windows version, or perhaps MS Store Python version. Or perhaps you're using some kind of non-default config that affects how MS Store package path redirection works.

As I was very interested in why I never had this issue (and believe me, I'm well aware of the issues MS Store Python can cause due to these strange path redirection stuff), I at least looked into that quickly and found the reason (after close review of the details in your issue it was quite obvious):

In the change log of version 1.3.0 (https://github.com/pypa/pipx/releases/tag/1.3.0) we find the line "Move pipx paths to ensure compatibility with the platform-specific user directories", which corresponds to #1001. The documentation change in https://github.com/pypa/pipx/pull/1001/files#diff-8b042e3f94ca5c59a7cd990b950aec0073ea84fe811e7b22be51158d7b180d56 exactly states, why I could (at first) not relate or reproduce you issue: as I use pipx since years, I obviously have my venvs in ~\.local\pipx and thus, pipx still uses this folder. A new installation (or if I rename my ~\.local\pipx folder) uses some location in ~\AppData\Local, which is prone to path redirection, and I can easily reproduce #1164 then.

Bottomline: #1164 is caused by #1001 (and of cause MS Store Python path redirection of AppData\Local), but the changes of #1001 were not "active" in my setup (due to the fallback to the old folder, if it exists).

@dechamps
Copy link
Contributor

I see. I guess one option would be to look into (partially) reverting #1001 then, although I'm not sure that would make sense as I think the new location where pipx puts venvs today makes sense in MS Store Python: everything is under the Python package so nicely isolated/sandboxed, and it's consistent with where pip installs stuff as well. I suspect it would make more sense to try to make it work with the new locations, as I tried to do in #1164.

@guahki
Copy link
Contributor Author

guahki commented Jan 28, 2024

Yes, I think we should just make #1168 work. With this additional understanding we can easily both stand by our core statements:

#1164 was "quite severe and did warrant a fix" and the state before "qualifies as "software is completely broken"" (to cite your words).

Nevertheless, #1168 as fix for this issue still "broke the --python flag on Linux (#1195), on Windows (#1205) and (just as a sideeffect) the self-managing capability of pipx on Windows" (to cite myself), and thus maybe was not a really good fix.

So, as discussed: I will also do some more investigation, but at first glance the idea presented by @Gitznik in #1232 (review) looks promising to me and might be the best way forward.

@gaborbernat
Copy link
Contributor

Sounds good for me too 👍💪

@guahki
Copy link
Contributor Author

guahki commented Feb 2, 2024

I reviewed and reiterated #1231 (the PR fixing this issue) and #1232 (a closely related PR) and during this, I discovered a new way to look at

I guess one option would be to look into (partially) reverting #1001 then, although I'm not sure that would make sense as I think the new location where pipx puts venvs today makes sense in MS Store Python: everything is under the Python package so nicely isolated/sandboxed, and it's consistent with where pip installs stuff as well.

I realized, that the path redirection done by the MS Store Python (together with pipx storing it's venvs in the redirected local app data) in the end means, that a MS Store Python user can not manage packages installed with different python versions in pipx. This is due to the python version used when creating a venv might create it in a different "sandboxed" path than the python version running pipx, so pipx can never see the created venv, nor execute scripts/entrypoints in it.

With this new angle, I would like the core devs to rethink #1001 (at least on windows). As this discussion might further leave the original issue reported here (while still kind of related), I'm happy to open this a new issue, if you want me to.

@Gitznik
Copy link
Contributor

Gitznik commented Feb 4, 2024

@guahki can you give me a exact procedure to reproduce this error? I just

  1. setup a windows 10 VM
  2. installed python3.12 from the store
  3. Installed pipx via pip
  4. installed pipx via pipx
  5. installed pycowsay via pipx
  6. uninstalled pipx via pipx

with no issues.

@guahki
Copy link
Contributor Author

guahki commented Feb 4, 2024

Hi @Gitznik,

I will try to give exact details on a "minimal working example" asap, but from first reading the question is:
which pipx is used in step 6. You have pipx installed via pip and pipx installed in pipx, and the question is which is first in the PATH. For the issue to occur, you have to make sure to delete pipx "with itself", i.e. with the pipx.exe installed through step 4 (not step 3). Either by executing the pipx.exe by full path or by uninstalling the pipx installed by pip in step 3 (pip uninstall pipx).

PS: without testing and just from thinking, my workflow would be as follows:

  1. setup VM and install MS Store Python (I use 3.12, but version should be not really important)
  2. create a venv and activate it
  3. install pipx inside venv
  4. pipx install pipx (with pipx inside venv)
  5. deactivate venv (important, to use the pipx installed in step 4, not the one installed in step 3) (irl I would delete the venv after "bootstrapping", but for testing it is very convenient to be able to activate and deactivate it)
  6. pipx uninstall pipx should cause the error, as it uses the "correct" pipx (installed by pipx) to cause the error

@Gitznik
Copy link
Contributor

Gitznik commented Feb 4, 2024

  1. setup VM and install MS Store Python (I use 3.12, but version should be not really important)
  2. create a venv and activate it
  3. install pipx inside venv
  4. pipx install pipx (with pipx inside venv)
  5. deactivate venv (important, to use the pipx installed in step 4, not the one installed in step 3) (irl I would delete the venv after "bootstrapping", but for testing it is very convenient to be able to activate and deactivate it)
  6. pipx uninstall pipx should cause the error, as it uses the "correct" pipx (installed by pipx) to cause the error

That did it, thanks 👍

@dechamps
Copy link
Contributor

dechamps commented Feb 4, 2024

the path redirection done by the MS Store Python (together with pipx storing it's venvs in the redirected local app data) in the end means, that a MS Store Python user can not manage packages installed with different python versions in pipx.

Correct. Every major Python version is packaged as a different MS Store app, and therefore uses separate and independent sandboxes.

I am really not sure it would be a good idea to try to make pipx work around that, because pip is also subject to this behavior. Making pipx behave differently from pip seems like it may cause more problems/confusion than it would solve.

@Gitznik
Copy link
Contributor

Gitznik commented Feb 4, 2024

I am really not sure it would be a good idea to try to make pipx work around that, because pip is also subject to this behavior. Making pipx behave differently from pip seems like it may cause more problems/confusion than it would solve.

The alternative being documenting this issue and recommending not installing python from the MS Store if using different python versions is a relevant use case to the user?

@guahki
Copy link
Contributor Author

guahki commented Feb 4, 2024 via email

@guahki
Copy link
Contributor Author

guahki commented Feb 10, 2024

Just for documentation:

With this new angle, I would like the core devs to rethink #1001 (at least on windows). As this discussion might further leave the original issue reported here (while still kind of related), I'm happy to open this a new issue, if you want me to.

@Gitznik moved the discussion to #1247, thanks!

The original issue is solved by #1231, which is under review right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working windows
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants