jitterentropy: 3.3.1 -> 3.4.1 #241671
Merged
jitterentropy: 3.3.1 -> 3.4.1 #241671
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Janik-Haag
added
the
12. first-time contribution
ofborg
bot
added
10.rebuild-darwin: 0
|
Build of rng-tools is failing with this update. For a updated package, see #241685. |
nikstur
reviewed
Jul 6, 2023
thillux
force-pushed
the
jitterentropy-3.4.1
branch
from
cb8a42f to
3dcb2bb
Compare
|
rng-tools 6.16 was merged now, so we can look at this MR again. |
|
The message of 3dcb2bb should also start with the component, e.g.: |
|
Result of 3 packages built:
|
thillux
force-pushed
the
jitterentropy-3.4.1
branch
from
3dcb2bb to
211459b
Compare
Done |
nikstur
added
the
12.approvals: 1
|
Result of 1 package failed to build:
2 packages built:
|
Janik-Haag
reviewed
Jul 7, 2023
There was a problem hiding this comment.
Diff looks good, rng-tools is expected to fail because of the version miss merge and should be fine after a rebase since #241685 is already merged, right?
Very odd. For me all packages build (just ran nixpkgs-review again). This should already be rebased for the changes in rng-tools. |
that's the build log |
|
@ofborg build rng-tools |
|
Now ofborg failed as well :/ |
thillux
force-pushed
the
jitterentropy-3.4.1
branch
from
211459b to
d92f351
Compare
|
I rebased to master again. |
|
Result of 3 packages built:
|
|
Result of 1 package failed to build:
2 packages built:
|
|
I don't get it, still failes with the same error for me. |
|
This might be an issue with hardware impurities. I think the most sensible thing to do here is to disable the test on |
|
@nikstur suggested, that these tests may be somehow hardware dependent. I'll test on some other platform this evening and will have a look again @Janik-Haag. |
|
Thank's for all the effort, I really appreciate it D: |
|
@Janik-Haag I also experienced a failing test on an older notebook. Increasing the timeout in rng-tools did the trick for me: diff --git a/rngd.c b/rngd.c
index 38b81c6..a241dbf 100644
--- a/rngd.c
+++ b/rngd.c
@@ -232,7 +232,7 @@ static struct rng_option jitter_options[] = {
[JITTER_OPT_TIMEOUT] = {
.key = "timeout",
.type = VAL_INT,
- .int_val = 5,
+ .int_val = 10,
},
{
.key = NULL,Should I add a commit, which patches this in rng-tools? |
thillux
force-pushed
the
jitterentropy-3.4.1
branch
from
d92f351 to
daa4a03
Compare
|
Result of 3 packages built:
|
|
Jup that fixed it, I will merge this once ci is successful. Sorry for all the trouble. Btw my computer isn't thaattttt old, it's a t480 from 2018 so only 5 years with a 4 core/8 thread i7. |
|
My daily driver notebook is a T430 from 2011 or so. That's what old meant for me :) I did not have your T480 from the future in mind ^^ |
mweinelt
removed
the
12.approvals: 1
nikstur
requested changes
Jul 10, 2023
There was a problem hiding this comment.
At a minimum the patch needs a link to the upstream PR or Issue.
However, overall, I'd prefer if we just disable the tests instead of including a patch here. The patch will remain in the git history forever and patching upstream bugs is out of scope for Nixpkgs (unless its fixing something Nix-specific or is security relevant).
|
Upstream discussion around rng-tools and current kernels: nhorman/rng-tools#195 |
That's not entirely correct there are some cases where we have to patch stuff like paths or other stuff that is nix specific where a comment describing why and what are we doing is enough. In this case a comment with the link to the upstream issue should be enough in my opinion. |
That's the key to me: if something is nix specific (i.e. is only relevant for Nixpkgs and NixOS) it should (and also kinda has) to be patched inside Nixpkgs. But this is not the case here. Every patch we include becomes a maintenance burden for Nixpkgs maintainers, so we should be wary of them and use them very sparingly. That's why, for example, the systemd maintainers in Nixpkgs try to actively reduce the number of patches we need to apply: #80038 Anyways, anything becomes bikeshedding at some point, so I'm fine with including the patch as is if we get a comment inside the code. |
|
I just wanted to paint a bigger picture, that it's not always possible to put in a link to the upstream issue so the minimum is a comment, and as soon as it's added I'll happily merge it. Your point is totally valid and I would have missed is in this case. |
update to new version 3.4.1 in order to gain support for native timestamps on ARM64. Signed-off-by: Markus Theil <theil.markus@gmail.com>
This install strip is later done through nixpkgs and is not needed. Furthermore it failed for me when cross compiling on x86-64 to aarch64. Signed-off-by: Markus Theil <theil.markus@gmail.com>
thillux
force-pushed
the
jitterentropy-3.4.1
branch
from
daa4a03 to
8f6e64e
Compare
|
rng-tools already has a closed PR regarding the timeout behavior. nhorman/rng-tools#178 I'm currently testing with an adapted package for rng-tools, which sets a larger timeout for the tests, without needing a separate patch. I'll ping you, if it works for me. |
…ropy-3.4.1 With jitterentropy 3.4.1 the initial timeout of rng-tools for the initialization of jitterentropy seems to be too small in some cases. Set a larger timeout for tests. Add comment how this timeout can be set by users needing it. Signed-off-by: Markus Theil <theil.markus@gmail.com>
thillux
force-pushed
the
jitterentropy-3.4.1
branch
from
8f6e64e to
a8efd66
Compare
|
@nikstur @Janik-Haag Please have a look again, if this works for you. I can build rng-tools with running tests on my platform with the latest commit even without an additional patch. |
|
Result of 3 packages built:
|
Janik-Haag
approved these changes
Jul 13, 2023
nikstur
approved these changes
Jul 13, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes
Update jitterentropy to 3.4.1 in order to enable ARM64 support for direct high resolution timestamp reading.
Furthermore, disable jitterentropy's own install with strip, as it is redundant and failed for me when cross-compiling
x86-64 to aarch64.
For a complete changelog, see: https://www.chronox.de/jent.html
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)