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

pynitrokey: 0.4.9 -> 0.4.26 #183099

Merged
merged 11 commits into from
Aug 17, 2022
Merged

Conversation

sbruder
Copy link
Contributor

@sbruder sbruder commented Jul 27, 2022

Description of changes

This updates pynitrokey as part of the Summer of Nix.
It introduces a few new packages that are new dependencies of pynitrokey.

A few notes on how this is packaged:

  • It depends on Update nrfutil and dependencies. #180075, which fixes the nrfutil package, a new dependency of pynitrokey. This PR’s branch includes the commits from that PR to make it build self-contained, but it should not be merged before the other PR is merged.
  • It does not fully use pypemicro, which is an optional dependency of some projects. Where it is a hard dependency, it is patched out. The reason is that it packages prebuilt shared libraries for which no source code is available: Source code for shared libraries? nxp-mcuxpresso/pypemicro#10
  • Since I can’t guarantee that I will be able to maintain this after Summer of Nix, I also added @frogamic as a maintainer to each package, I hope this is okay for you.
  • I don’t own a Nitrokey device and couldn’t test this with real hardware. All (well, most, cf. spsdk) tests for the dependencies pass and the CLI looks functional, so I think it should work. @devhell @frogamic, you were involved in initially adding the package, could you please test if this works with a Nitrokey?
  • I also ran into Missing binutils dependency #176917, which I addressed by adding binutils.bintools to the downstream packages and wrapping the binaries with a LD_LIBRARY_PATH including OpenSSL. I don’t know if this is the preferred way to do this.
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

patches = [
# Fix build with GCC 11
(fetchpatch {
url = "https://github.com/NordicSemiconductor/pc-ble-driver/commit/37258e65bdbcd0b4369ae448faf650dd181816ec.patch";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
url = "https://github.com/NordicSemiconductor/pc-ble-driver/commit/37258e65bdbcd0b4369ae448faf650dd181816ec.patch";
name = "gcc11-fix.patch";
url = "https://github.com/NordicSemiconductor/pc-ble-driver/commit/37258e65bdbcd0b4369ae448faf650dd181816ec.patch";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is directly taken from #180075, so you would need to review this there.

@risicle
Copy link
Contributor

risicle commented Jul 27, 2022

Looking good. I'd suggest adding pythonImportsCheck to all the python packages, even those with tests enabled, as it can sometimes catch problems the tests miss.

@sbruder sbruder force-pushed the update-pynitrokey branch from 23ac66e to f06024c Compare July 28, 2022 07:25
@sbruder
Copy link
Contributor Author

sbruder commented Jul 28, 2022

Thank you for the review! I implemented your suggestions (except for the one from the depending PR). It should now also work on Darwin, though I have no system to test this with. I left the comments to the patches there in addition to the patch names, because I think they add valuable information, though I’m open to removing them.

@sbruder sbruder force-pushed the update-pynitrokey branch from f06024c to 1b72377 Compare July 29, 2022 06:51
@DamienCassou
Copy link
Contributor

I rebased the PR on top of latest master and will merge it in the coming days if nobody says otherwise (or merges before me).

@DamienCassou
Copy link
Contributor

Works fine with my Nitrokey start.

pkgs/development/libraries/libusbsio/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/libusbsio/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/libusbsio/default.nix Outdated Show resolved Hide resolved

installPhase = ''
runHook preInstall
install -D bin/${binDirPrefix}$(uname -m)/libusbsio.${libSuffix} $out/lib/libusbsio.${libSuffix}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we want to use uname -m here. Can't we get that from a nix variable or use a wildcard?

Copy link
Contributor Author

@sbruder sbruder Aug 4, 2022

Choose a reason for hiding this comment

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

The makefile uses uname -m to determine the directory, so I think it should be used to get the same path that the build uses. Or is there a nixpkgs function/constant that returns this?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know but I would suspect that it could be a source for impurity.

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 replaced it with stdenv.targetPlatform.parsed.cpu.name.

pkgs/development/python-modules/spsdk/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/spsdk/default.nix Outdated Show resolved Hide resolved
asn1crypto
astunparse
bincopy
binutils.bintools # required for ctypes to find libcrypto (https://github.com/dbt-labs/dbt-core/issues/3366#issuecomment-849764321)
Copy link
Member

Choose a reason for hiding this comment

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

Can we patch the code instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would probably require patching the python interpreter (see #176917)

Copy link
Member

Choose a reason for hiding this comment

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

thats strange

Copy link
Member

@FRidh FRidh Aug 4, 2022

Choose a reason for hiding this comment

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

binutils (objdump) is indeed one of the methods for ctypes to find dependencies.

Still, in Nixpkgs we patch out ctypes calls in the calling code. This is not for the interpreter.

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 patched the calls in the oscrypto library, from where they originate. This should also fix #176917.

pkgs/development/python-modules/spsdk/default.nix Outdated Show resolved Hide resolved
pkgs/top-level/python-packages.nix Outdated Show resolved Hide resolved
@sbruder sbruder force-pushed the update-pynitrokey branch 4 times, most recently from 461e03b to 05d5a88 Compare August 4, 2022 15:19
@DamienCassou
Copy link
Contributor

Can this be merged now @SuperSandro2000?

@sbruder sbruder force-pushed the update-pynitrokey branch from 05d5a88 to de4cbc3 Compare August 12, 2022 09:48
@sbruder sbruder requested review from SuperSandro2000 and removed request for jonas-schievink August 12, 2022 09:48
@bobby285271 bobby285271 added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Aug 16, 2022
@sbruder sbruder force-pushed the update-pynitrokey branch from de4cbc3 to d6bbf7d Compare August 17, 2022 11:13
@sbruder
Copy link
Contributor Author

sbruder commented Aug 17, 2022

I rebased it onto latest master and fixed an issue with cross-compilation (libusbsio used targetPlatform instead of hostPlatform for determining the library architecture).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: python 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants