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

feat(install): add support for pre-injected packages #900

Merged
merged 1 commit into from Aug 6, 2023
Merged

feat(install): add support for pre-injected packages #900

merged 1 commit into from Aug 6, 2023

Conversation

TheKevJames
Copy link
Contributor

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

Summary of changes

Adds support for injecting packages before performing the main package installation. This allows for keyring support via the following, for example for using the Google Artifact Registry:

pipx install \
  --index-url=https://us-python.pkg.dev/my-project/my-index/simple \
  --inject=keyrings.google-artifactregistry-auth \
  my-private-package

This should allow users to install packages with arbitrary additional third-party requirements. This is important especially in cases where those third party requirements are not themselves build dependencies which could be specified via PEP 517: for example, at my company we have a private PyPI index hosted on Google's Artifact Registry which we must install from. The packages themselves should not specify the artifact registry credential helper as a build dependencies, as the packages themselves may be hosted on PyPI itself, this private index, or any other location. Rather, users fetching from that index must be able to specify "...and here's how to do it".

The current best approach I could find through pipx is to make use of --system-site-packages and to install these credential helpers globally. That has a few downsides I would like to avoid, though:

  • requires global installations -- what if we want different versions of our credential helpers, or if their dependencies conflict with other global installs?
  • requires giving projects access to site packages -- this can have plenty of other side-effects which we may not want!

Open question:

  • the --inject flag here has somewhat different semantics than other uses of the "inject" term in pipx. Would renaming to, eg. --pre-inject be clearer?

Test plan

Tested by running the following for several packages hosted on my company's private index. I don't have a great test path to offer folks who don't have access to a private index for testing against. I might be able to look into spinning up a test GCP project and giving a maintainer adhoc access via a Google account? Alternatively, this should support other private indexes such as AWS CodeArtifact, if anyone has access to it for testing!

venv/bin/pipx install \
    --extra-index-url=\https://us-python.pkg.dev/PACKAGE_INDEX/simple\ \
    --inject='keyrings.google-artifactregistry-auth' \
    PACKAGE_NAME

venv/bin/pipx install \
    --pip-args='--extra-index-url=https://us-python.pkg.dev/PACKAGE_INDEX/simple' \
    --inject='keyrings.google-artifactregistry-auth' \
    PACKAGE_NAME

@chrysle
Copy link
Contributor

chrysle commented Feb 11, 2023

I think this should be linked with #442

@dukecat0
Copy link
Member

dukecat0 commented Jul 6, 2023

@uranusjr Any thoughts on this feature?

@uranusjr
Copy link
Member

uranusjr commented Jul 7, 2023

The feature sounds reasonable, and the implementation looks good to me. I am not sure about the --inject name though, since pipx inject is post-injection and that might confuse people. Would it be more obvious if we name the option e.g. --preinstall-inject? (ideas are very welcomed)

@chrysle
Copy link
Contributor

chrysle commented Jul 7, 2023

Would it be more obvious if we name the option e.g. --preinstall-inject? (ideas are very welcomed)

How about shorter --pre-inject?

@uranusjr
Copy link
Member

uranusjr commented Jul 7, 2023

I considered that as well but --pre-inject sounds like doing something before injection, not doing injection before installation. But that’s only my personal feeling.

@chrysle
Copy link
Contributor

chrysle commented Jul 7, 2023

Then we could swap preference, e.g. --inject-pre.

@TheKevJames
Copy link
Contributor Author

Totally open to changing the name! Some other possibilities which could make sense:

  • --init / --initial-packages / --initialize-with
  • --preinstall / --pre-install-packages
  • --inject / --pre-inject / --inject-pre (thanks @chrysle!)

@dukecat0
Copy link
Member

dukecat0 commented Jul 8, 2023

I personally think --preinstall would be a better choice, since those packages are installed before the main package.

@uranusjr
Copy link
Member

I don’t like --inject and --pre-inject (mentioned previously); no particular preference otherwise.

@chrysle
Copy link
Contributor

chrysle commented Jul 10, 2023

@uranusjr Does it sound acceptable to you to use --preinject? I think this removes the focus a bit from your objection.

Adds support for installing packages before performing the main package
installation. This allows for keyring support via the following, for
example for using the Google Artifact Registry:

   pipx install \
      --index-url=https://us-python.pkg.dev/my-project/my-index/simple \
      --preinstall=keyrings.google-artifactregistry-auth \
      my-private-package
@TheKevJames
Copy link
Contributor Author

Updated PR to use --preinstall.

Copy link
Member

@dukecat0 dukecat0 left a comment

Choose a reason for hiding this comment

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

Could you add a test for this feature?

src/pipx/main.py Show resolved Hide resolved
@dukecat0
Copy link
Member

dukecat0 commented Aug 6, 2023

Thanks for adding this feature! I can add tests for this in a separate PR.

@dukecat0 dukecat0 merged commit 871782f into pypa:main Aug 6, 2023
11 checks passed
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.

None yet

4 participants