initrd: cross compile fix #96005

xaverdh · 2020-08-22T16:40:18Z

Motivation for this change

Make initrd building logic cross friendly, see discussion here.
I hope this is the right way to go about it, I have zero experience with cross.

As a side effect this makes for a somewhat cleaner interface to initrd compression options, which also get exposed (the compressor option was marked as internal before this).

Things done

Did some basic testing on my local machine playing with xz compression and compression levels.
Sadly I don't have the setup to quickly test cross compiling though..

lopsided98

I haven't tested this, but it seems like a good approach.

lopsided98 · 2020-08-22T17:39:40Z

nixos/modules/system/boot/stage-1.nix

+    boot.initrd.compressorArgs = mkOption {
+      internal = true;
+      default = null;
+      type = types.nullOr types.str;


I think this would be better as types.listOf types.str. Then you could use escapeShellArgs and it would handle spaces in arguments. I'm not sure why you would need spaces in compressor arguments, but it doesn't hurt to do it right.

Also, you could get rid of types.nullOr by specifying the gzip fallback using boot.initrd.compressorArgs = mkIf (config.boot.initrd.compressor == "gzip") mkDefault [ "-9n" ] (that might need a few more parens to work right).

Sounds like the right solution here, thanks!

On second thought, I have issues passing the compressor to the builder shell script. I guess this would be easier with structured attributes, but there is a lot of bashism in the shell script, that I am not familiar with and don't want to touch.
The way I do it now, the compression command gets passed to the build as a string, but this then requires using eval..

well I guess the current version also does eval by expanding the variable like this, so its not worse than before I guess

Yeah, that is an annoying problem. I don't think eval is too bad here, but maybe someone else has an opinion.

nixos/modules/system/boot/stage-1.nix

lopsided98 · 2020-08-22T19:47:09Z

Also, this will need a changelog entry because it is a breaking change.

xaverdh · 2020-08-22T21:59:56Z

Also, this will need a changelog entry because it is a breaking change.

Just the changes to makeInitrd, or those to the boot.initrd module as well?
Strictly speaking the compressor option is marked as internal, so breaking it should not need documentation? On the other hand I can imagine people actually use this option, internal or not.

lopsided98 · 2020-08-22T22:10:31Z

Strictly speaking the compressor option is marked as internal, so breaking it should not need documentation? On the other hand I can imagine people actually use this option, internal or not.

Yeah, I don't see why it should be internal. I'd say we should remove the internal flag and document the breakage since users are definitely using the option.

xaverdh · 2020-08-22T22:27:02Z

I pushed two more commits, one for the release notes, and one that makes the compression options public.

xaverdh · 2020-08-23T08:16:25Z

reworded the release notes slightly and put them in the right section

lheckemann

I like the idea of fleshing out the options for compression a bit, especially laying the pathway to handling argument escaping properly — big 👍 on that (shame it can't be done without structuredAttrs or some hackery that would blow this out of proportion).

I'm not so sure about forcing referencing a package from a specified set — specifying the compressor when cross-building is already possible by specifying something like compressor = "${lib.getBin buildPackages.xz}/bin/xz";, and this approach is more flexible in that it allows using any compressor (as long as it accepts piping from stdin) even if it isn't known to the makeInitrd expression.

lheckemann · 2020-09-04T16:58:19Z

nixos/doc/manual/release-notes/rl-2009.xml

@@ -743,6 +743,11 @@ CREATE ROLE postgres LOGIN SUPERUSER;
     See <link xlink:href="https://github.com/NixOS/nixpkgs/pull/82743#issuecomment-674520472">the PR that changed this</link> for more info.
    </para>
   </listitem>
+   <listitem>
+    <para>
+     The <literal>makeInitrd</literal> helper package now seperates the <literal>compressor</literal> parameter from the arguments passed to the <literal>compressor</literal> (which will be properly escaped). So if you previously used e.g. <programlisting>makeInitrd { compressor = "xz --check=crc32 --lzma2=dict=512KiB"; }</programlisting>, replace this by: <programlisting>makeInitrd { compressor = "xz"; compressorArgs = [ "--check=crc32" "--lzma2=dict=512KiB" ]; }</programlisting> Similar changes were made to the compressor option of the <literal>boot.initrd</literal> module, which is no longer marked as internal.


nit: separates

lheckemann · 2020-09-04T17:00:14Z

nixos/modules/system/boot/stage-1.nix

-      default = "gzip -9n";
-      type = types.str;
+      default = "gzip";
+      type = types.enum [ "cat" "gzip" "bzip2" "xz" "lz4" "lzop" "zstd" ];


I'm not all that keen on limiting the options like this, nor on then having to special-case cat elsewhere — e.g. lzma is missing here, as well as parallel implementations of some of the algorithms like pigz and pixz; and the kernel might grow new compression algorithm support. Duplicating that information here isn't ideal.

Re: lzma see #96005 (comment)

lheckemann · 2020-09-04T17:09:16Z

pkgs/build-support/kernel/make-initrd.nix

@@ -31,7 +34,8 @@ in stdenvNoCC.mkDerivation rec {
  makeUInitrd = stdenvNoCC.hostPlatform.platform.kernelTarget == "uImage";

  nativeBuildInputs = [ perl cpio ]
-    ++ stdenvNoCC.lib.optional makeUInitrd ubootTools;
+    ++ stdenvNoCC.lib.optional makeUInitrd ubootTools


nit: we're taking lib as an arg, so we don't need to use stdenvNoCC to get to it :) though since this was already like this I don't think it needs to be changed in the context of this PR.

lheckemann · 2020-09-04T17:22:37Z

Clarification on what I hope we can achieve here: all the flexibility of being able to specify arbitrary compressors without having to modify the nixpkgs tree.

lopsided98 · 2020-09-04T17:29:45Z

I'm not so sure about forcing referencing a package from a specified set — specifying the compressor when cross-building is already possible by specifying something like compressor = "${lib.getBin buildPackages.xz}/bin/xz";, and this approach is more flexible in that it allows using any compressor (as long as it accepts piping from stdin) even if it isn't known to the makeInitrd expression.

The problem is that the compressor is needed at build time, as well as potentially at run time for the initrd secrets system. Would the user have to manually set a second option with the host version of the package?

lheckemann · 2020-09-04T17:42:27Z

Aaah, good catch. One way around this, though awful (:sweat_smile:), is to require a function like compressor = pkgs: "${lib.getBin pkgs.xz}/bin/xz";… Maybe a decent middle ground would be providing these strings as handy defaults, with the function as an "escape hatch"? This is very much a matter of opinion and I'm not sure what mine is yet.

lheckemann · 2020-09-04T17:44:45Z

I've just opened #97145 with some tangentially related work.

xaverdh · 2020-09-05T10:43:59Z

Aaah, good catch. One way around this, though awful (sweat_smile), is to require a function like compressor = pkgs: "${lib.getBin pkgs.xz}/bin/xz";… Maybe a decent middle ground would be providing these strings as handy defaults, with the function as an "escape hatch"? This is very much a matter of opinion and I'm not sure what mine is yet.

Yes, its somewhat unfortunate that we don't have a canonical way to get the binary contained in a package. Another way I can think of, would be for the user to pass in tuples like { package = "..."; binary = ".."; } (as an escape hatch).

But really the proper solution I think is to have a function in lib which extracts the path the binary out of a package, and fails at build time if it can't figure it out.

xaverdh · 2020-09-05T10:47:32Z

To clarify, the values in the tuple would be opaque strings, so maybe it should be
{ packageName = "..."; binaryName = "..."; }

xaverdh · 2020-09-05T10:55:33Z

@lheckemann by the way feel free to incorporate changes from here into your pr, if feel that it fits in nicely!

lheckemann · 2020-09-05T17:59:51Z

Yes, its somewhat unfortunate that we don't have a canonical way to get the binary contained in a package.

That's ill-defined, many packages (including the ones here) have multiple binaries. gzip for instance has lots of other tools, some convenience "aliases" (like gunzip = gzip -d, zcat = gzip -dc), others scripts that wrap gzip for convenience like zgrep and zless. Representing all these tools and their uses in nix is a massive bike shed (should this apply to "compressors" specifically? What about packages containing multiple compressors like xz, which has both xz and lzma, where xz can also behave like lzma? …) we really don't need to paint in this PR IMHO :D

{ packageName = "..."; binaryName = "..."; }

I think I prefer the versatility of the function, which can do whatever it want in the end, rather than being forced into the scheme of ${package}/bin/${binary} — what if a custom compressor from outside nixpkgs is used, or the desired package isn't at the top level of nixpkgs, or the compressor package has a nonstandard layout? (in decreasing order of likelihood :D )

I'll take a shot at implementing this (allowing both predefined compressors and a custom "escape hatch" function), as well as "merging" in the more sensible argument handling) in my PR sometime in the next few days, unless you have any objections/better ideas?

xaverdh · 2020-09-05T19:30:07Z

Yes, its somewhat unfortunate that we don't have a canonical way to get the binary contained in a package.

That's ill-defined [...]

Yes it is, it would be a partial function at best. Still better than hard coding "${package}/bin/binary" as people do all over the place currently I think. But that discussion belongs somewhere else.

{ packageName = "..."; binaryName = "..."; }

I think I prefer the versatility of the function, which can do whatever it want in the end, rather than being forced into the scheme of ${package}/bin/${binary} — what if a custom compressor from outside nixpkgs is used, or the desired package isn't at the top level of nixpkgs, or the compressor package has a nonstandard layout? (in decreasing order of likelihood :D )

Indeed your function is strictly more general. It should probably get some good commentary / docs though, otherwise I think it will be quite hard to use.

what if a custom compressor from outside nixpkgs is used

^^ That part still won't work when cross compiling though (unless I misunderstood). If the user provided function does not take the binary from the provided pkgs argument, its value will not depend on whether buildPackages or pkgs is passed to it, so cross compiling will break. Should be ok when not doing cross though.

I'll take a shot at implementing this (allowing both predefined compressors and a custom "escape hatch" function), as well as "merging" in the more sensible argument handling) in my PR sometime in the next few days, unless you have any objections/better ideas?

Sure, go ahead!

xaverdh · 2020-09-05T19:36:03Z

Ah one additional thing occurred to me when looking at the code again:
The default arguments (in case none are specified) for the friendly interface in the xz case should probably include --check=crc32, because the kernel requires it (the defaults won't work and lead to a non booting system). Maybe we should even unconditionally append that to the command line for xz..

xaverdh · 2020-09-05T19:43:34Z

On second thought the custom compressor from outside nixpkgs can actually be made to work with cross by inspecting the platform from pkgs.stdenv. So I guess my point above is moot.

lheckemann · 2020-09-08T07:33:39Z

On second thought the custom compressor from outside nixpkgs can actually be made to work with cross by inspecting the platform from pkgs.stdenv. So I guess my point above is moot.

Or by using (pkgs: pkgs.callPackage ./my-custom-compressor.nix {}) :)

ofborg bot added 6.topic: nixos 8.has: module (update) labels Aug 22, 2020

xaverdh force-pushed the initrd-cross-compression branch from ca03094 to 8f956f3 Compare August 22, 2020 17:29

lopsided98 suggested changes Aug 22, 2020

View reviewed changes

xaverdh force-pushed the initrd-cross-compression branch from 8f956f3 to f5914ce Compare August 22, 2020 19:44

bjornfor added the 6.topic: cross-compilation Building packages on a different sort platform than than they will be run on label Aug 22, 2020

initrd: cross compile fix

5fd9e5f

xaverdh force-pushed the initrd-cross-compression branch from f5914ce to 5fd9e5f Compare August 22, 2020 21:29

nixos/initrd: make compression options public

bd01069

xaverdh changed the title ~~WIP: initrd: cross compile fix~~ initrd: cross compile fix Aug 22, 2020

ofborg bot added 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 8.has: changelog 8.has: documentation labels Aug 23, 2020

release-notes: mention changes to makeInitrd and initrd module

c7510d4

xaverdh force-pushed the initrd-cross-compression branch from 9abe4cb to c7510d4 Compare August 23, 2020 08:14

xaverdh mentioned this pull request Aug 25, 2020

initrd: Make compression method an option NixOS/mobile-nixos#195

Merged

lheckemann reviewed Sep 4, 2020

View reviewed changes

lheckemann mentioned this pull request Sep 4, 2020

Initrd improvements #97145

Merged

4 tasks

ryantm added 2.status: merge conflict and removed 2.status: merge conflict labels Oct 3, 2020

lheckemann closed this in #97145 Dec 18, 2020

xaverdh deleted the initrd-cross-compression branch December 20, 2020 06:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initrd: cross compile fix #96005

initrd: cross compile fix #96005

xaverdh commented Aug 22, 2020 •

edited

lopsided98 left a comment

lopsided98 Aug 22, 2020

xaverdh Aug 22, 2020

xaverdh Aug 22, 2020

xaverdh Aug 22, 2020

lopsided98 Aug 22, 2020

lopsided98 commented Aug 22, 2020

xaverdh commented Aug 22, 2020

lopsided98 commented Aug 22, 2020

xaverdh commented Aug 22, 2020

xaverdh commented Aug 23, 2020

lheckemann left a comment

lheckemann Sep 4, 2020

lheckemann Sep 4, 2020

lopsided98 Sep 4, 2020

lheckemann Sep 4, 2020

lheckemann commented Sep 4, 2020

lopsided98 commented Sep 4, 2020

lheckemann commented Sep 4, 2020

lheckemann commented Sep 4, 2020

xaverdh commented Sep 5, 2020 •

edited

xaverdh commented Sep 5, 2020

xaverdh commented Sep 5, 2020

lheckemann commented Sep 5, 2020

xaverdh commented Sep 5, 2020 •

edited

xaverdh commented Sep 5, 2020 •

edited

xaverdh commented Sep 5, 2020

lheckemann commented Sep 8, 2020

initrd: cross compile fix #96005

initrd: cross compile fix #96005

Conversation

xaverdh commented Aug 22, 2020 • edited

Motivation for this change

Things done

lopsided98 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lopsided98 commented Aug 22, 2020

xaverdh commented Aug 22, 2020

lopsided98 commented Aug 22, 2020

xaverdh commented Aug 22, 2020

xaverdh commented Aug 23, 2020

lheckemann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lheckemann commented Sep 4, 2020

lopsided98 commented Sep 4, 2020

lheckemann commented Sep 4, 2020

lheckemann commented Sep 4, 2020

xaverdh commented Sep 5, 2020 • edited

xaverdh commented Sep 5, 2020

xaverdh commented Sep 5, 2020

lheckemann commented Sep 5, 2020

xaverdh commented Sep 5, 2020 • edited

xaverdh commented Sep 5, 2020 • edited

xaverdh commented Sep 5, 2020

lheckemann commented Sep 8, 2020

xaverdh commented Aug 22, 2020 •

edited

xaverdh commented Sep 5, 2020 •

edited

xaverdh commented Sep 5, 2020 •

edited

xaverdh commented Sep 5, 2020 •

edited