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

Make uninstall on Windows not erroneously uninstall apps from dependencies #672

Merged
merged 32 commits into from
Apr 27, 2021

Conversation

itsayellow
Copy link
Contributor

@itsayellow itsayellow commented Apr 17, 2021

  • I have added an entry to docs/changelog.md

Summary of changes

Closes #670 .

This fixes a bug on Windows where uninstalling a venv would uninstall app binaries in the local bin directory from its dependencies also (even though the original pipx install did not use --include-dependencies.)

This change updates and refactors the code in uninstall.py to use the function get_exposed_app_paths_for_package from common.py to determine the associated binaries in the local binary directory for a given venv. It thus benefits from the bug fixes that were applied in #650 to common.py.

And as a bonus we get rid of duplicate code in uninstall.py that did the same function as code in common.py.

I also added tests to check that pipx uninstall doesn't delete the apps of a dependency if the original app wasn't installed with --include-dependencies.

Also in the test code:

  • I added a helper function remove_venv_interpreter
  • updated helper functionmock_legacy_venv to allow metadata_version argument to use the current metadata version
  • removed the fixture capsys in test_uninstall.py from tests where it was not being used.

Test plan

Tested by running on WINDOWS (from example in #670)

pipx install coveo-stew
pipx install poetry
pipx uninstall coveo-stew

This PR result:

> pipx install coveo-stew
done!
creating virtual environment...
installing coveo-stew...
  installed package coveo-stew 1.2.3, Python 3.9.1
  These apps are now globally available
    - pyproject.exe
    - stew.exe
> ls ~/.local/bin
pyproject.exe*  stew.exe*
> pipx install poetry
done!
creating virtual environment...
installing poetry...
  installed package poetry 1.1.6, Python 3.9.1
  These apps are now globally available
    - poetry.exe
> ls ~/.local/bin
poetry.exe*  pyproject.exe*  stew.exe*
> pipx uninstall coveo-stew
uninstalled coveo-stew!
> ls ~/.local/bin
poetry.exe*
>

pipx 0.16.1 result: (poetry.exe is erroneously removed from ~/.local/bin)

> pipx install coveo-stew
done!
creating virtual environment...
installing coveo-stew...
  installed package coveo-stew 1.2.3, Python 3.9.1
  These apps are now globally available
    - pyproject.exe
    - stew.exe
> ls ~/.local/bin
pyproject.exe*  stew.exe*
> pipx install poetry
done!
creating virtual environment...
installing poetry...
  installed package poetry 1.1.6, Python 3.9.1
  These apps are now globally available
    - poetry.exe
> ls ~/.local/bin
poetry.exe*  pyproject.exe*  stew.exe*
> pipx uninstall coveo-stew
uninstalled coveo-stew!
> ls ~/.local/bin
>

@cs01
Copy link
Member

cs01 commented Apr 17, 2021

Why does “done!” appear in the output before it’s actually done?

@itsayellow
Copy link
Contributor Author

itsayellow commented Apr 17, 2021

Why does “done!” appear in the output before it’s actually done?

Ah yes I noticed that too. It's because I am not a "Windows person" and when I use Windows I use MSYS2 and mintty as a shell. It buffers all stdout and stderr and then plays each back in a block, so if they are interspersed they end up out of order.

When using Powershell on Windows it all appears in the proper order.

@itsayellow
Copy link
Contributor Author

One thing I notice is that the Github Windows CI machines actually DO ALLOW symlinks. So just now I ran our tests on my local Windows VM and found that one of the tests fails when it succeeds on the Windows CI.

This is because by design in the code, if we are on a system without symlinks, no metadata + no python interpreter causes the code to not remove any apps in the local binary directory. However on Github CI this is never tested, because the Github Windows machines all allow symlinks.

So I've added a clause to the tests to skip the relevant assertions that binaries were removed in that case on Windows, so the tests work on all Windows machines.

Not sure if there's a way to make the Github CI Windows machines not allow symlinks to mimic the typical Windows experience?

@itsayellow
Copy link
Contributor Author

I asked about Windows symlinks on github CI machines
https://github.community/t/symbolic-links-symlinks-on-windows-machines/175132

@itsayellow
Copy link
Contributor Author

itsayellow commented Apr 24, 2021

I'm pretty sure I need to also look for injected packages to uninstall from the local binary dir, at least for the full metadata present case. I'll double check the code and possibly add some tests that involve injected packages. Never mind, it's fine, it is actually iterating through all the injected packages also.

I suppose I could add an uninstall test for injected apps though.

@itsayellow
Copy link
Contributor Author

One thing I notice is that the Github Windows CI machines actually DO ALLOW symlinks. So just now I ran our tests on my local Windows VM and found that one of the tests fails when it succeeds on the Windows CI.

Investigating a little further, this seems to be because if you are running as Administrator on Windows, you are allowed to make symbolic links, and if you are running as a regular user, you are not allowed to make symlinks. Apparently on Github machines the tests run as Administrator.

We might have to mock the function can_symlink() to disable symlink behavior on Github Windows running as Administrator. I tried fsutil behavior set symlinkevaluation L2L:0 L2R:0 as described here but it didn't seem to prevent pipx running as Administrator from believing it can make new symlinks.

@itsayellow
Copy link
Contributor Author

itsayellow commented Apr 26, 2021

I added a monkeypatch to the pipx_temp_env fixture to make testing on Windows always test to the case where symlinks are not possible. I felt this is the more common case for Windows, so it was good to test it over all of our tests.

This is to get around the fact that Github CI Windows runs as Administrator and thus runs atypically with the ability to make symlinks.

@itsayellow
Copy link
Contributor Author

I'm going to merge this soon unless anyone has any questions or issues they want to bring up. Afterwards I plan on doing the next pipx release.

Copy link

@jonapich jonapich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@cs01
Copy link
Member

cs01 commented Apr 27, 2021

Starting with Windows 10 Insiders build 14972, symlinks can be created without needing to elevate the console as administrator. This will allow developers, tools and projects, that previously struggled to work effectively on Windows due to symlink issues, to behave just as efficiently and reliably as they do on Linux or OSX.

https://blogs.windows.com/windowsdeveloper/2016/12/02/symlinks-windows-10/

Does pipx make symlinks on windows if it’s able to? You noted the admin case works, and it looks like it will work more often than not with newer builds of windows.

Sorry I am not at my laptop and not able to inspect the code.

@itsayellow
Copy link
Contributor Author

Starting with Windows 10 Insiders build 14972, symlinks can be created without needing to elevate the console as administrator. This will allow developers, tools and projects, that previously struggled to work effectively on Windows due to symlink issues, to behave just as efficiently and reliably as they do on Linux or OSX.

https://blogs.windows.com/windowsdeveloper/2016/12/02/symlinks-windows-10/

Does pipx make symlinks on windows if it’s able to? You noted the admin case works, and it looks like it will work more often than not with newer builds of windows.

I have the newest version of Windows 10 to play with, and it does NOT allow making symlinks in its stock configuration without being Administrator.

The link is very informative though!

Apparently you need to put Windows into "Developer Mode" to make shell-based symbolic links work.
https://docs.microsoft.com/en-us/windows/apps/get-started/enable-your-device-for-development
This is not typical for most users but seems possible for some pipx users.

There is also a Windows API feature that is worth further investigation, which seems to allow any program to utilize symbolic links in any mode (if I'm reading it correctly?) Using the flag SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE.
https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createsymboliclinka?redirectedfrom=MSDN

@itsayellow
Copy link
Contributor Author

My thought is that somehow we need to be testing the non-symlink pipx code in CI (We're not currently testing it.) It seems reasonable to test it on Windows since the "normal" behavior of Windows is still to disallow symlinks.

But I'm willing to discuss going back to our old testing.

@cs01 cs01 self-requested a review April 27, 2021 18:25
Copy link
Member

@cs01 cs01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this is definitely an improvement. Thanks for doing this.

@itsayellow
Copy link
Contributor Author

I will say that I'm glad we test if symlinks are possible, rather than simply testing if we're in Windows. In actual use of pipx some Windows users will get the benefit of symlinks.

@itsayellow itsayellow merged commit ded184a into pypa:master Apr 27, 2021
@itsayellow itsayellow deleted the fix-win-uninstall branch April 27, 2021 19:51
@itsayellow
Copy link
Contributor Author

Here's another note on this. It seems that even with the win api call to CreateSymbolicLink* with the SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE flag used, the machine still needs to be in "Developer Mode" for this method to work. (I also just tried it and it didn't work for me not in Developer Mode.)

https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createsymboliclinka?redirectedfrom=MSDN

SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE 0x2: Specify this flag to allow creation of symbolic links when the process is not elevated. Developer Mode must first be enabled on the machine before this option will function.

@itsayellow itsayellow mentioned this pull request Apr 29, 2021
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pipx deletes unrelated executables from .local/bin when uninstalling
3 participants