Skip to content

make-initrd: use closureInfo again#372931

Merged
philiptaron merged 3 commits intoNixOS:stagingfrom
xaverdh:make-initrd-closure-info
Mar 10, 2025
Merged

make-initrd: use closureInfo again#372931
philiptaron merged 3 commits intoNixOS:stagingfrom
xaverdh:make-initrd-closure-info

Conversation

@xaverdh
Copy link
Copy Markdown
Contributor

@xaverdh xaverdh commented Jan 11, 2025

nix 1.* is quite old now (the original issue #36268 was closed many years ago)

Things done

This is basically a revert of 165b32d

  • 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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added the 6.topic: kernel The Linux kernel label Jan 11, 2025
@nix-owners nix-owners bot requested a review from philiptaron January 11, 2025 13:26
@github-actions github-actions bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jan 11, 2025
Copy link
Copy Markdown
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

The diff looks good to me. How should it be tested? I ran a few nixos tests, since it looks like this is version of making the initrd is the one used there. They all came out fine.

@mweinelt told me that things that make all nixos tests rebuild ought to go to staging, even if they have small rebuild counts here.

Nix 2.* is widely used now, so closureInfo should be used instead.
@xaverdh xaverdh force-pushed the make-initrd-closure-info branch from e9943fa to e540245 Compare January 23, 2025 15:23
@xaverdh xaverdh changed the base branch from master to staging January 23, 2025 15:24
@xaverdh
Copy link
Copy Markdown
Contributor Author

xaverdh commented Jan 23, 2025

The diff looks good to me. How should it be tested? I ran a few nixos tests, since it looks like this is version of making the initrd is the one used there. They all came out fine.

@mweinelt told me that things that make all nixos tests rebuild ought to go to staging, even if they have small rebuild counts here.

retargeted it at staging and removed pathsFromGraph

I tested it by building my own initrd both from a local checkout of nixpkgs and then cherry-picking the changes here and building from that. When comparing the outputs with diff they were bit-for-bit identical actually.

@xaverdh
Copy link
Copy Markdown
Contributor Author

xaverdh commented Jan 23, 2025

Also when testing, make sure the initrd you are building actually uses this code path and not make-initrd-ng, as e.g. systems with boot.initrd.systemd.enable = true.

Copy link
Copy Markdown
Contributor

@philiptaron philiptaron 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 sad to report that you've triggered the following bug in Nix:

For details, read:

To show this, run the nixosTests.installer.simple test. With this PR, it fails with an inscrutable curl error (per the bug).

How to fix? I think it's something like:

But I don't see the line that's triggering the bug immediately.

@Mic92
Copy link
Copy Markdown
Member

Mic92 commented Feb 3, 2025

@philiptaron how do you see that it triggers: NixOS/nix#11503 ?

@ElvishJerricco
Copy link
Copy Markdown
Contributor

@philiptaron I believe the issue isn't NixOS/nix#11503 but rather just an ordinary case of a missing dependency in the installer test. closureInfo depends on jq, and while the installation-device.nix module adds jq to system.extraDependencies, it's not adding the right output. It needs to add lib.getDev jq instead. This appears to fix it:

diff --git a/nixos/modules/profiles/installation-device.nix b/nixos/modules/profiles/installation-device.nix
index a4e5a4aac790..f01867c7e29c 100644
--- a/nixos/modules/profiles/installation-device.nix
+++ b/nixos/modules/profiles/installation-device.nix
@@ -102,7 +102,7 @@ with lib;
         stdenv
         stdenvNoCC # for runCommand
         busybox
-        jq # for closureInfo
+        (lib.getDev jq) # for closureInfo
         # For boot.initrd.systemd
         makeInitrdNGTool
       ];

Alternatively, we could append jq.all to that extraDependencies list just to make sure we always have the whole thing.

@philiptaron
Copy link
Copy Markdown
Contributor

@philiptaron how do you see that it triggers: NixOS/nix#11503 ?

Jörg, @Mic92, My evidence is circumstantial: most triggers of this bug cause curl to be erroneously redownloaded, and that's what I saw here when running the nixosTests.installer.simple test. Per Will, @ElvishJerricco, there are multiple ways to hit that, and my sniff test failed in this instance.

@philiptaron philiptaron dismissed their stale review February 13, 2025 17:33

Still need to make the nixosTests.installer.simple test pass

Copy link
Copy Markdown
Contributor

@ElvishJerricco ElvishJerricco left a comment

Choose a reason for hiding this comment

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

See above comment about jq for the installer tests.

As discovered in NixOS#372931, we need the dev output of jq for closureInfo. We opt to add the whole thing.
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Mar 9, 2025
@philiptaron philiptaron dismissed ElvishJerricco’s stale review March 10, 2025 01:47

nixosTests.installer.simple passes now.

@philiptaron philiptaron merged commit 711bf41 into NixOS:staging Mar 10, 2025
27 checks passed
@xaverdh xaverdh deleted the make-initrd-closure-info branch March 10, 2025 06:35
@nixos-discourse
Copy link
Copy Markdown

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/issue-building-linux-kernel-modules-after-flake-update/62322/8

@trofi
Copy link
Copy Markdown
Contributor

trofi commented Jan 22, 2026

Proposed a minor cleanup to drop no longer used toValidStoreName as:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: kernel The Linux kernel 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants