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

[WIP] add --keyring-provider flag #2592

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

Conversation

tdyas
Copy link
Contributor

@tdyas tdyas commented Nov 13, 2024

Add a --keyring-provider flag to allow Pex callers to configure Pip to use a keyring provider without the need to loosen the Pip isolated mode entirely as would be done if --use-pip-config were passed.

pex/pip/tool.py Outdated Show resolved Hide resolved
@tdyas tdyas changed the title add --with-extra-pip-arg flag [WIP] add --with-extra-pip-arg flag Nov 13, 2024
@jsirois
Copy link
Member

jsirois commented Nov 13, 2024

The "./dtox.sh -e py38-pip22_3_1-integration --shard 2/2" shard failure is bitrot from 11/11 addressed here: #2595

@tdyas
Copy link
Contributor Author

tdyas commented Nov 13, 2024

The "./dtox.sh -e py38-pip22_3_1-integration --shard 2/2" shard failure is bitrot from 11/11 addressed here: #2595

With dtox.sh, my unit test appears to pass. I'll leave this PR in WIP state though while I work on the Pants-side of the ultimate change.

@@ -415,16 +415,21 @@ def test_keyring_provider(
download_dir = os.path.join(str(tmpdir), "downloads")
assert not os.path.exists(download_dir)

with ENV.patch(PIP_KEYCHAIN_PROVIDER="invalid") as env, environment_as(**env):
assert "invalid" == os.environ["PIP_KEYCHAIN_PROVIDER"]
with ENV.patch(PIP_KEYRING_PROVIDER="invalid") as env, environment_as(**env):
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if you've seen /home/jsirois/dev/pex-tool/pex/tests/integration/test_keyring_support.py, but its probably what you eventually want to amend to test the "import" + --extra-pip-requirement and "subprocess" + PATH means of hooking the keyring provider.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Couldn't I just update that test to use the new --keyring-provider option instead of the existing --use-pip-config option? (Or paramterize the test to try both methods?)

Copy link
Member

Choose a reason for hiding this comment

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

~the latter is exactly what I was suggesting, yes.

Copy link
Member

Choose a reason for hiding this comment

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

Hopefully you've noticed though how tortured the very concept of a keyring provider is. There is a nasty bootstrap problem for any locked down org.

Copy link
Contributor Author

@tdyas tdyas Nov 19, 2024

Choose a reason for hiding this comment

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

Yeah, the whole shebang is a marvelous house of cards. The work in Pants using this support focuses on --keyring-provider=subprocess with a trampoline keyring script so Pants won't need to bootstrap keyring PyPi at all.

Copy link
Contributor Author

@tdyas tdyas Nov 19, 2024

Choose a reason for hiding this comment

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

Updated the integration test to test the option.

Weird failure with https://github.com/pex-tool/pex/actions/runs/11918318085/job/33215488401#step:5:5425 since the test should not have tried to execute with a Pip that does not support the option. Any thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Absolutely. I'll hint you 1st: how do you think Pex implements --pip-version any-not-vendored-version?

Copy link
Member

@jsirois jsirois Nov 19, 2024

Choose a reason for hiding this comment

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

The answer to that is fairly obvious and I'm sure you've already forehead smacked. That said, solving is thornier. I already had to deal with how to bootstrap --pip-version any-not-vendored-version here:

pex/pex/pip/installation.py

Lines 290 to 305 in 3361e79

bootstrap_pip_version = try_(
compatible_version(
targets,
PipVersion.VENDORED,
context="Bootstrapping Pip {version}".format(version=version),
warn=False,
)
)
if bootstrap_pip_version is not PipVersion.VENDORED and not extra_requirements:
return _pip_installation(
version=version,
iter_distribution_locations=_bootstrap_pip(version, interpreter=interpreter),
interpreter=interpreter,
fingerprint=_fingerprint(extra_requirements),
use_system_time=use_system_time,
)

The context there is using a new enough Python (3.12+) where distutils is gone from the stdlib and vendored Pip fails as a result. In that situation I had to find another way to bootstrap a specific Pip version without using vendored Pip. So, basically, iff using the current python to python -mvenv nets a venv with a Pip new enough to support --keyring-provider, the thorny bootstrap can be just the thorny problem you described, solved with --keyring-provider subprocess and some pre-arranged provider on the PATH. If not, you have an even thornier bootstrap problem than you expected.

@tdyas tdyas changed the title [WIP] add --with-extra-pip-arg flag [WIP] add --keyring-provider flag Nov 19, 2024
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.

2 participants