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

Let devpi fall back to other options if keyring fails #582

Merged
merged 5 commits into from Jul 14, 2022

Conversation

takluyver
Copy link
Contributor

keyring provides a plugin to supply the password to devpi-client. However, if keyring fails - e.g. it's installed but no backend is working, or unlocking the keyring fails - then devpi login becomes impossible, because the keyring exception is uncaught.

This catches any KeyringError, and returns None instead, which signals to devpi that it should try another way to get the password (docs).

keyring provides a plugin to supply the password to devpi-client. However, if keyring fails - e.g. it's installed but no backend is working, or unlocking the keyring fails - then `devpi login` becomes impossible, because the keyring exception is uncaught.

This catches any KeyringError, and returns None instead, which signals to devpi that it should try another way to get the password ([docs](https://devpi.net/docs/devpi/devpi/stable/+d/devguide/hooks.html#devpi-client-plugin-hooks-experimental)).
@jaraco
Copy link
Owner

jaraco commented Jul 14, 2022

LGTM. I'd prefer to use the suppress decorator, but I'll use your change as inspiration.

@jaraco jaraco self-assigned this Jul 14, 2022
@jaraco
Copy link
Owner

jaraco commented Jul 14, 2022

In this latest patch, I add the use of the jaraco.context.suppress as a simple, elegant decorator. Unfortunately, because that behavior isn't in the stdlib, it adds a dependency, which I just can't justify for an entry point like this, so I'll find another way.

@jaraco jaraco merged commit 278c108 into jaraco:main Jul 14, 2022
@eachimei
Copy link

Hello,
There's an issue with this recent change and how pluggy (which is used by devpi-client) checks the hook-impl.
The short story is: devpi-client uses pluggy -->pluggy uses inspect.getfullargspec(func) to get the implementation arguments --> inspect.getfullargspec(func) does not work well once the function is decorated with contextlib.ContextDecorator --> as a result, pluggy "thinks" the hookimpl doesn't expect any arguments and hence it does not forward the provided arguments to the hookimpl function.
As a result:

$ python -m devpi login <...>
Traceback (most recent call last):
  File "C:\Python38\lib\runpy.py", line 194, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "C:\Python38\lib\runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "...\site-packages\devpi\__main__.py", line 3, in <module>
    main.main()
  File "...\site-packages\devpi\main.py", line 36, in main
    return method(hub, hub.args)
  File "...\site-packages\devpi\login.py", line 13, in main
    password = hub.hook.devpiclient_get_password(
  File "...\site-packages\pluggy\_hooks.py", line 265, in __call__
    return self._hookexec(self.name, self.get_hookimpls(), kwargs, firstresult)
  File "...\site-packages\pluggy\_manager.py", line 80, in _hookexec
    return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
  File "...\site-packages\pluggy\_callers.py", line 60, in _multicall
    return outcome.get_result()
  File "...\site-packages\pluggy\_result.py", line 60, in get_result
    raise ex[1].with_traceback(ex[2])
  File "...\site-packages\pluggy\_callers.py", line 39, in _multicall
    res = hook_impl.function(*args)
  File "C:\Python38\lib\contextlib.py", line 75, in inner
    return func(*args, **kwds)
TypeError: devpiclient_get_password() missing 2 required positional arguments: 'url' and 'username'

I tested with Python 3.8 and 3.9 (same behavior).

Looks like a backwards-compatibility decision in inspect.getfullargspec regarding follow_wrapper_chains=False.

A suggestion for pluggy might be: use inspect.signature(func) instead of getfullargspec since it has the ability to follow wrapper chains (and that's actually the default behavior): https://docs.python.org/3/library/inspect.html#inspect.signature

@jaraco

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

3 participants