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
Conversation
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.
Nice! Thanks for the upgrade and cleanup. |
@@ -8,32 +8,43 @@ | |||
# Not sure if jitterentropy is safe to use for cryptography | |||
# and thus a default entropy source | |||
, jitterentropy ? null, withJitterEntropy ? false |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Why would you not have the error upfront with nix instead of during the
build?
By default all the various derivations are being passed in. You would have to go out of your way to *not* have them available and if you assume that the maintainer or submitter of a PR has done testing with and without a given option enabled, you don't really gain anything. Then the assets are just extra boiler plate code without any real value.
|
Ah, right. I do see your point. I've got two arguments that against that:
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. |
If we follow this to its logical conclusion we can also remove the ? null
default value of the args then?
In this case we can, in general we can't.
There are certain OS specific derivations (systemd comes to mind as the most obvious one) which isn't available on darwin.
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)
Did you mean "when I wouldn't do that"? If not, I don't get it.
2. It makes it obvious and programmatically checkable which dependencies
are needed for which feature. I think that's cool to have. The
Yes, that would very cool indeed although such a tool doesn't exist to my knowledge.
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.
Indeed.
We could offload this discussion into an extra issue for how we want to
handle this in general so more people could chime in.
Discourse is probably the better forum for this kind of discussion.
I guess I'm less convinced of my opinion than before and I'm fine with not
doing it.
So otherwise this PR is good to go from your point of view?
|
Yeah looks good! |
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:
PKCS11 is not fully tested yet as my hardware hasn't arrived.
Cc: @JohnAZoidberg
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)