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

Vendor keyring & keyring-subprocess #11399

Closed
wants to merge 4 commits into from

Conversation

Darsstar
Copy link
Contributor

I went ahead and made a proper PR for what I first talked about in #8719 (comment)

Q: But keyring has C dependencies, how do you expect this to work?
A: Some of keyring's first party backend's have C dependencies, we won't be using those and instead use my keyring-subprocess library's pure Python backend. Therefore we don't actually need to vendor them. If you would like I can alter the vendoring configuration to make sure those backend's are not vendored.

Fixes #8719
Can be an alternative, or in addition to: #11215. Which fixes the same issue.

Would go very well together with #11029

I still haven't looked at the Pip zipapp implementation and what that means for keyring users. I only know keyring and truststore being exceptions to the vendoring rule came up.

Related: pypa/pipx#829 would no longer be something would want

Dos Moonen and others added 4 commits August 21, 2022 19:38
…t opt in.

Opting in is done by installing keyring-subprocess[landmark] and making sure the keyring-subprocess executable it provides can be found on the PATH.
@Darsstar Darsstar force-pushed the vendor-keyring-subprocess branch from d145abb to 024bfdd Compare August 21, 2022 18:56
@github-actions github-actions bot added the needs rebase or merge PR has conflicts with current master label Nov 10, 2022
@judahrand
Copy link
Contributor

@Darsstar I'm not sure this is needed now that #11589 is merged? They achieve similar goals if I understand it correctly?

@Darsstar
Copy link
Contributor Author

Woow, you got that merged quick!

Time to take a look. My first question to answer by studing the code would be "how does this interact with activating a virtual environment which has keyring as a dependency?".
My keyring-subprocess first tried to look for a global keyring, but Poetry has keyring as a dependency and therefor a keyring executable which was found before the global one.

I suspect I will end up closing this PR.

@judahrand
Copy link
Contributor

judahrand commented Nov 10, 2022

Woow, you got that merged quick!

Time to take a look. My first question to answer by studing the code would be "how does this interact with activating a virtual environment which has keyring as a dependency?". My keyring-subprocess first tried to look for a global keyring, but Poetry has keyring as a dependency and therefor a keyring executable which was found before the global one.

I suspect I will end up closing this PR.

Yeah, the code that got merged defaults to using a locally installed keyring via import. This was done to avoid breaking things for people. The fallback of looking on PATH just uses shutil.which so it'll find the first one anyway.

Additionally, with the addition of the --python option to pip one could try having a 'global' pip installed as a zipapp or with pipx and a pipx install of keyring. The global pip and keyring could then talk to each other even when using pip install --python .venv for example.

Super cool possibilities.

@uranusjr
Copy link
Member

There’s an issue on not automatically importing keyring #8719. Perhaps we should make the option multi-choice (no keyring, force subprocess, and force import)?

@judahrand
Copy link
Contributor

judahrand commented Nov 10, 2022

There’s an issue on not automatically importing keyring #8719. Perhaps we should make the option multi-choice (no keyring, force subprocess, and force import)?

The issue is that if we're just looking on PATH with which then import==subprocess when import is possible, no? (In terms of the results) Unless, we make that logic more sophisticated.

I like the idea of making it up to the user which option is used though! A lot.

@uranusjr
Copy link
Member

uranusjr commented Nov 10, 2022

Not always, say you can do

$ virtualenv foo
$ foo/bin/pip install keyring
$ pipx install keyring
$ foo/bin/pip install whatever  # Likely using the pipx one, not the virtualenv.

But I get your point, the subprocess implementation preferring the same keyring in the same environment is still a problem in many situations.


Also for the Poetry issue specifically, keyring in the env Poetry is installed into should not be in PATH (or you probably should seriously visit how you set up your dev env), so at least for this scenario it’s possible to prefer the out-of-env keyring installation.

@Darsstar
Copy link
Contributor Author

Darsstar commented Nov 10, 2022

Perhaps we should make the option multi-choice (no keyring, force subprocess, and force import)?

You/it got my vote! For whatever that is worth... (it referring to your idea, not you. Regardless of who might end up implementing it.)


The issue is that if we're just looking on PATH with which then import==subprocess when import is possible, no? (In terms of the results) Unless, we make that logic more sophisticated.

I guess shutil.which's path parameter can be used after using the sysconfig module to remove the scripts path from PATH would be a pretty good start?


From what I can tell by just studying the code that should work after a tiny change in our setup. (include a username in the URL.)
I'll try to test that next thursday at work when I'm back in the office after some days off.

@judahrand
Copy link
Contributor

Yup it also requires us to set the username. We're using the Google Artifact Registry keyring backend.

@Darsstar
Copy link
Contributor Author

I'll try to test that next thursday at work when I'm back in the office after some days off.

I forgot, then I remembered but didn't have time, then I forgot again, but I finally remembered again!

It doesn't work on Windows due to hardcoding "\n" as the line seperator instead of os.linesep. I just fixed that as part of my --force-keyring PR: #11029.

Other than that it works great! My fear of it tripping up Poetry turned out to be a nothing burger as you expected, @uranusjr .

Time to create a very quick bug report...

@Darsstar Darsstar closed this Dec 13, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 29, 2022
@Darsstar Darsstar deleted the vendor-keyring-subprocess branch January 15, 2024 23:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs rebase or merge PR has conflicts with current master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keyring support should require an --enable-keyring flag
3 participants