nixos/device-tree: Add more options to the module

[ktrinh-anduril]

Things done

Improve devicetree module by:

• Exposing compileDTS to make it easier for user to compile their own custom dts

• Expose more customization options for compiling dtbo in hardware.devicetree

• Allow custom dtb source in hardware.devicetree so that users don't have to use dtb from kernel but still get other benefits of the module like overlay support

• Built on platform(s)

  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin

• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)

• Tested, as applicable:

  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages

• 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/)

• 23.11 Release Notes (or backporting 23.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.

Tested by building with the various new options on the closure nixos/modules/installer/sd-card/sd-image-aarch64.nix closure. The dtb changes are observed to take effects

[nixos-discourse]

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/2676

[Majiir]

Could you share an example on how to use dtbSource properly? I tried to test this by replacing an expression in my config, and I'm having trouble getting it working.

I have dtbs/my-device-tree.dts in my Nix repo. I tried this config:

{
  hardware.deviceTree = {
    dtbSource = ./dtbs;
  };
}

but then result/dtbs/ contains only the source dts and no compiled dtbs.

[ktrinh-anduril]

Thank you for testing! The dtbSource actually points to a directory that should contain dtb not dts since it's used as a base to apply dtb overlays on. So to test it, the directory should contain dtbs/my-device-tree.dtb, the output should contain the dtb that has overlays applied.

[Majiir]

D'oh, thanks. It's been a while since I looked at my device tree configs.

compileDTS

I had a config that was patching the kernel source to include a .dts for board with includes from the kernel tree. Using this PR, I replaced that with this slightly shorter, cleaner config and ended up with identical dtbs:

hardware.deviceTree = {
  dtbSource = let
    kernelDts = pkgs.runCommand "kernelDts" {} ''
      mkdir -p "$out"
      tar xv \
        -f ${config.boot.kernelPackages.kernel.src} \
        -C "$out" \
        --strip-components=1 \
        "linux-${config.boot.kernelPackages.kernel.modDirVersion}/arch/arm/boot/dts"
    '';
    dtb = pkgs.deviceTree.compileDTS {
      name = "my-device-tree";
      dtsFile = ./my-device-tree.dts;
      includePaths = [
        "${lib.getDev config.boot.kernelPackages.kernel}/lib/modules/${config.boot.kernelPackages.kernel.modDirVersion}/source/scripts/dtc/include-prefixes"
        "${kernelDts}/arch/arm/boot/dts"
      ];
    };
  in pkgs.runCommandLocal "dtbs" {} ''
    mkdir -p "$out"
    cp ${dtb} "$out/my-device-tree.dtb"
  '';
};

So dtbSource and compileDTS are definitely useful. The expression above is still a bit clunky though, so either I'm doing something wrong or there's an opportunity to expand the options to handle dts that aren't overlays.

Other testing

• I built another config that has overlays, and everything worked fine.
• I didn't test dtboBuildExtraPreprocessorFlags.

Code review

• Changes LGTM.
• I was surprised to find that dtboBuildExtraIncludePaths was a top-level option rather than per-overlay. I don't know if there is a use case for configuring these per-overlay, though.
• I suggest squashing your last two commits into the first two.

[ktrinh-anduril]

Thank you for the detailed feedbacks! I added a dtsSource option that can be used instead of dtbSource, also moved the preprocessors_flag and extraIncludePaths options to be part of the overlay type

[Majiir]

I think there are two ways you could proceed with this PR:

1. The changes in the first four commits (through 88f3043) look good to me. You should just squash them down to 1-2 commits to make it ready to merge. Then, you could return to the dtsSource changes in a second PR.

2. You could keep going with the dtsSource changes, making the options more composable as discussed in my other comments.

I encourage you to go with option (1) so that it's easier to merge this change ahead of 23.11. If you do, feel free to ping me on a second PR for a review.

[Majiir]

This first commit (temporarily) breaks things because compileDTS is no longer in scope when it's used in withDTBOs. It also doesn't update the call to provide the right name. It looks like both of these are present in the next commit.

Generally speaking, splitting up commits like this is good for readability, but you should make sure that each commit can be taken independently. The tree at each commit should work without errors, pass tests, etc. You can use git rebase -i and similar commands to edit your earlier commits.

[ktrinh-anduril]

Gotcha, I will be more mindful in the future with my git commits.

[Majiir]

Is this restriction needed?

I see that it won't be useful to configure a filter when using dtsSource, since only one dtb will be produced anyway. Here are some reasons to make filter work with it anyway:

• A future change could allow multiple dtb to be built from sources.
• Conditional application makes options more confusing.
• The default filter is null anyway.

[Majiir]

Config options should compose rather than conflict with each other. It should be possible for a user to combine all of these options in a useful way.

For example, suppose I want to produce an image supporting four boards:

• Board 1 has a device tree in the Linux kernel. I want to use the DTB from the compiled kernelPackage.
• Board 2 needs a compiled dtb from the vendor. I want to include that with dtbSource.
• Board 3 has my own dts that I need to build. I would use dtsSource for this.
• Board 4 also comes from my own dts. I want to specify dtsSource multiple times.
• Then, I might want to apply a filter on top to build images tailored to each board.

As a user, I would expect this to "just work". Each option should build upon others. Something like this:

• multiple dtsSource -> custom dtbs tree
• custom dtbs tree + user-supplied dtbSource + ${kernelPackage}/dtbs -> combined dtb tree
• combined dtb tree + filter -> filtered dtb tree
• filtered dtb tree + overlays -> final dtb tree

All of these configs could have sensible defaults that can be overridden. But that override should be an explicit choice by the user and preferably not automatic behavior behind the scenes, even if it is documented.

For example, the default value of dtbSources could include both kernelPackages dtbs and dtbs compiled from dtsSource. If a user doesn't want to include dtbs from kernelPackages, they could set kernelPackages = null.

[ktrinh-anduril]

Gotcha! Thank you again for the detailed feedbacks! I have squashed the first 4 commits and will make another PR to wrap up the other feature.

[nixos-discourse]

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

https://discourse.nixos.org/t/prs-already-reviewed/2617/1117

[ktrinh-anduril]

@Majiir do you know when we can merge this in? Do I need to do anything else to move this along?

[Majiir]

It is ready to merge in my opinion, but I don't have merge access. We will need to wait for someone in the committers group.

[gador]

I'm getting the following error, and I guess it has to do with this PR..

error:
       … while calling the 'derivationStrict' builtin

         at /builtin/derivation.nix:9:12: (source not available)

       … while evaluating derivation 'nixos-system-kari-23.11.20231007.a1decd5'
         whose name attribute is located at /nix/store/6bs7cvmiiv6792bs2mbhigmr0jgaj9ma-source/pkgs/stdenv/generic/make-derivation.nix:300:7

       … while evaluating attribute 'buildCommand' of derivation 'nixos-system-kari-23.11.20231007.a1decd5'

         at /nix/store/6bs7cvmiiv6792bs2mbhigmr0jgaj9ma-source/nixos/modules/system/activation/top-level.nix:53:5:

           52|     passAsFile = [ "extraDependencies" ];
           53|     buildCommand = systemBuilder;
             |     ^
           54|

       (stack trace truncated; use '--show-trace' to show the full trace)

       error: attribute 'compileDTS' missing

       at /nix/store/6bs7cvmiiv6792bs2mbhigmr0jgaj9ma-source/nixos/modules/hardware/device-tree.nix:91:7:

           90|       in
           91|       pkgs.deviceTree.compileDTS {
             |       ^
           92|         name = "${o.name}-dtbo";

any pointers at where to look?

[RaitoBezarius]

Can you validate if nix repl <nixpkgs> can you give pkgs.deviceTree.compileDTS using the proper <nixpkgs> input you just upgraded to?

[gador]

yes it can:

> nix repl -I nixpkgs=/Users/gador/software/git/nixpkgs
nix-repl: :l <nixpkgs>
nix-repl> pkgs.deviceTree.compileDTS
«lambda @ /Users/gador/software/git/nixpkgs/pkgs/os-specific/linux/device-tree/default.nix:6:17»

[gador]

I'm not sure if its relevant, but I am cross compiling from an x86_64-linux to an aarch64-linux system. The nixpkgs is the same, though

[ktrinh-anduril]

hi, sorry to hear that you are running into issues. Is it possible that an older nixpkgs was used for pkgs when the device-tree module was eval?

[gador]

No, the nixpkgs used is throughout the same. But I figured, it has something to do with nixos-hardware:

(--show-trace)
[...] 
at /nix/store/6bs7cvmiiv6792bs2mbhigmr0jgaj9ma-source/lib/types.nix:493:15:

          492|       descriptionClass = "noun";
          493|       check = x: isStringLike x && builtins.substring 0 1 (toString x) == "/";
             |               ^
          494|       merge = mergeEqualOption;

       … while evaluating derivation 'device-tree-overlays'
         whose name attribute is located at /nix/store/6bs7cvmiiv6792bs2mbhigmr0jgaj9ma-source/pkgs/stdenv/generic/make-derivation.nix:300:7

       … while evaluating attribute 'buildCommand' of derivation 'device-tree-overlays'

         at /nix/store/b7xl2abnwi94znl0hpqk43z41m9l5spa-source/raspberry-pi/4/apply-overlays-dtmerge.nix:8:3:

            7|   nativeBuildInputs = [ dtc libraspberrypi ];
            8|   buildCommand = let
             |   ^
            9|     overlays = toList overlays';

       … from call site
[...]

The apply-overlays-dtmerge in question is this https://github.com/NixOS/nixos-hardware/blob/master/raspberry-pi/4/apply-overlays-dtmerge.nix

and the heading starts with:
https://github.com/NixOS/nixos-hardware/blob/bb2db418b616fea536b1be7f6ee72fb45c11afe0/raspberry-pi/4/apply-overlays-dtmerge.nix#L1-L2
which state

# modification of nixpkgs deviceTree.applyOverlays to resolve https://github.com/NixOS/nixpkgs/issues/125354
# derived from https://github.com/NixOS/nixpkgs/blob/916ca8f2b0c208def051f8ea9760c534a40309db/pkgs/os-specific/linux/device-tree/default.nix

so I guess the problem is the cooperation between the nixos-hardware raspberry-pi-4 and nixpkgs tree