Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

treewide: more defaultText for options #148785

Merged
merged 16 commits into from
Dec 17, 2021
Merged

treewide: more defaultText for options #148785

merged 16 commits into from
Dec 17, 2021

Conversation

pennae
Copy link
Contributor

@pennae pennae commented Dec 5, 2021

like #147441, but handles the bits that are more complicated than copying an expression and surrounding it with quotes.

just this PR alone seems to speed up the docs build by about 15%, which makes for a ~3% improvement on nixos-rebuild on our machine.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Dec 5, 2021
@pennae pennae requested review from samueldr, jtojnar and cole-h December 5, 2021 22:24
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Dec 5, 2021
@Artturin Artturin requested a review from roberth December 5, 2021 23:17
@pennae pennae mentioned this pull request Dec 8, 2021
13 tasks
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Regarding not knowing how to show an option path for submodule options, I've wanted this myself before too. It's very nicely possible with this change (unfortunately reverted for now from the PR it's contained in): 1e6a84b.

Do you think you could use a patch like this to simplify many of these changes?

Also an additional correctness benefit of this is that such an approach will also show the correct nested option path if you use all of NixOS as a submodule (e.g. as I'm doing in https://github.com/Infinisil/nixus).

nixos/modules/services/databases/neo4j.nix Outdated Show resolved Hide resolved
@infinisil
Copy link
Member

Is there something (now or in the future) that can check whether all options have a constant default/defaultText?

@pennae
Copy link
Contributor Author

pennae commented Dec 8, 2021

Is there something (now or in the future) that can check whether all options have a constant default/defaultText?

pretty much, #149532 introduces hard failures for non-constant documentation attributes in the cached part of the docs build. the uncached part is unrestricted, and we haven't found a satisfying way of making defaultText mandatory without also breaking interpolations like ${cfg.package.version} in description texts :/

infinisil and others added 13 commits December 8, 2021 21:41
nixos no longer ships journalbeat 5 and hasn't since at least 20.09. remove
checks for older versions from the module.
adds defaultText for all options that set their default to a path expression
using the ubiquitous `cfg` shortcut bindings.
the kubernetes modules cross-reference their config using an additional shortcut
binding `top = config.services.kubernetes`, expand those to defaultText like
`cfg` previously.
adds defaultText for all options that use `cfg.*` values in their
defaults, but only for interpolations with no extra processing (other
than toString where necessary)
…faults

adds defaultText for options with defaults that use only literals, full config.*
paths, and the cfg shortcut binding.
escape interpolations in descriptions where possible, replace them with
sufficiently descriptive text elsewhere. also expand cfg.* paths in
descriptions.
escape interpolations in examples, or replace them where they are not
useful.
some options have default that are best described in prose, such as
defaults that depend on the system stateVersion, defaults that are
derivations specific to the surrounding context, or those where the
expression is much longer and harder to understand than a simple text
snippet.
instead of keeping a defaultConfig value around, set that value as the
default of the option and explicitly use the option default instead.
this also allows us to write a defaultText that makes sense and is in
proximity to the definition of the default.
unfortunately we don't have a good way to represent defaults that
reference other values of the current submodule, so we just use the
relative path of the referenced value and assume that the submodule was
declared as `rec`.
easiest way to do this is to move the default expression out and
abstract over what is substituted into it, using a dependent value for
the default and a descriptive value for defaultText
these are mostly options that use alias bindings, bindings to constants,
or bindings to calculated values.
@pennae pennae requested review from edolstra and nbp as code owners December 9, 2021 00:52
@adisbladis adisbladis removed their request for review December 9, 2021 06:39
@pennae pennae requested a review from infinisil December 12, 2021 02:54
@grahamc grahamc merged commit 06edb74 into NixOS:master Dec 17, 2021
@grahamc
Copy link
Member

grahamc commented Dec 17, 2021

Thank you, this looks great! I just turned off docs on one of my systems because of how slow it is ... :).

@pennae pennae deleted the more-option-doc-staticizing branch March 24, 2022 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants