Skip to content

Conversation

trueNAHO
Copy link
Member

@trueNAHO trueNAHO commented Jun 12, 2025

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

@stylix-automation stylix-automation bot added the topic: testbed Testbed changes label Jun 12, 2025
@trueNAHO trueNAHO added the backport: release-25.05 Changes to release-25.05 stable branch label Jun 12, 2025
@trueNAHO trueNAHO force-pushed the stylix-add-module-maintainers-to-corresponding-testbed-derivations branch from a6381e1 to 60dbddb Compare June 12, 2025 17:40
@trueNAHO
Copy link
Member Author

trueNAHO commented Jun 12, 2025

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 trueNAHO force-pushed the stylix-add-module-maintainers-to-corresponding-testbed-derivations branch from 60dbddb to f4a22f2 Compare June 12, 2025 18:40
Copy link
Contributor

@awwpotato awwpotato left a comment

Choose a reason for hiding this comment

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

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

@danth
Copy link
Collaborator

danth commented Jun 16, 2025

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

@trueNAHO
Copy link
Member Author

trueNAHO commented Jun 20, 2025

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.

@stylix-automation stylix-automation bot added the status: merge conflict Merge conflict label Jun 25, 2025
@trueNAHO trueNAHO force-pushed the stylix-add-module-maintainers-to-corresponding-testbed-derivations branch from f4a22f2 to ad5c46c Compare June 25, 2025 20:23
@stylix-automation stylix-automation bot removed the status: merge conflict Merge conflict label Jun 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport: release-25.05 Changes to release-25.05 stable branch topic: testbed Testbed changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants