lib/modules: Add mkPostMerge

[babbaj]

Motivation for this change

NixOS provides the ability to forcibly set the value of an option and add to the value value of option but what is missing is a way to remove from an option's value. Currently the only way to achieve the same thing is with mkOverride but that is impractical if the module that is being overriden is non trivial. This change adds a new property that allows you to to provide an arbitrary function that gets applied to the merged value.
From my config the use of it looks like this:

qemu.options = lib.mkPostMerge (lib.remove "-device virtio-keyboard")
  [
    "-nographic"
    "-cpu host"
    "-device vfio-pci,host=0000:28:00.0"
    "-device vfio-pci,host=0000:28:00.1"
    "-device vfio-pci,host=0000:28:00.2"
    "-device vfio-pci,host=0000:28:00.3"
  ];

Things done

• 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/)
• 22.05 Release Notes (or backporting 21.11 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
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[pennae]

not sure it's a good idea to have this without also having postmerge functions be explicitly orderable (as if by mkOrder). also very much not a fan of the proliferation of ' in mergeDefinitions, that's a footgun waiting to go off at someone.

[babbaj]

I considered making it strictly ordered but I'm not sure if the extra complexity is worth it

[K900]

One solution might be to just make it an error to have more than one postMerge, and worry about ordering if it starts causing issues.

[infinisil]

This sounds like #155903 from @roberth

[babbaj]

Looks like a possibly nicer solution that works well with mkOverride but it doesn't provide a way to provide a normal definition along with with function. I think I'll try to rewrite mine to work in a similar way.

[babbaj]

I think enforcing strictly defined ordering would just end up creating a worse problem than this solves. If multiple modules make use of this feature on the same option then what will happen is that the config will refuse to be evaluated with no way for the user to solve it when it would most likely be perfectly fine to apply the multiple functions in any order. The only case I can see where multiple uses of this feature could be a problem is if one module used it to add to an option but that would just be an unnecessary misuse of the feature. I think there is no good solution to the problem and it's best to just allow the functions to be applied in the order they appear.

[roberth]

I think enforcing strictly defined ordering would just end up creating a worse problem than this solves.

I disagree. Without a strictly defined ordering, it's not safe to change the order of imports and won't allow us to fix the weird order in which definitions are currently processed. Some things are getting reversed, as of now.
Allowing such entropy into the module system will make it much more fragile.

If multiple modules make use of this feature

This is what we shouldn't do. mkPostMerge / mkTransform do not compose well by their very nature.

it would most likely be perfectly fine to apply the multiple functions in any order.

This would indicate that we don't need the power of functions, but something weaker instead, such as the merging we have or any other commutative operation. Composition of arbitrary functions is not commutative.

I think there is no good solution to the problem and it's best to just allow the functions to be applied in the order they appear.

No, it breaks the goals of the module system. The solution is no good when it just allows the functions to be applied in any order they appear.

When I wrote my version of it, what I had in mind is that end users would use it in cases where they want to reuse the default, as the main use case, then generalize it to other valid priority combinations. Although it is a semantically valid solution, it steepens the learning curve when users start writing their own modules, or modules for Nixpkgs, where this feature must not be used, as it makes such modules harder to reuse and combine with other modules that may or may not use it.

Instead, we should improve the modules that would invite the use of this feature, because by adding this feature, we allow those hard to use options to remain unfixed, moving both NixOS and the module system in a direction that's more fragile and complex, and less composable/reusable.

[roberth]

I invite anyone to come up with a real-world use case for this function that isn't better solved by improving the options.

Unless we find one, this is a bad feature. (I wish it was different, or that it was obvious for that matter)

[K900]

I think the primary use case most people run into is removing a thing from a list without redefining the entire list.

[roberth]

I think the primary use case most people run into is removing a thing from a list without redefining the entire list.

Removals are also bad, but for another reason. It's only commutative if you consider the option to consist of two values, a positive and a negative list, that only combine at merge time. When implemented this way, it avoids some of the composability argument, but, it still creates a bad coupling between modules (where a configuration is also a module).

It's better to teach the original module to insert less. That way, the coupling is expressed in high-level terms, making clear why things are the way they are in the original module.
For example an option virtualisation.qemu.enableVirtIOKeyboard could be added to solve the problem in the PR description. This has the additional benefit of documenting your use case and allowing the module to take into account what needs to be done for your use case when things change in the future. For instance, the module may need to add another virtual keyboard related option that doesn't work for you. Chances are good that the author of that change will add it inside the enableVirtIOKeyboard conditional, so you won't have to debug that in the future.

[babbaj]

Opening a pull request and waiting for it to get merged is a very inconvenient short term solution. It should be possible to get my configuration to how I need it within the module/config system without copy/pasting an entire module with one small change or using a weird hack. The solution doesn't need to compose well and I think it would even be fine if it was limited to just the main configuration.

[infinisil]

I've argued for this before, but I think this problem is best solved by deprecating types.listOf and others like it. We'd replace them with better alternatives. E.g. instead of types.listOf types.str it would be types.attrsOf types.bool. We could also have a types.stringSet, which would work the same way but also automatically transform [ "foo" "bar" ] into { foo = true; bar = true; } for convenienc. This removes two problems in one go:

• Makes module order not influence definitions anymore
• Allows people to remove definitions with { foo = false; }

[Majiir]

I invite anyone to come up with a real-world use case for this function that isn't better solved by improving the options.

@roberth I find a need to modify configuration values produced by a module whenever I am patching a NixOS issue in my system configuration, but either a PR hasn't been merged yet or a general solution is not yet known. I think these examples meet your criteria because "improving the options" requires that a PR is merged to do that. Leaving our systems in a broken state while we wait for a PR to be merged or authored is not a good option.

Example 1: Removing a kernel module

I have an ARM system that could not boot build because of the issue described in #207969. A NixOS module adds entries to boot.initrd.availableKernelModules that need to be made conditional instead. This PR has not yet merged, but one of the great features of NixOS is that I can incorporate it into my system anyway, without forking nixpkgs. Let's take a look at my patch module:

# Make TPM2 modules optional.
# See: https://github.com/NixOS/nixpkgs/pull/207969
{ nixpkgs, lib, config, pkgs, utils, ... }:
{
  # Mask the original module.
  disabledModules = [
    "system/boot/systemd/initrd.nix"
  ];

  # Reimport the module so that we can modify its config.
  imports = let
    module = (import "${nixpkgs}/nixos/modules/system/boot/systemd/initrd.nix" { inherit config lib pkgs utils; });
  in [
    # Remove tpm-tis and tpm-crb from availableKernelModules.
    (lib.recursiveUpdate module {
      # The 'content' in the path refers to a mkIf.
      config.content.boot.initrd.availableKernelModules = lib.remove "tpm-tis" (lib.remove "tpm-crb" module.config.content.boot.initrd.availableKernelModules);
    })
  ];

  # Add them back conditionally.
  boot.initrd.availableKernelModules = 
    lib.optional (config.boot.kernelPackages.kernel.config.isEnabled "TCG_TIS") "tpm-tis" ++
    lib.optional (config.boot.kernelPackages.kernel.config.isEnabled "TCG_CRB") "tpm-crb";
}

Some things that feel hacky:

• The responsible module has to be identified.
• The responsible module has to be disabled and manually reimported.
• The overridden attribute path includes .content to account for a mkIf in the implementation.

This is a little better than maintaining a fork, but it's brittle.

Example 2: Fixing a boot script

I found that boot.initrd.systemd and boot.loader.generationsDir are currently incompatible (#209964). To fix it on my system, I modified the script option of a systemd service. But this fix does not work in the general case, so I cannot expect it to be merged yet. Here it is:

# Fixes application of device tree overlays.
# TODO: See: https://github.com/NixOS/nixpkgs/pull/209964
{ config, lib, ... }:
{
  boot.initrd.systemd.services.initrd-nixos-activation.script = let
    cfg = config.boot.initrd.systemd;
  in lib.mkForce ''
    set -uo pipefail
    export PATH="/bin:${cfg.package.util-linux}/bin"

    # Figure out what closure to boot
    closure=
    for o in $(< /proc/cmdline); do
        case $o in
            init=*)
                IFS== read -r -a initParam <<< "$o"
                closure="$(dirname "$(readlink -m "/sysroot''${initParam[1]}")")"
                ;;
        esac
    done

    # Sanity check
    if [ -z "''${closure:-}" ]; then
      echo 'No init= parameter on the kernel command line' >&2
      exit 1
    fi

    # If we are not booting a NixOS closure (e.g. init=/bin/sh),
    # we don't know what root to prepare so we don't do anything
    if ! [ -x "/sysroot$closure/prepare-root" ]; then
      echo "NEW_INIT=''${initParam[1]}" > /etc/switch-root.conf
      echo "$closure does not look like a NixOS installation - not activating"
      exit 0
    fi
    echo 'NEW_INIT=' > /etc/switch-root.conf


    # We need to propagate /run for things like /run/booted-system
    # and /run/current-system.
    mkdir -p /sysroot/run
    mount --bind /run /sysroot/run

    # Initialize the system
    export IN_NIXOS_SYSTEMD_STAGE1=true
    exec chroot /sysroot $closure/prepare-root
  '';
}

This has a few problems:

• To change one line, I had to copy the entire script.
• Doing this requires reproducing every expression that's used in the value. In this case, the only expression used is cfg, but you can imagine a situation where a complex derivation is referenced in a part of the script.
• You can't tell at a glance what's been modified.
• If I update my system and this script has changed in nixpkgs, this module will quietly discard those changes.

Option modifiers

To sidestep the ordering issue, we can give up the full power of functions, as mentioned earlier on this PR.

Example 1 could be addressed by introducing a function to remove list elements.

Example 2 could be improved by introducing a function to apply a patch file to an option value. This would strip the patch module down to only the operative change, and it would solve the problem of nixpkgs updates being clobbered.

Module patches

However, patching an option value would break in situations where part of the diff contains an expression, like a patch to delete a line that contains Nix store path. In that situation, we are more interested in applying a patch to the module source itself. That capability would solve for both examples. Would such a patch approach partially solve the ordering issue?

[Majiir]

It looks like NixOS/nix#3920 could also address these use cases (in a similar way as "module patches").

[pennae]

Example 1: Removing a kernel module

this could be solved much more elegantly with a proposal from @infinisil a while back: just don't string lists, use attrsets. list elements become keys, true/false become "present" vs "not present". you'd write boot.initrd.availableKernelModules.tpm-tis = mkForce false, which composes a lot better.

Example 2: Fixing a boot script

adding some sort of patch functionality that is in any way resilient against unrelated modifications requires IFD and is thus not viable. what we could do though is to move all scripts that are currently defined inline in modules into files. once that's done you can redefine the script as being a derivation built from the stock script and a real patch. doing that would also give us the opportunity to clean up many (lack of) shell-escaping crimes.

[ncfavier]

Since I don't see it here, I'll mention that I've been using

boot.supportedFilesystems = mkOption {
  apply = subtractLists [ "reiserfs" "xfs" "cifs" "f2fs" ];
};

for ages to work around this issue. Of course, this is nothing more than a work-around, and supportedFilesystems is a clear example of an option that would benefit from the suggested "set of strings" type. Such a type could even accept lists as inputs to make the transition smoother, converting them with l: genAttrs l (_: true).

---

I don't think it's reasonable to expect to be able to use the module system to apply arbitrary patches (in the sense of git) to modules, and I'm inclined to put "changes to scripts" in that category. Note that the lazy-trees branch of Nix is exciting in this regard since it allows applying patches to flake inputs.

[nixos-discourse]

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

https://discourse.nixos.org/t/natural-deduction-rules-for-the-nixos-module-merge/26513/6

[roberth]

This does keep popping up and I'm still ambivalent about mkPostMerge.
If we do end up accepting this feature, we'll probably want to sort them by mkOrder order and handle mkOverride as well.

lib.mkMerge [
  (mkPostMerge (mkAfter (x: x + 1)))
  (mkPostMerge (x: x * 100))
  3
]
# => 301 

As with anything that supports mkOrder, we should check that the definitions are not susceptible to imports ordering. (Like we don't for lists, overlays, etc, all of which may be ordered reproducibly but arbitrarily, and unpredictably under superficial changes in configuration; a whole other can of worms)

[Majiir]

For what it's worth, I found a much nicer solution for my use cases: patching the nixpkgs input in flake.nix before building the system. This uses patchChannel from flake-utils-plus with a little wrapper to make it work nicely with nixosSystem:

let
  patches = [
    ./fix-something.patch
    # can also use fetchpatch
  ];
  patchNixpkgs = system: input: patches:
    let
      source = flake-utils-plus.lib.patchChannel system input patches;
      flake = import "${source}/flake.nix";
      outputs = flake.outputs { self = patched; };
      patched = outputs // {
        inherit outputs;
        outPath = "${source}";
      };
    in patched;
  patchedNixpkgs = patchNixpkgs "x86_64-linux" nixpkgs patches;
in
  patchedNixpkgs.lib.nixosSystem {
    modules = [
      # ordinary system modules here
    ];
  }

It works great! Before 23.05, I had something like 30 patches pulled from unstable on top of 22.11. I still have a few.

[roberth]

30 patches pulled from unstable on top of 22.11

Yikes. We should backport patches! Feel free to use the backport label or ask maintainers to do it if you don't have permission.
I surely hope the majority of those were suitable for backporting.

patchNixpkgs

For future readers, proof of concept code (NixOS/nix#6530) exists to add patch support to flake inputs natively.