stylix: improve how discord testbed is disabled

[MattSturgeon]

Note

This PR includes some commits that do not make sense to be squashed, such as slightly off-topic cleanup and refactoring.

• stylix: use concatMapStringSep in testbed.nix
• stylix: use concatMap in testbeds.nix
• stylix: refactor textbed.nix autoload
• stylix: add testbed enable option
• stylix: use a cheaper module eval for the enable option
• discord: use new enable option for testbed

Summary

I've added a stylix.testbed.enable option to testbeds.

When set to false, the testbed is not included in the output of stylix/testbed.nix.

enable

Whether to enable this testbed.

The testbed will not be included as a flake output if set to false;

[!CAUTION]

This option's value can only evaluate lib and pkgs inputs,
if it reads other inputs, the testbed will fail to evaluate;
e.g., config or options.

This restriction was made for performance reasons.
See isEnabled.

Type: boolean
Default: true
Example: lib.meta.availableOn pkgs.stdenv.hostPlatform pkgs.discord

Discord

This is used to make the discord testbed conditional:

# Discord is not available on arm64.
stylix.testbed.enable = lib.meta.availableOn pkgs.stdenv.hostPlatform package;

Performance

I found in testing that needing to evaluate the testbed system configurations is somewhat expensive, which was causing nix flake show to hang as it needed to read the enable option from each testbed before knowing what flake outputs exist.

I've solved this by adding a second, much cheaper minimal configuration, which only evals the testbed module without any nixos modules. This has plenty of downsides, but we're unlikely to run into any of them here.

Potential future issues

The most likely issue would be a testbed module requesting a non-standard module arg, such as osConfig. This could be solved by adding some stub args to the miminal eval's _module.args.

Another issue may be the enable option value depending on an overlayed pkgs instance. This could be solved by declaring the nixpkgs.overlays option, and applying it to _module.args.pkgs.

It is also possible that the enable option would want to depend on another config value usually present in a system configuration. This cannot be supported without massive performance hits.

However given that you currently only use this option for one module, and it will be rare to use it in others, the chances of these restrictions causing problems seem low.

Things done

• Tested locally
• Tested in testbed
• Commit message follows commit convention
• Fits style guide
• Respects license of any existing code used

Notify maintainers

• Discord: @Flameopathic

[0xda157]

LGTM, @Flameopathic should also review this b/c they refactored testbeds recently

[trueNAHO]

LGTM, except for nitpicks and minor comments.

From 4a88b6d4ac040fb826b7204ef544fb28df87cb91 Mon Sep 17 00:00:00 2001
From: Matt Sturgeon <[email protected]>
Date: Sat, 17 May 2025 00:42:10 +0100
Subject: [PATCH 1/6] stylix: use `concatMapStringSep` in testbed.nix

---
 stylix/testbed.nix | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/stylix/testbed.nix b/stylix/testbed.nix
index 833da4875..7572aeb6b 100644
--- a/stylix/testbed.nix
+++ b/stylix/testbed.nix
@@ -189,8 +189,8 @@ let
   makeTestbed =
     testbed: stylix:
     let
-      name = builtins.concatStringsSep testbedFieldSeparator (
-        map
+      name =
+        lib.concatMapStringsSep testbedFieldSeparator
           (
             field:
             lib.throwIf (lib.hasInfix testbedFieldSeparator field)
@@ -205,8 +205,7 @@ let
             "image${lib.optionalString (stylix.image or null == null) "less"}"
             "scheme${lib.optionalString (stylix.base16Scheme or null == null) "less"}"
             "cursor${lib.optionalString (stylix.cursor or null == null) "less"}"
-          ]
-      );
+          ];

       system = lib.nixosSystem {
         inherit (pkgs) system;

From e0e9d70af531dabecf121258b1279bdd8d05eb0b Mon Sep 17 00:00:00 2001
From: Matt Sturgeon <[email protected]>
Date: Sat, 17 May 2025 00:45:44 +0100
Subject: [PATCH 2/6] stylix: use `concatMap` in testbeds.nix

---
 stylix/testbed.nix | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/stylix/testbed.nix b/stylix/testbed.nix
index 7572aeb6b..b5e5a2842 100644
--- a/stylix/testbed.nix
+++ b/stylix/testbed.nix
@@ -305,5 +305,5 @@ in
 # Testbeds are merged using lib.attrsets.unionOfDisjoint to throw an error if
 # testbed names collide.
 builtins.foldl' lib.attrsets.unionOfDisjoint { } (
-  lib.flatten (map makeTestbeds autoload)
+  builtins.concatMap makeTestbeds autoload
 )

Thanks for improving the code elegance.

From 66356733bd1b6fdd92dff00c2cbfbd3de223f6b8 Mon Sep 17 00:00:00 2001
From: Matt Sturgeon <[email protected]>
Date: Sat, 17 May 2025 00:57:46 +0100
Subject: [PATCH 3/6] stylix: refactor textbed.nix autoload

Use `lib.pipe` and `builtins.concatMap`
---
 stylix/testbed.nix | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/stylix/testbed.nix b/stylix/testbed.nix
index b5e5a2842..7e924cecd 100644
--- a/stylix/testbed.nix
+++ b/stylix/testbed.nix
@@ -158,11 +158,16 @@ let
       directory = "testbeds";
       modules = "${inputs.self}/modules";
     in
-    lib.flatten (
-      lib.mapAttrsToList (
-        module: _:
+    lib.pipe modules [
+      builtins.readDir
+      builtins.attrNames
+      (builtins.concatMap (
+        module:
         let
           testbeds = "${modules}/${module}/${directory}";
+          files = lib.optionalAttrs (builtins.pathExists testbeds) (
+            builtins.readDir testbeds
+          );
         in
         lib.mapAttrsToList (
           testbed: type:
@@ -182,9 +187,9 @@ let
               name = lib.removeSuffix ".nix" testbed;
               path = "${testbeds}/${testbed}";
             }
-        ) (lib.optionalAttrs (builtins.pathExists testbeds) (builtins.readDir testbeds))
-      ) (builtins.readDir modules)
-    );
+        ) files
+      ))
+    ];

   makeTestbed =
     testbed: stylix:

Although, I really like pipes in general, I never use lib.pipe or its |> successor because I am happily waiting for [RFC 0148] Pipe operator to land. I am fine using lib.pipe, but I assume enabling the experimental pipe-operators feature would be bad.

From 99b7cad19123ba8ed4348bf19baca249935517cb Mon Sep 17 00:00:00 2001
From: Matt Sturgeon <[email protected]>
Date: Sat, 17 May 2025 01:28:24 +0100
Subject: [PATCH 4/6] stylix: add testbed enable option

When disabled, the testbed will not be used.
---
 stylix/testbed.nix | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/stylix/testbed.nix b/stylix/testbed.nix
index 7e924cecd..5e57d948b 100644
--- a/stylix/testbed.nix
+++ b/stylix/testbed.nix
@@ -37,6 +37,21 @@ let
       };
     };

+  enableModule =
+    { lib, ... }:
+    {
+      options.stylix.testbed.enable = lib.mkOption {
+        type = lib.types.bool;
+        default = true;
+        example = lib.literalExpression "lib.meta.availableOn pkgs.stdenv.hostPlatform pkgs.discord";
+        description = ''
+          Whether to enable this testbed.
+
+          The testbed will not be included as a flake output if set to false;

I think you used the wrong terminal character:

-          The testbed will not be included as a flake output if set to false;
+          The testbed will not be included as a flake output if set to false.

+        '';
+      };
+    };
+
   applicationModule =
     { config, lib, ... }:
     {
@@ -217,6 +232,7 @@ let

         modules = [
           commonModule
+          enableModule
           applicationModule
           inputs.self.nixosModules.stylix
           inputs.home-manager.nixosModules.home-manager
@@ -229,6 +245,8 @@ let
         ];
       };

+      inherit (system.config.stylix.testbed) enable;
+
       script = pkgs.writeShellApplication {
         inherit name;
         text = ''
@@ -248,7 +266,7 @@ let
         '';
       };
     in
-    {
+    lib.optionalAttrs enable {
       ${name} = script;
     };

LGTM.

From 4aa36f0babe652f27535d036529b24f9955685f6 Mon Sep 17 00:00:00 2001
From: Matt Sturgeon <[email protected]>
Date: Sat, 17 May 2025 02:03:30 +0100
Subject: [PATCH 5/6] stylix: use a cheaper module eval for the `enable` option

The enable option must be evaluated for things like `nix flake show`, as
it determines which testbeds are included in the docs.

We can improve performance by skipping all the NixOS and home-manager
warnings and assertions that a system configuration usually evaluates.

We can further improve performance by not even importing the NixOS or
home-manager modules at all! This has some downsides; the `enable`
option cannot read `config` for example, and overlays will not be
applied to `pkgs`.

But it is worth it for the performance improvement when evaluating the
flake outputs.
---

This explanation seems to be a summary from your cover letter:

I found in testing that needing to evaluate the testbed system configurations is somewhat expensive, which was causing nix flake show to hang as it needed to read the enable option from each testbed before knowing what flake outputs exist.

I've solved this by adding a second, much cheaper minimal configuration, which only evals the testbed module without any nixos modules. This has plenty of downsides, but we're unlikely to run into any of them here.

Potential future issues

The most likely issue would be a testbed module requesting a non-standard module arg, such as osConfig. This could be solved by adding some stub args to the miminal eval's _module.args.

Another issue may be the enable option value depending on an overlayed pkgs instance. This could be solved by declaring the nixpkgs.overlays option, and applying it to _module.args.pkgs.

It is also possible that the enable option would want to depend on another config value usually present in a system configuration. This cannot be supported without massive performance hits.

However given that you currently only use this option for one module, and it will be rare to use it in others, the chances of these restrictions causing problems seem low.

Although commit messages should be concise, I am unsure how much we care about them being short. Either way, it would be great to see a maybe slightly adapted version of this excellent explanation in the commit message.

 stylix/testbed.nix | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/stylix/testbed.nix b/stylix/testbed.nix
index 5e57d948b..515edbb89 100644
--- a/stylix/testbed.nix
+++ b/stylix/testbed.nix
@@ -48,6 +48,15 @@ let
           Whether to enable this testbed.

           The testbed will not be included as a flake output if set to false;
+
+          > [!CAUTION]
+          >
+          > This option's value can only evaluate `lib` and `pkgs` inputs,
+          > if it reads other inputs, the testbed will fail to evaluate;
+          > e.g., `config` or `options`.
+          >
+          > This restriction was made for performance reasons.
+          > See `isEnabled`.

This can be wrapped at 80 characters, aligning with commit 2fbe00a ("doc: wrap to width of 80 (#1262)").

-          > This option's value can only evaluate `lib` and `pkgs` inputs,
-          > if it reads other inputs, the testbed will fail to evaluate;
-          > e.g., `config` or `options`.
-          >
-          > This restriction was made for performance reasons.
-          > See `isEnabled`.
+          > This option's value can only evaluate `lib` and `pkgs` inputs, if it
+          > reads other inputs, the testbed will fail to evaluate; e.g.,
+          > `config` or `options`.
+          >
+          > This restriction was made for performance reasons. See `isEnabled`.

         '';
       };
     };
@@ -168,6 +177,26 @@ let
       };
     };

+  # Creates a minimal configuration to extract the `stylix.testbed.enable`
+  # option value.
+  #
+  # This is for performance reasons. Primarily, to avoid fully evaluating
+  # testbed system configurations to determine flake outputs.
+  # E.g., when running `nix flake show`.

This can also be wrapped:

-  # This is for performance reasons. Primarily, to avoid fully evaluating
-  # testbed system configurations to determine flake outputs.
-  # E.g., when running `nix flake show`.
+  # This is for performance reasons. Primarily, to avoid fully evaluating
+  # testbed system configurations to determine flake outputs. E.g., when running
+  # `nix flake show`.

+  isEnabled =
+    module:
+    let
+      minimal = lib.evalModules {
+        modules = [
+          module
+          enableModule
+          { _module.check = false; }
+          { _module.args = { inherit pkgs; }; }
+        ];
+      };
+    in
+    minimal.config.stylix.testbed.enable;
+
   autoload =
     let
       directory = "testbeds";
@@ -245,8 +274,6 @@ let
         ];
       };

-      inherit (system.config.stylix.testbed) enable;
-
       script = pkgs.writeShellApplication {
         inherit name;
         text = ''
@@ -266,7 +293,7 @@ let
         '';
       };
     in
-    lib.optionalAttrs enable {
+    lib.optionalAttrs (isEnabled testbed.path) {
       ${name} = script;
     };

Looks great!

From 572ab0e0df73210481ff1cb2735aef536831fcae Mon Sep 17 00:00:00 2001
From: Matt Sturgeon <[email protected]>
Date: Sat, 17 May 2025 01:31:21 +0100
Subject: [PATCH 6/6] discord: use new enable option for testbed

---
 flake/packages.nix                   | 50 +++++++++-------------------
 modules/discord/testbeds/vencord.nix | 11 ++++--
 2 files changed, 24 insertions(+), 37 deletions(-)

diff --git a/flake/packages.nix b/flake/packages.nix
index 055c7cafd..c06d62db9 100644
--- a/flake/packages.nix
+++ b/flake/packages.nix
@@ -7,46 +7,28 @@
 {

   perSystem =
-    {
-      pkgs,
-      system,
-      config,
-      ...
-    }:
+    { pkgs, config, ... }:
     {
       # Build all packages with 'nix flake check' instead of only verifying they
       # are derivations.
       checks = config.packages;

-      packages =
-        let
-          testbedPackages = import "${self}/stylix/testbed.nix" {
+      packages = lib.mkMerge [
+        # Testbeds are virtual machines based on NixOS, therefore they are
+        # only available for Linux systems.
+        (lib.mkIf pkgs.stdenv.hostPlatform.isLinux (
+          import "${self}/stylix/testbed.nix" {
             inherit pkgs inputs lib;
-          };
-
-          # Discord is not available on arm64. This workaround filters out
-          # testbeds using that package, until we have a better way to handle
-          # this.
-          testbedPackages' =
-            if system == "aarch64-linux" then
-              lib.filterAttrs (
-                name: _: !lib.hasPrefix "testbed:discord:vencord" name
-              ) testbedPackages
-            else
-              testbedPackages;
-        in
-        lib.mkMerge [
-          # Testbeds are virtual machines based on NixOS, therefore they are
-          # only available for Linux systems.
-          (lib.mkIf pkgs.stdenv.hostPlatform.isLinux testbedPackages')
-          {
-            docs = pkgs.callPackage "${self}/docs" {
-              inherit inputs;
-              inherit (inputs.nixpkgs.lib) nixosSystem;
-              inherit (inputs.home-manager.lib) homeManagerConfiguration;
-            };
-            palette-generator = pkgs.callPackage "${self}/palette-generator" { };
           }
-        ];
+        ))
+        {
+          docs = pkgs.callPackage "${self}/docs" {
+            inherit inputs;
+            inherit (inputs.nixpkgs.lib) nixosSystem;
+            inherit (inputs.home-manager.lib) homeManagerConfiguration;
+          };
+          palette-generator = pkgs.callPackage "${self}/palette-generator" { };
+        }
+      ];
     };
 }
diff --git a/modules/discord/testbeds/vencord.nix b/modules/discord/testbeds/vencord.nix
index f090acfd0..411681606 100644
--- a/modules/discord/testbeds/vencord.nix
+++ b/modules/discord/testbeds/vencord.nix
@@ -6,9 +6,14 @@ let
   };
 in
 {
-  stylix.testbed.ui.application = {
-    name = "discord";
-    inherit package;
+  stylix.testbed = {
+    # Discord is not available on arm64.
+    enable = lib.meta.availableOn pkgs.stdenv.hostPlatform package;
+
+    ui.application = {
+      name = "discord";
+      inherit package;
+    };
   };

   environment.systemPackages = [ package ];

This is much better. Thanks.

[MattSturgeon]

Although, I really like pipes in general, I never use lib.pipe or its |> successor because I am happily waiting for [RFC 0148] Pipe operator to land. I am fine using lib.pipe

I find lib.pipe can often make code more readable, especially deeply-nested function application. But also the pattern of using many intermediate variables, where you end up with intermediate binding names like listOfFoo, filteredListOfFoo, foosMappedToBars, finalList, etc.

When the |> operator becomes stable that'll help even more, but even without that lib.pipe is great IMO.

I assume enabling the experimental pipe-operators feature would be bad.

Yes, in a publicly consumed project. In your personal repos it should be fine to use experimental features.

[trueNAHO]

From 781e9090709466c45e30ecbc8e4031aaa589f44f Mon Sep 17 00:00:00 2001
From: Matt Sturgeon <[email protected]>
Date: Sat, 17 May 2025 02:03:30 +0100
Subject: [PATCH 5/6] stylix: use a cheaper module eval for the `enable` option

The `stylix.testbed.enable` option is needed to determine which testbeds
to include in flake outputs. However evaluating a NixOS system is
expensive, and significantly impacts the performance of `nix flake show`.

We work around this issue by adding minimal configuration, which
evaluates the testbed module on its own, without any nixos modules.

This has plenty of downsides, but we're unlikely to run into any of them
here.

Potential future issues:

1. A testbed module requiring a non-standard module arg.
   This could be solved by adding stubs to `_module.args`.

2. The enable option depending on an overlayed `pkgs` instance.
   This could be solved by implementing the `nixpkgs.overlays` option.

3. The enable option depending on another Stylix or NisOS option.

This is a typo:

-3. The enable option depending on another Stylix or NisOS option.
+3. The enable option depending on another Stylix or NixOS option.

I will just be merge this as-is, as this should not be a big deal and everyone should know that "NixOS" was meant.

[trueNAHO]

I accidentally squash merged this patchset... I will revert this commit and do a proper merge commit.