stylix icon indicating copy to clipboard operation
stylix copied to clipboard

stylix: add module maintainers to corresponding testbed derivations

Open trueNAHO opened this issue 6 months ago • 3 comments

Regarding the performance comment about lib.recursiveUpdate, should we still use lib.recursiveUpdate since the script attribute set is fairly small? For reference, here is the relevant lib.recursiveUpdate implementation:

  /**
    Does the same as the update operator '//' except that attributes are
    merged until the given predicate is verified.  The predicate should
    accept 3 arguments which are the path to reach the attribute, a part of
    the first attribute set and a part of the second attribute set.  When
    the predicate is satisfied, the value of the first attribute set is
    replaced by the value of the second attribute set.

    # Inputs

    `pred`

    : Predicate, taking the path to the current attribute as a list of strings for attribute names, and the two values at that path from the original arguments.

    `lhs`

    : Left attribute set of the merge.

    `rhs`

    : Right attribute set of the merge.

    # Type

    ```
    recursiveUpdateUntil :: ( [ String ] -> AttrSet -> AttrSet -> Bool ) -> AttrSet -> AttrSet -> AttrSet
    ```

    # Examples
    :::{.example}
    ## `lib.attrsets.recursiveUpdateUntil` usage example

    ```nix
    recursiveUpdateUntil (path: l: r: path == ["foo"]) {
      # first attribute set
      foo.bar = 1;
      foo.baz = 2;
      bar = 3;
    } {
      #second attribute set
      foo.bar = 1;
      foo.quz = 2;
      baz = 4;
    }

    => {
      foo.bar = 1; # 'foo.*' from the second set
      foo.quz = 2; #
      bar = 3;     # 'bar' from the first set
      baz = 4;     # 'baz' from the second set
    }
    ```

    :::
  */
  recursiveUpdateUntil =
    pred: lhs: rhs:
    let
      f =
        attrPath:
        zipAttrsWith (
          n: values:
          let
            here = attrPath ++ [ n ];
          in
          if length values == 1 || pred here (elemAt values 1) (head values) then
            head values
          else
            f here values
        );
    in
    f [ ] [ rhs lhs ];

  /**
    A recursive variant of the update operator ‘//’.  The recursion
    stops when one of the attribute values is not an attribute set,
    in which case the right hand side value takes precedence over the
    left hand side value.

    # Inputs

    `lhs`

    : Left attribute set of the merge.

    `rhs`

    : Right attribute set of the merge.

    # Type

    ```
    recursiveUpdate :: AttrSet -> AttrSet -> AttrSet
    ```

    # Examples
    :::{.example}
    ## `lib.attrsets.recursiveUpdate` usage example

    ```nix
    recursiveUpdate {
      boot.loader.grub.enable = true;
      boot.loader.grub.device = "/dev/hda";
    } {
      boot.loader.grub.device = "";
    }

    returns: {
      boot.loader.grub.enable = true;
      boot.loader.grub.device = "";
    }
    ```

    :::
  */
  recursiveUpdate =
    lhs: rhs:
    recursiveUpdateUntil (
      path: lhs: rhs:
      !(isAttrs lhs && isAttrs rhs)
    ) lhs rhs;

-- Nixpkgs, /lib/attrsets.nix

The evaluation of all testbed maintainer values can be verified with the following nix repl command:

let
  system = "x86_64-linux";
in
  builtins.mapAttrs
  (_: module: module.meta.maintainers)
  outputs.packages.${system}

Things done

Notify maintainers

trueNAHO avatar Jun 12 '25 17:06 trueNAHO

Regarding the performance comment about lib.recursiveUpdate, should we still use lib.recursiveUpdate since the script attribute set is fairly small?

On second thought, I would prefer using lib.recursiveUpdate:

diff --git a/stylix/testbed/default.nix b/stylix/testbed/default.nix
index cb57c27..b4a0189 100644
--- a/stylix/testbed/default.nix
+++ b/stylix/testbed/default.nix
@@ -97,16 +97,8 @@ let
       };
     in
     lib.optionalAttrs (isEnabled testbed.path) {
-      # For performance reasons, the more convenient and generic
-      # lib.recursiveUpdate function is not used as follows:
-      #
-      #     lib.recursiveUpdate script {
-      #       meta.maintainers = meta.${testbed.module}.maintainers;
-      #     }
-      ${name} = script // {
-        meta = script.meta // {
-          inherit (meta.${testbed.module}) maintainers;
-        };
+      ${name} = lib.recursiveUpdate script {
+        meta.maintainers = meta.${testbed.module}.maintainers;
       };
     };

trueNAHO avatar Jun 12 '25 17:06 trueNAHO

Is it not possible to pass meta directly to pkgs.writeShellApplication?

danth avatar Jun 16 '25 14:06 danth

doesn't really seem necessary imo, but I also don't see any downside

Seems like a nice addition.

Is it not possible to pass meta directly to pkgs.writeShellApplication?

This would imply being a maintainer of the script, which is not the intention. If

${name} = script;

is ever extended to be something like

${name} = pkgs.buildEnv {
  inherit name;
  paths = [script /* Filesystem of the testbed */];
};

it would silently work not as intended.

Actually, I stumbled accross the maintainer addition when trying to add the testbed's filesystem under result/${testbed.name}/, while keeping the script under result/bin/${script.meta.mainProgram}. The code ended up being very complicated with not much benefit because the testbed's filesystem can be somewhat inconveniently inspected inside nix run .#testbed:*:*. So I threw that code away and just kept the meta.maintainer addition.

trueNAHO avatar Jun 20 '25 14:06 trueNAHO