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

Add --shared to pipx install and pipx runpip. #829

Closed
wants to merge 4 commits into from

Conversation

Darsstar
Copy link
Contributor

@Darsstar Darsstar commented Mar 18, 2022

Hi,

Is this something you folks would be willing to maintain?

Alternative aproaches to make pipx more suitable for private registries you might prefer which I am willing to implement:

  • make keyring special and always share it
  • support PIPX_PYTHONPATH

If there is interest in maintaining anything like this I will add tests, if not I will maintain this as my private fork without going through the effort of adding tests.

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

Summary of changes

Add --shared to pipx install and pipx runpip.

The main use case for it is as explained in the help text: providing keyring backends while creating virtual environments while pip is looking at private repositories which require authentication.
PYTHONPATH is not an option since pipx explicitly removes it from the dict of environment variables to be passed to the subprocess.
Injecting it after it is created would ia a chicken and the egg problem.

It still won't work out of the box. But now it should no longer be a deal-breaker and instead be, assuming PyPI is reachable, a slight hassle after installing pipx. Documentation should be able to alleviate this further.

Test plan

Tested by running

# command(s) to exercise these changes
pipx install -i https://pypi.org/simple/ --shared keyring-subprocess
pipx install -i https://pypi.org/simple/ keyring
pipx inject -i https://pypi.org/simple/ --include-apps keyring keyring-subprocess-landmark
pipx inject -i https://pypi.org/simple/ keyring artifacts-keyring

# now we are good to go! This should list pip, wheel, setup-tools and keyring-subprocess 
pipx runpip --shared list

pipx install black
pipx install ...

While the following would achieve the same thing, I hope we can all agree the above is better in the sense that we don't use pipx runpip and are thus able to use pipx upgrade. (Also

pipx runpip --shared install keyring-subprocess

@cs01
Copy link
Member

cs01 commented Jun 27, 2022

Thanks for putting this together, and apologies for such a long wait time on the review (we are all volunteers so I hope you can understand).

I haven't used private pypi indexes before so I had to do a little reading to come up to speed on this, but I think I'm there now. For anyone else not familiar, keyring integrates with your OS's password manager, and pip will automatically load credentials for urls in saved in keyring (reference).

I see there is also a netrc, which should be automatically used by pip. Is there a reason you aren't using this instead? (I'm guessing because storing your password in plaintext isn't desirable, but wanted to confirm this was considered as an option).

@Darsstar
Copy link
Contributor Author

Darsstar commented Jun 27, 2022

Plaintext is one reason, yes. The other is that Azure DevOps Personal Access Tokens expire.

Microsoft's artifacts-keyring keyring backend handles that expiration problem.

Pipx solves weird dependecy conflicts I have been asked to help with. Artifacts-keyring reduces authentication related issues for me to help colleagues with by a lot.

Having these two (pipx & keyring) work together better would be nice for me, since I get to clean up our powershell script. (See the keyring-subprocess package's Readme if you are interested)
For other companies it might be the difference in whether they choose to use this excellent tool or not because they gave up sooner then I did.

Until I hear otherwise I'll assume I provided enough motivation for the feature and start working my way through the checklist.

@Darsstar Darsstar force-pushed the pipx_install_--share branch 3 times, most recently from d2e9acb to 2b54cc4 Compare June 27, 2022 22:31
@Darsstar Darsstar marked this pull request as ready for review June 27, 2022 22:53
@Darsstar Darsstar changed the title Add --share to pipx install and --shared to pipx runpip. Add --shared to pipx install and pipx runpip. Jun 27, 2022
@Darsstar
Copy link
Contributor Author

Darsstar commented Jun 27, 2022

Tests, documentation and changelog enties are now all ready for review as well.

I'm also mention my related PR for Pip pypa/pip#11029 in the hope that someone ends up there via this PR. As mentioned there I hope to create a PR that passes--no-input unless --verbose is passed as you mentioned in #219 (comment), but only once I have a way to make sure keyring is still used.

Edit: Tests failed on Windows. That is fixed now as well.

@Darsstar Darsstar force-pushed the pipx_install_--share branch 7 times, most recently from 7dea364 to 7f9a5ad Compare June 28, 2022 21:30
docs/installation.md Outdated Show resolved Hide resolved
Darsstar added 4 commits July 20, 2022 20:17
The main use case for it is as explained in the help text: providing keyring backends while creating virtual environments while pip is looking at private repositories which require authentication.
PYTHONPATH is not an option since pipx explicitly removes it from the dict of environment variables to be passed to the subprocess.
Injecting it after it is created would ia a chicken and the egg problem.

It still won't work out of the box. But now it should no longer be a deal-breaker and instead be, assuming PyPI is reachable, a slight hassle after installing pipx.

In my case:
```
pipx install -i https://pypi.org/simple/ --shared keyring-subprocess
pipx install -i https://pypi.org/simple/ keyring
pipx inject -i https://pypi.org/simple/ --include-apps keyring keyring-subprocess-landmark
pipx inject -i https://pypi.org/simple/ keyring artifacts-keyring
# now we are good to go!
# I made keyring-subprocess specifically for this feature, it has zero dependencies and should not cause a new sort of dependency hell
# we could run `pipx runpip --shared freeze` to verify pip can find keyring-subprocess
pipx install black
pipx install ...
```

While the following would achieve the same thing, I hope we can all agree the above is better:
```
pipx runpip --shared install artifacts-keyring
```
@pfmoore
Copy link
Member

pfmoore commented Aug 22, 2022

Note that in the next release, pip will include a --python option and will be supported when run from a zipapp. Those two changes will open up opportunities for changing the current shared-libs approach (which has a negative effect of making the shared libraries visible in every application venv, which might be an issue for apps that, for example, depend on a specific version of pip, which is unlikely but possible).

As a result, making the existence of the shared libraries visible in the user interface may be something we want to avoid, as it will make it harder to change.

@Darsstar
Copy link
Contributor Author

I should probably repeat here what I said about this PR in my "vendor keyring & keyring-subprocess" PR: if that one gets merged this one would no longer be of any value to me. (And presumably potential Pipx users that want to retrieve credentials for private repositories with the keyring library.)

That is because it is more general: pyprojectx, something a colleague pointed out as something we should take a look at, would become viable as soon as we updated Pip.

@Darsstar
Copy link
Contributor Author

Darsstar commented Jul 3, 2023

Pip 23.1 included the --keyring-provider flag I contributed. Which makes this PR obsolete.

@Darsstar Darsstar closed this Jul 3, 2023
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.

4 participants