stylix: add imports to mkTarget

[0xda157]

imports is necessary for renamed/removed options. Note that imports is "unsafe" and might cause failure to check != null (maybe a different way would be better?)

cc @trueNAHO @MattSturgeon @Flameopathic

Things done

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

Notify maintainers

[MattSturgeon]

Note that imports is an "unsafe" input and might cause failure to check != null

One approach would be to have mkTarget apply mkTarget to each import?

Or maybe that doesn't make sense because not all imports will need an enable option. If so, perhaps mkTarget should be split into a function for creating "whole" targets and another for creating "parts" of targets?

A "part" could have extra options, config elements, and imports. A "whole" target has those and also an enable option.

Although it'd be better if we don't need this complexity. I.e. if imports are only used for creating additional "sub" targets with an enable option.

[0xda157]

One approach would be to have mkTarget apply mkTarget to each import?

Or maybe that doesn't make sense because not all imports will need an enable option. If so, perhaps mkTarget should be split into a function for creating "whole" targets and another for creating "parts" of targets?

A "part" could have extra options, config elements, and imports. A "whole" target has those and also an enable option.

Although it'd be better if we don't need this complexity. I.e. if imports are only used for creating additional "sub" targets with an enable option.

I was think adding imports so we could use mkRenamedOption and mkRemovedOption, I don't think adding imports to import other full modules is necessary.

[MattSturgeon]

I was think adding imports so we could use mkRenamedOption and mkRemovedOption, I don't think adding imports to import other full modules is necessary.

Ah. That makes sense.

If you want to prevent importing arbitrary modules, but want to support renaming or removing options, the only way I can think of is by having mkTarget map some other list applying the relevant functions.

I.e. adding a renames attribute and a removed attribute to mkTarget's args.

However this won't scale well as you'd have to have a hard-coded attr for each function you wanted to support. E.g. mkChangedOptionModule, mkMergedOptionModule, mkAliasOptionModule, etc are used less often, but would need changes to mkTarget if you ever needed to use them.

---

You could have a more flexible design, by having a single option whose entries specify the function used:

mkTarget {
  modules = [
    {
      function = "mkRenamedOptionModule";
      apply = fn: fn [ "foo" "bar" ] [ "x" "y" ];
    }
  ];
}

Which is still kinda clunky, but provides a nice middle ground of not being too inflexible while still restricting to lib.modules.${function} functions.

[Flameopathic]

The way I envisioned this working was basically as follows:

{ mkTarget, lib, ... }:
{
  imports = [
    /* etc */
  ];
} // mkTarget { /* etc */ }

I don't see a reason to limit what people can import or make warnings for; are there any other reasons that this pattern would be undesirable (or possibly non-functional)?

[0xda157]

The way I envisioned this working was basically as follows:

{ mkTarget, lib, ... }:
{
  imports = [
    /* etc */
  ];
} // mkTarget { /* etc */ }

I don't see a reason to limit what people can import or make warnings for; are there any other reasons that this pattern would be undesirable (or possibly non-functional)?

Manually having to call mkTarget is bad ux and we should remove it once we've transferred all the modules to mkTarget (if they can all be transferred to mkTarget). Treating mkTarget as something you merge with the rest of the module seems really janky and undesirable in the mean time.

---

In my view the final form of mkTarget would be each module is either

{ lib, pkgs } -> MkTargetBody

or

{ lib, pkgs } -> [ MkTargetBody ]

e.g.

{ lib, pkgs }:
{
  name = "foo";
  humanName = "Foo";

  configElements = [ ];
}

or

{ lib, pkgs }:
map 
  (
    { name, humanName }:
    {
       inherit name humanName;
       
       configElements = [ ];
    }
  )
  [
    {
      name = "foo";
      humanName = "Foo";
    }
    {
      name = "bar";
      humanName = "Bar";
    }
  ]

[trueNAHO]

You could have a more flexible design, by having a single option whose entries specify the function used:

mkTarget {
  modules = [
    {
      function = "mkRenamedOptionModule";
      apply = fn: fn [ "foo" "bar" ] [ "x" "y" ];
    }
  ];
}

Which is still kinda clunky, but provides a nice middle ground of not being too inflexible while still restricting to lib.modules.${function} functions.

Yes, this seems like the easiest way to scale this, while ensuring imports being local to their specific mkTarget instance. Should the parameter be called imports instead of modules to make it more obvious what is being interfaced?

However this won't scale well as you'd have to have a hard-coded attr for each function you wanted to support. E.g. mkChangedOptionModule, mkMergedOptionModule, mkAliasOptionModule, etc are used less often, but would need changes to mkTarget if you ever needed to use them.

Although, this [{ function :: str, apply :: Fn }] pattern generally interfaces lib.modules.<FUNCTION>, there are not many functions that would have to be re-exported in mkTarget, and lib.modules.<FUNCTION> is very likely to receive substantial new functions only rarely. This would improve the API into something like the following (feel free to suggest improvements):

mkTarget {
  imports.mkRenamedOptionModule = [[[ "foo" "bar" ] [ "x" "y" ]]];
}

In my view the final form of mkTarget would be each module is either

{ lib, pkgs } -> MkTargetBody

or

{ lib, pkgs } -> [ MkTargetBody ]

e.g.

{ lib, pkgs }:
{
  name = "foo";
  humanName = "Foo";

  configElements = [ ];
}

or

{ lib, pkgs }:
map
  (
    { name, humanName }:
    {
       inherit name humanName;

       configElements = [ ];
    }
  )
  [
    {
      name = "foo";
      humanName = "Foo";
    }
    {
      name = "bar";
      humanName = "Bar";
    }
  ]

For reference, this differs from my proposal in #1341 (comment) by not treating "simple" modules as pure attribute sets.

[Flameopathic]

I don't see a reason to limit what people can import or make warnings for; are there any other reasons that this pattern would be undesirable (or possibly non-functional)?

Manually having to call mkTarget is bad ux and we should remove it once we've transferred all the modules to mkTarget (if they an all be transferred to mkTarget). Treating mkTarget as something you merge with the rest of the module seems really janky and undesirable in the mean time.

I see what you mean. In that case, I support this PR as-is, as I do not see a reason to limit what can be imported.

[trueNAHO]

I don't see a reason to limit what people can import or make warnings for; are there any other reasons that this pattern would be undesirable (or possibly non-functional)?

Manually having to call mkTarget is bad ux and we should remove it once we've transferred all the modules to mkTarget (if they an all be transferred to mkTarget). Treating mkTarget as something you merge with the rest of the module seems really janky and undesirable in the mean time.

I see what you mean. In that case, I support this PR as-is, as I do not see a reason to limit what can be imported.

Considering the ongoing mkTarget migration may require this functionality, I am fine interfacing imports for now and potentially polishing this in the future: #1363 (comment).

[stylix-automation]

Successfully created backport PR for release-25.05:

• [release-25.05] stylix: add imports to mkTarget #1426