doc: use stricter, more minimal module evals

[MattSturgeon]

Summary

docs: use minimal module evals

We don't actually need fully blown NixOS or home-manager configurations just to read the declared options.

Instead, we can directly call lib.evalModules to build a minimal configuration containing only stylix modules.

This is faster, simpler, more compartmentalised, etc.

This should allow for removing the option declaration location based filtering. Draft PR: https://github.com/danth/stylix/pull/1215.

This may also make adding nix-darwin docs easier, in the future.

docs: ensure pkgs is not used in option docs

Using a stubbed instance of pkgs while evaling the modules for the docs ensures no options accidentally use real packages in their description/default/example text.

This highlighted an issue that was resolved in https://github.com/danth/stylix/pull/1225.

Testing

I've tested that the docs still build.

I've spot checked that a few pages look the same as the current docs.

Issues

Compartmentalizing the eval so that third-party modules are missing has highlighted a few pre-existing minor issues with a handful of options.

Fixing those issues is not in scope for this PR, but I have added FIXME comments next to the relevant code and next to the workaround.

The main issue with autoEnable being dynamic is that mkEnableTarget uses it to decide whether or not to set defaultText. That means autoEnable needs to be evaluated in order to decide how the option is documented. Additionally, it is also used to define example.

To be clear: these are not new issues introduced by this PR; this PR does not need to fix them, as workarounds are implemented.

---

cc @awwpotato

Things done

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

[0xda157]

shouldn't using absolute paths with inputs.self for imports be removed as part of this pr?

[MattSturgeon]

shouldn't using absolute paths with inputs.self for imports be removed as part of this pr?

I'm in favour of keeping PR scope small; this makes it easier to review the changes and also reduces the pressure on contributors wanting to make small gradual improvements.

Refactoring the module path style can't be done with this PR alone, as we'd additionally need to remove the hasPrefix filtering before moving away from using ${self}.

Should be doable with a small follow up PR, though :slightly_smiling_face:

[0xda157]

I'm in favour of keeping PR scope small; this makes it easier to review the changes and also reduces the pressure on contributors wanting to make small gradual improvements.

Sounds good

[danth]

Checks are failing with the following error:

error:
       … while calling the 'derivationStrict' builtin
         at <nix/derivation-internal.nix>:37:12:
           36|
           37|   strict = derivationStrict drvAttrs;
             |            ^
           38|

       … while evaluating derivation 'stylix-book'
         whose name attribute is located at /nix/store/wkjmkjrfr9ik1blw8zmv9g5xjl412py8-source/pkgs/stdenv/generic/make-derivation.nix:480:13

       … while evaluating attribute 'patchPhase' of derivation 'stylix-book'
         at /nix/store/vl08jajqp9wdi1h9mi0z8kzks6vdsbv9-source/doc/default.nix:633:3:
          632|
          633|   patchPhase = ''
             |   ^
          634|     ${writePages}

       (stack trace truncated; use '--show-trace' to show the full, detailed trace)

       error: attribute 'xsession' missing
       at /nix/store/vl08jajqp9wdi1h9mi0z8kzks6vdsbv9-source/modules/feh/hm.nix:12:10:
           11|   autoEnable =
           12|     with config.xsession.windowManager;
             |          ^
           13|     bspwm.enable

Other than that. is it just #1244 which is blocking this PR?

[MattSturgeon]

Checks are failing with the following error:

       error: attribute 'xsession' missing
       at /nix/store/vl08jajqp9wdi1h9mi0z8kzks6vdsbv9-source/modules/feh/hm.nix:12:10:
           11|   autoEnable =
           12|     with config.xsession.windowManager;
             |          ^
           13|     bspwm.enable

Other than that. is it just #1244 which is blocking this PR?

Yes, this fails because the minimal eval doesn't have those config values. Normally that's fine, however an enable option is trying to use them in its default.

The point of #1244 is to fix that by setting a defaultText for all dynamic defaults; that's why it blocks this PR. AFAIK nothing else is blocking it.

I could actually take this a step further and strictly enforce that no config attrs are used in the docs values, the same way NixOS does. IIRC, they supply a config specialArg that throws when any attr is accessed. This may be worth investigating when I undraft.

[MattSturgeon]

could actually take this a step further and strictly enforce that no config attrs are used in the docs values[.] IIRC, [NixOS] supply a config specialArg that throws when any attr is accessed.

This is done. It caught and fixed a few more issues. This nearly fixes #98, but not quite, because we still rely on config.lib for the target functions, which are used to declare options. See this thread: https://github.com/nix-community/stylix/issues/98#issuecomment-2956126807

[MattSturgeon]

Following up on the discussion from https://github.com/nix-community/stylix/pull/1212#discussion_r2138827473, I believe the true intention is to simply negate default such that true and false are used in default and example:

-          example = config.stylix.image == null;
+          example = !(config.stylix.image != null && autoEnable);

In that case, a cleaner solution would be to get the default value into scope with an explicit let-in scope (or a silly rec):

-          example = config.stylix.image == null;
+          example = !default;

This defeats the point of the change. !default can only be used when default is known to be static. Evaluating default in the docs should be avoided at all costs, unless you control the default value and know it is static.

The current patch was discussed briefly here: https://github.com/nix-community/stylix/pull/1212#discussion_r2138827473

EDIT: I thought about this some more. Since the function already assumes autoEnable can be read safely in docs values, we can match the defaultText impl in the example impl. If the assumption ever becomes incorrect, the stricter docs impl should catch it for both example and defaultText.

I think the previous behaviour was better because it is simpler and more obvious by avoiding an additional string indirection.

Sorry, I'm unclear what behaviour you prefer? Could you post a concrete suggestion?

The intention of the change was to allow fonts to specify a string (representing a stylix font option name) or an int (which should be a literal value). In the case that an option is referenced, a defaultText is added. This is similar in-principal to mkPackageOption.

Considering how small the doc/eval_compat.nix and doc/hm_compat.nix modules are, it might be better to just inline them, since they are also used only once.

If you prefer. I moved them to separate files as I find that idiomatic for modules. Plus it keeps implementation details "out of site" so they don't distract from the detail of the main code.

They were bigger when I first implemented them though, now they are smaller there's a stronger argument for inlining.

What about the following differently structured explanation that is also wrapped at 80 characters (wrap accordingly in the code):

# Declare the arbitrarily named __stub attribute to allow modules to evaluate
# 'options.<ATTRIBUTE> ? <OPTION>'.
#
# TODO: Replace 'options.<ATTRIBUTE> ? <OPTION>' instances with 'options ?
# <ATTRIBUTE>.<OPTION>' to remove this __stub workaround.

SGTM :+1:

EDIT: Done

It might be better to replace #98 with https://github.com/nix-community/stylix/issues/98 in the commit message and the code comment to make the reference easier accessible. Also, the second paragraph in the code comment might benefit from a TODO: or NOTE: marker, although TODO: seems more appropriate here.

SGTM :+1:

EDIT: Done

[stylix-automation[bot]]

Successfully created backport PR for release-25.05:

• #1530