-
Notifications
You must be signed in to change notification settings - Fork 416
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
Comments
Please note, though, that we don't recommend managing |
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 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:
|
I wasn't sure about that, so I just wanted to point you towards it ;-)
I see your point and agree, since you aren't really asking for a breaking behavioural change. A PR would be welcome, I guess.
That sounds like a good option to me, since it's well-tested.
Or you could use the |
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 🤝
I would even say, I'm asking to revert a (most likly accidental) breaking behavioural change 😎
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. |
I added #1231 implemeting solution 2. See there for details about the implementation. A short reasoning about the other solutions:
|
I just stumbled upon this thread after being pinged on #1205.
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.
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.
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 |
@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 💪 |
@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
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. |
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 Bottomline: #1164 is caused by #1001 (and of cause MS Store Python path redirection of |
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. |
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. |
Sounds good for me too 👍💪 |
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 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 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. |
@guahki can you give me a exact procedure to reproduce this error? I just
with no issues. |
Hi @Gitznik, I will try to give exact details on a "minimal working example" asap, but from first reading the question is: PS: without testing and just from thinking, my workflow would be as follows:
|
That did it, thanks 👍 |
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 |
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? |
I think just changing the pipx folder (where it stores it venvs) to a place outside the sandboxed AppData would solve the problem. So „reverting“ (in the meaning of undo behavioral changes, not git revert) #1001 on windows (or switching to an even different folder) solves the issue (as on my machine with the pre-#1001 behavior).
|
Just for documentation:
@Gitznik moved the discussion to #1247, thanks! The original issue is solved by #1231, which is under review right now. |
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:
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:
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 byshutil.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.
The text was updated successfully, but these errors were encountered: