Add support for pulling username from keyring subprocess provider - rebased#14150
Add support for pulling username from keyring subprocess provider - rebased#14150YakBizzarro wants to merge 4 commits into
Conversation
|
Hi @YakBizzarro, thanks for the effort you put into rebasing and reviving this! |
thank you! sure, I fully understand, no worry. |
49b24d5 to
3946090
Compare
|
Yeah, in particular, none of the maintainers have any real experience with our keyring support so reviewing any keyring PR is less easy than it may appear on the surface. We'll aim to take a look when we can, but it may be a few weeks until then. |
sure, I understand. Unfortunately I've seen the situation of keyring and its integration in pip is less than ideal, I hope this PR is more help than a burden :) Thanks all! |
There was a problem hiding this comment.
OK, I lied. I didn't want to leave this hanging for too long, so I took a look.
This seems generally good, but do we have a sense of whether keyring < 25.2 is likely to be used out in the wild? At the minimum, we should present a clear warning/error message when an incompatible keyring is used so people know to upgrade keyring.
(My worry is that since keyring is most likely to be used in institutional environments, keyring is unlikely to be regularly upgraded.)
3946090 to
431094c
Compare
This fixes pypa#12543. Somewhat confusingly, when using using `PIP_KEYRING_PROVIDER=import`, pip was able to fetch both a username and a password from keyring, but when using `PIP_KEYRING_PROVIDER=subprocess`, it needed the username. I had to rework the existing tests quite a bit to fit with this new behavior, as it's now OK to ask for a username/password for an index even if you don't have a username. This is why `KeyringSubprocessResult` now subclasses `KeyringModuleV2`. While I was in here, I opted to remove the "fixtures" values from `KeyringModuleV2` by introducing this new `add_credential` contextmanager. IMO, they were hurting the readability of our tests: you had to jump around quite a bit to see what the contents of the keyring would be during our tests. It also forced all our tests to play nicely with the same fixtures values, which IMO was an unnecessary constraint. Note: per the discussion [here](pypa#12748 (comment)), I've opted not to implement any "feature detection" to see if the `keyring` subprocess supports `--mode=creds`. For the record, this feature was added in jaraco/keyring@7830a64, which landed in keyring v25.2.0.
I found that with my changes, some of the xfails in here started failing (because the underlying tests are actually passing now). I found the existing test very difficult to understand (I'm honestly not sure I undertood them, especially the logic around `keyring_provider` in the test setup), so I opted to refactor things a bit to hopefully make them make sense.
431094c to
063473a
Compare
Thank @ichard26 for the quick review!! I think it's a valid concern. I believe that as long as we warn the user about the incompatibility, they can upgrade with something with pipx, even if there is a system-wide keyring installation that can't be upgraded. For that, I added a detection that warns the user if its keyring is old, see commit 6ea43ce It will result in something like this: Small note: version 25.2.0 is bugged and the credential mode doesn't work and it's not detected by my current code. I can add it, but the it's more code to maintain and test, and this release was out for only 2 weeks before being fixed in 25.2.1. I didn't found any Linux package of it. So there is a very small risk that someone has a incompatible version, but we don't warn him. Let me know if you think it's something to take care. |
With the latest changes, now we requires keyring 25.2.1 or later. This commit add a check for keyring version by capturing and parsing the error message of keyring invocation when querying credentials. The check is reported as warning to the user. A test is also added to verify this error path.
063473a to
6ea43ce
Compare
ichard26
left a comment
There was a problem hiding this comment.
Small note: version 25.2.0 is bugged and the credential mode doesn't work and it's not detected by my current code. I can add it, but the it's more code to maintain and test, and this release was out for only 2 weeks before being fixed in 25.2.1. I didn't found any Linux package of it. So there is a very small risk that someone has a incompatible version, but we don't warn him. Let me know if you think it's something to take care.
That's fine. Perhaps it'd be worth asking the keyring folks to yank 25.2.0.
This looks good, thanks!
|
many thanks! especially for fixing the failing test :) @jaraco do you think it's possible to yank version 25.2.0 of keyring from PyPI? thank you! |

What does this PR do?
Somewhat confusingly, when using using PIP_KEYRING_PROVIDER=import,
pip was able to fetch both a username and a password from keyring, but
when using PIP_KEYRING_PROVIDER=subprocess, it needed the username.
In this PR, I rebased the work done by @jfly in #12748 unto the the latest main. In the rebase I:
pip("install", ...I moved topip_install_localFrom the original PR, still apply:
I think all the points raised in the original review have been addressed. The point in #12748 (comment) I think has been addressed in the original PR, but please let me know if I misunderstood and there is something still open.
Relative tests pass.
For @jfly: many thanks for the original work and the permission to re-sumbit. I left you as author since you did 99% of the job
Fixes #12543
PR Checklist:
Assisted-by: Claude Sonnet 5
AI has been used only as final check to verify the sanity of the rebase after it has been done by me. No code has been authored by AI directly or indirectly. No text has been written or assisted by AI, all mistakes are my own.