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

rng-tools: 6.6 -> 6.7 and libp11: 0.4.9 -> 0.4.10 #60142

Merged
merged 2 commits into from Apr 25, 2019

Conversation

peterhoeg
Copy link
Member

Motivation for this change

https://github.com/nhorman/rng-tools/releases/tag/v6.7

The libp11 change is included here as rng-tools is now the first consumer of libp11.

A few other changes:

  • use autoreconfHook instead of doing it manually
  • clean up with/enable flags
  • add support for PKCS11 entropy sources

PKCS11 is not fully tested yet as my hardware hasn't arrived.

Cc: @JohnAZoidberg

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

The build was broken as the output was nested inside /nix/store so fix that as
well. We didn't know as no other derivation was using it.
A few other changes:

 - use autoreconfHook instead of doing it manually
 - clean up with/enable flags
 - add support for PKCS11 entropy sources

PKCS11 is not fully tested yet as my hardware hasn't arrived.
@peterhoeg peterhoeg changed the title rng-tools: 6.6 -> 6.6 and libp11: 0.4.9 -> 0.4.10 rng-tools: 6.6 -> 6.7 and libp11: 0.4.9 -> 0.4.10 Apr 24, 2019
@JohnAZoidberg
Copy link
Member

Nice! Thanks for the upgrade and cleanup.
I had the version bump sitting in my git stash for a while and forgot about it...

@@ -8,32 +8,43 @@
# Not sure if jitterentropy is safe to use for cryptography
# and thus a default entropy source
, jitterentropy ? null, withJitterEntropy ? false
Copy link
Member

Choose a reason for hiding this comment

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

We could add asserts for the dependencies of the optional features, so that if the feature is enabled the dependencies cannot be null, i.e. withJitterEntropy -> jitterentropy != null.
That's why I added the dependencies for the features in the same line as the option.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have seen that pattern in quite a few places but must admit I'm not really convinced about it. If you try building with a feature enabled but the relevant library is missing, the build will simply error out, so except for large packages where it could blow up 20 minutes into the build and we therefore want it to stop immediately, I don't really think we gain anything. That's just my opinion of course.

Copy link
Member

Choose a reason for hiding this comment

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

Why would you not have the error upfront with nix instead of during the build?
So are you of the opinion we should only do this for large packages? Why not for small ones? What's the advantage?
Sorry, I don't quite understand what you're trying to say.

@peterhoeg
Copy link
Member Author

peterhoeg commented Apr 25, 2019 via email

@JohnAZoidberg
Copy link
Member

Ah, right. I do see your point.
If we follow this to its logical conclusion we can also remove the ? null default value of the args then?

I've got two arguments that against that:

  1. The derivation stands on it's own - you make your conclusion based on the assumption that it's always called with callPackage that automatically passes all possible packages. (Can't think of a concrete useful situation when I would do that)

  2. It makes it obvious and programmatically checkable which dependencies are needed for which feature. I think that's cool to have. The downside is that we don't actually try to build all combinations of arguments to see whether the checks are correct - so they are an additional burden on the maintainers.

We could offload this discussion into an extra issue for how we want to handle this in general so more people could chime in.

I guess I'm less convinced of my opinion than before and I'm fine with not doing it.

@peterhoeg
Copy link
Member Author

peterhoeg commented Apr 25, 2019 via email

@JohnAZoidberg
Copy link
Member

Yeah looks good!

@peterhoeg peterhoeg merged commit 1011fae into NixOS:master Apr 25, 2019
@peterhoeg peterhoeg deleted the u/rng branch April 25, 2019 05:19
@peterhoeg peterhoeg restored the u/rng branch April 29, 2019 06:49
@peterhoeg peterhoeg deleted the u/rng branch April 29, 2019 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants