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.

Sign up for GitHub

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

Jump to bottom

Make pyscard dependency optional #580

Merged
merged 2 commits into from
Nov 28, 2024
Merged

Make pyscard dependency optional #580

merged 2 commits into from
Nov 28, 2024

Conversation

sosthene-nitrokey
Copy link
Contributor

This PR allows using nitropy without pyscard installed.

Changes

  • Make pyscard dependency optional

Checklist

Make sure to run make check and make fix before creating a PR, otherwise the CI will fail.

  • tested with Python3.9
  • signed commits
  • updated documentation (e.g. parameter description, inline doc, docs.nitrokey)
  • added labels

Test Environment and Execution

  • OS:
  • device's model:
  • device's firmware version:

Relevant Output Example

 nitropy.py nk3 piv --experimental info
Command line tool to interact with Nitrokey devices 0.6.0
Critical error:
pyscard is not available, please consult https://docs.nitrokey.com/nitrokey3/linux/troubleshooting#pcsc-unavailable for more information

--------------------------------------------------------------------------------
Critical error occurred, exiting now
Unexpected? Is this a bug? Would you like to get support/help?
- You can report issues at: https://support.nitrokey.com/
- Writing an e-mail to [email protected] is also possible
- Please attach the log: '/tmp/nitropy.log.fkkdw9qs' with any support/help request!
- Please check if you have udev rules installed: https://docs.nitrokey.com/nitrokey3/linux/firmware-update.html#troubleshooting

Fixes #579

@sosthene-nitrokey
Copy link
Contributor Author

Not sure how to verify that pyscard is still included in the pre-built binaries.

@sosthene-nitrokey
Copy link
Contributor Author

Looks like flit by default installs all dependencies so we should be good: https://flit.pypa.io/en/stable/cmdline.html#cmdoption-flit-install-deps

robin-nitrokey
robin-nitrokey reviewed Oct 24, 2024
View reviewed changes
pynitrokey/cli/nk3/piv.py Outdated
@click.argument("args", nargs=-1, type=click.UNPROCESSED)
def piv(args: list[str]) -> None:
"""Nitrokey PIV App"""
local_critical(PCSC_ABSENT)
Copy link
Member

Choose a reason for hiding this comment

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

I would set support_hint=False because this is not an unexpected error (so we don’t need the logs) and it is easy to miss the actual error message in all that noise.

docs/packaging.rst
Comment on lines +23 to +24
To limit the need to install pyscard, it is made optional.
If you make it an optional dependency of your package, please patch ``pynitrokey/cli/nk3/pcsc_absent.py`` to indicate users how they can install it.
Copy link
Member

Choose a reason for hiding this comment

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

Should we recommend to always enable it if possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm in favor of allowing users to make use of a minimal set of dependencies.

pynitrokey/cli/nk3/pcsc_absent.py Outdated
@@ -0,0 +1 @@
PCSC_ABSENT = "pyscard is not available, please consult https://docs.nitrokey.com/nitrokey3/linux/troubleshooting#pcsc-unavailable for more information"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
PCSC_ABSENT = "pyscard is not available, please consult https://docs.nitrokey.com/nitrokey3/linux/troubleshooting#pcsc-unavailable for more information"
PCSC_ABSENT = "This command requires the pyscard library that is not available on your system. Please consult https://docs.nitrokey.com/nitrokey3/linux/troubleshooting#pcsc-unavailable for more information"

@robin-nitrokey
Copy link
Member

AFAIR it is easy to build the installer locally. I can test it later today or tomorrow to verify the behavior.

@robin-nitrokey
Copy link
Member

The Linux onefile build includes pcsc so the Windows builds should be fine too.

@sosthene-nitrokey
Copy link
Contributor Author

sosthene-nitrokey commented Oct 25, 2024 •
edited
Loading

  • This PR will need to be update for the link to the troubleshooting steps when the docs are updated

@robin-nitrokey
Copy link
Member

I think we can merge this now, right?

@sosthene-nitrokey sosthene-nitrokey merged commit d67ae66 into master Nov 28, 2024
8 checks passed
@sosthene-nitrokey sosthene-nitrokey deleted the fix-deps branch November 28, 2024 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robin-nitrokey robin-nitrokey robin-nitrokey approved these changes

@daringer daringer daringer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

During pipx upgrade on Debian: fatal error: winscard.h: No such file or directory
3 participants
@sosthene-nitrokey @robin-nitrokey @daringer