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

qemu: enable canokey by default #311914

Merged
merged 4 commits into from
May 22, 2024
Merged

qemu: enable canokey by default #311914

merged 4 commits into from
May 22, 2024

Conversation

alyssais
Copy link
Member

Description of changes

Given that we were overriding qemu_test to enable this anyway, enabling this by default saves Hydra a QEMU build.

There's also clear demand from users for this feature, so our alternatives are:

  • Offer a qemu-canokey attribute. I don't want to do this, because I don't think there's any reason to make Hydra build an extra QEMU.

  • Enable it only for qemu_test. I don't want to do this, because it will lead to users using qemu_test without understanding its subtleties.

  • Force users to build from source. I don't think there's any reason to do this when it's unlikely to hurt anybody having it enabled by default. There's no reason to single out canokey to be disabled by default in spite of users' needs given that we enable so many other optional QEMU features.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@alyssais alyssais requested a review from oxalica May 15, 2024 11:16
@github-actions github-actions bot added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label May 15, 2024
@ofborg ofborg bot requested a review from edolstra May 15, 2024 11:58
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 labels May 15, 2024
@tlaurion
Copy link

@alyssais @oxalica thanks for moving this forward.

Copy link
Contributor

@oxalica oxalica left a comment

Choose a reason for hiding this comment

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

I'm fine with enabling it as default. Diff LGTM.

@NickCao
Copy link
Member

NickCao commented May 16, 2024

If the impact on closure size is minimal, I'm for this.

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label May 16, 2024
@oxalica
Copy link
Contributor

oxalica commented May 16, 2024

If the impact on closure size is minimal

canokey-qemu's closure (31.5M) is smaller than the rounding errors of QEMU's (1.5G). The outputs of nix path-info -S -h is the same and you need to remove that -h to see the difference.

/nix/store/qz1n0j1c47crdj67zybn8lymbiryw14d-qemu-8.2.3   1653097944
/nix/store/yji397fspxvawqfiyx47kkfvkk46rfpa-qemu-8.2.3   1652325560

@ofborg ofborg bot requested a review from oxalica May 16, 2024 08:33
@alyssais
Copy link
Member Author

Weird error to only get for a single architecture (x86_64-darwin):

/tmp/nix-build-canokey-qemu-0-unstable-2023-06-06.drv-0/source/canokey-core/applets/oath/oath.c:44:50: error: too many arguments provided to function-like macro invocation
  memcpy(RDATA, (uint8_t[]){OATH_TAG_VERSION, 3, 0x05, 0x05, 0x05, OATH_TAG_NAME, HANDLE_LEN}, 7);
                                                 ^
/nix/store/vw8y07yai2pjv02s1piw3r5cyhmjbddf-Libsystem-1238.60.2/include/secure/_string.h:64:9: note: macro 'memcpy' defined here
#define memcpy(dest, src, len)                                  \
        ^
/tmp/nix-build-canokey-qemu-0-unstable-2023-06-06.drv-0/source/canokey-core/applets/oath/oath.c:44:3: note: parentheses are required around macro argument containing braced initializer list
  memcpy(RDATA, (uint8_t[]){OATH_TAG_VERSION, 3, 0x05, 0x05, 0x05, OATH_TAG_NAME, HANDLE_LEN}, 7);
  ^
                (                                                                            )
1 error generated.

@alyssais
Copy link
Member Author

I guess maybe on aarch64-darwin's libsystem memcpy isn't a macro

@alyssais alyssais marked this pull request as draft May 17, 2024 07:59
@alyssais alyssais force-pushed the qemu-canokey branch 2 times, most recently from 6b5549c to ab94b67 Compare May 18, 2024 08:00
@alyssais alyssais marked this pull request as ready for review May 18, 2024 08:00
alyssais added 2 commits May 18, 2024 11:10
Given that we were overriding qemu_test to enable this anyway,
enabling this by default saves Hydra a QEMU build.

There's also clear demand from users[1] for this feature, so our
alternatives are:

 - Offer a qemu-canokey attribute.  I don't want to do this, because I
   don't think there's any reason to make Hydra build an extra QEMU.

 - Enable it only for qemu_test.  I don't want to do this, because it
   will lead to users using qemu_test without understanding its
   subtleties.

 - Force users to build from source.  I don't think there's any reason
   to do this when it's unlikely to hurt anybody having it enabled by
   default.  There's no reason to single out canokey to be disabled by
   default in spite of users' needs given that we enable so many other
   optional QEMU features.

[1]: canokeys/canokey-qemu#6
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label May 18, 2024
@alyssais
Copy link
Member Author

@ofborg build qemu.tests

@alyssais alyssais merged commit aa0ce1a into NixOS:master May 22, 2024
29 checks passed
@alyssais alyssais deleted the qemu-canokey branch May 22, 2024 16:26
@tlaurion
Copy link

https://nixpk.gs/pr-tracker.html?pr=311914

Still new to nix so not sure how to track when things are part of nixos-unstable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants