lib/options/showOption: fix quoting of attr-names that are not identifiers #194035

Ma27 · 2022-10-02T08:36:38Z

Description of changes

Personally, I think that warnings such as

warning: The option `services.redis.enable' defined in `/home/ma27/Projects/nixpkgs/test.nix@node-vm' has been renamed to `services.redis.servers..enable'.

are fairly confusing because of the .. and it's more correct to actually quote that. With this change the warning now looks like this:

warning: The option `services.redis.enable' defined in `/home/ma27/Projects/nixpkgs/test.nix@node-vm' has been renamed to `services.redis.servers."".enable'.

While implementing that I realized that you'd have a similar problem whenever you use attribute-names that aren't identifiers, e.g.

services.nginx.virtualHosts."example.org".locations."/".invalid = 23;

now results in the following error:

error: The option `interactive.nodes.vm.services.nginx.virtualHosts."example.org".locations."/".invalid' does not exist. Definition values:
       - In `/home/ma27/Projects/nixpkgs/test.nix@node-vm': 23

Of course there are some corner-cases where this won't work: when generating the manual, you display submodules like this:

services.nginx.virtualHosts.<name>

Since <name> isn't a value, but an indicator for a submodule, it must not be quoted. This also applies to the following identifiers:

* for listOf submodule
<function body> for functionTo

This might not be correct if you actually have a submodule with an attribute name called <name>, but I think it's an improvement over the current situation and for this you'd probably need to make even more complex changes to the module system.

Closes #152649

Things done

roberth · 2022-10-06T10:15:04Z

We shouldn't have been mixing placeholders and unescaped identifiers in the first place.
Instead, we could represent paths like [ "etc" { placeholder = "<name>"; } "mode" ]. This makes the code accurate and moves type-specific logic back to the types where it belongs.

Ma27 · 2022-10-06T20:21:42Z

@roberth do you think it's possible to implement that without noticeable breaking changes? If yes, I'm happy to give it a try, but otherwise I'd say that this is the pragmatic solution to improve the situation a little bit at least :)

roberth

While I do think the correct solution is feasible, I don't mind getting there in steps.

lib/options.nix

lib/types.nix

lib/options.nix

…to indicate a function inside an option structure The motivation is to have a single identifier for that. Useful for the next commit where I'll try to escape option-parts correctly (options can be any kind of strings, but unless these are Nix identifiers, they must be quoted). Since `<function body>` (or `<name>`/`*`) are special identifiers in error messages and the manual, we need a unique way to mark an option part as function call because these are not to be quoted.

…fiers Personally, I think that warnings such as warning: The option `services.redis.enable' defined in `/home/ma27/Projects/nixpkgs/test.nix@node-vm' has been renamed to `services.redis.servers..enable'. are fairly confusing because of the `..` and it's more correct to actually quote that. With this change the warning now looks like this: warning: The option `services.redis.enable' defined in `/home/ma27/Projects/nixpkgs/test.nix@node-vm' has been renamed to `services.redis.servers."".enable'. While implementing that I realized that you'd have a similar problem whenever you use attribute-names that aren't identifiers, e.g. services.nginx.virtualHosts."example.org".locations."/".invalid = 23; now results in the following error: error: The option `interactive.nodes.vm.services.nginx.virtualHosts."example.org".locations."/".invalid' does not exist. Definition values: - In `/home/ma27/Projects/nixpkgs/test.nix@node-vm': 23 Of course there are some corner-cases where this won't work: when generating the manual, you display submodules like this: services.nginx.virtualHosts.<name> Since `<name>` isn't a value, but an indicator for a submodule, it must not be quoted. This also applies to the following identifiers: * `*` for `listOf submodule` * `<function body>` for `functionTo` This might not be correct if you actually have a submodule with an attribute name called `<name>`, but I think it's an improvement over the current situation and for this you'd probably need to make even more complex changes to the module system.

Ma27 · 2022-10-07T08:09:40Z

Thanks a lot for your review! I addressed all of your coments :)

roberth

Looks ok to me
@infinisil please review

infinisil

This currently breaks the manual build, since the XSLT processing relies on this in some weird way, you can check with

nix-build nixos/release.nix -A manualHTML.x86_64-linux

I previously broke the manual in almost the same way when I tried to change the implementation of showOption a bit in #82461, which was then subsequently reverted in #85241. There's also a bug report regarding showOption in #152649

Now we even have options like `services.listmonk.database.settings."app.notify_emails"` shown correctly (i.e. with quotes).

Ma27 · 2022-10-09T08:15:17Z

@infinisil I replaced the quotes within the xslt now with a _ and it appears to work fine. Options like services.listmonk.database.settings."app.notify_emails" are also shown correctly quoted in the manual now :)

roberth · 2022-10-09T11:17:46Z

nixos/lib/make-options-doc/options-to-docbook.xsl

-                '*&lt; >[]:',
-                '_______'
+                '*&lt; >[]:&quot;',
+                '________'


This changes the identifiers, breaking inbound links. Could you remove the quotes instead of replacing them with underscores?

Shouldn't that throw an error if <xref linkend="" /> points to an unknown location?

Then that would have been a problem before.
You'd have to define options."" = mkOption ... at the top level, which I don't think is something we have to worry about.

Then that would have been a problem before.

Yes. But considering that I can build the manual locally, doesn't that mean that there are no breaking links?

You'd have to define options."" = mkOption ... at the top level, which I don't think is something we have to worry about.

Even though it's just a theoretical problem, having e.g. settings."foo.bar" and settings.foo.bar defined would cause a conflict when removing the quotes, that's why I kept them.

Or am I misunderstanding your point? :)

ping @roberth

I was more concerned about links from the web to the manual. Passing the internal validation will not detect such broken links.
It does appear that the impact is quite limited:

({+_+} is git word diff syntax for "add underscore")

138 ids relating to sr.ht such as opt-services.sourcehut.settings.{+_+}sr.ht{+_+}.owner-name

opt-services.listmonk.database.settings.{+_+}app.notify_emails{+_+}

opt-services.listmonk.database.settings.{+_+}bounce.mailboxes{+_+}

opt-services.listmonk.database.settings.{+_+}privacy.domain_blocklist{+_+}

opt-services.listmonk.database.settings.{+_+}privacy.exportable{+_+}

opt-services.xserver.windowManager.{+_+}2bwm{+_+}.enable

This seems like a small price to pay for a simpler and more injective id generating function.

Resolved

Ma27 · 2022-10-16T18:09:02Z

@roberth so is it fine to keep that as-is? And: anything else tbd here? :)

This reverts commit 6259b29, reversing changes made to 46e516e.

It's now necessary to also escape `"` (i.e. `"`) in option ids, otherwise the build will fail because `"` is not a valid part of an XML element id. See also NixOS/nixpkgs#196651 and the original change in NixOS/nixpkgs#194035 (comment) for further context. The `translate` expression is directly taken from the nixpkgs's `options-to-docbook.xsl`.

…-quoting"" This reverts commit dedb37a.

Ma27 requested review from edolstra, nbp and infinisil as code owners October 2, 2022 08:36

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Oct 2, 2022

infinisil added the 6.topic: module system About "NixOS" module system internals label Oct 3, 2022

roberth reviewed Oct 6, 2022

View reviewed changes

lib/options.nix Outdated Show resolved Hide resolved

lib/types.nix Show resolved Hide resolved

lib/options.nix Show resolved Hide resolved

Ma27 force-pushed the show-option-quoting branch from 671fc21 to 4cc267c Compare October 7, 2022 08:08

Ma27 force-pushed the show-option-quoting branch from 4cc267c to 6396482 Compare October 7, 2022 08:09

Ma27 requested a review from roberth October 7, 2022 08:09

roberth approved these changes Oct 7, 2022

View reviewed changes

bobby285271 added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 7, 2022

infinisil previously requested changes Oct 7, 2022

View reviewed changes

nixos/doc: fix build

2480532

Now we even have options like `services.listmonk.database.settings."app.notify_emails"` shown correctly (i.e. with quotes).

Ma27 requested a review from infinisil October 9, 2022 08:14

github-actions bot added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Oct 9, 2022

roberth reviewed Oct 9, 2022

View reviewed changes

roberth approved these changes Oct 13, 2022

View reviewed changes

roberth merged commit 6259b29 into NixOS:master Oct 18, 2022

andrevmatos mentioned this pull request Oct 19, 2022

[nix-darwin/home-manager] manual-combined.xml fails to validate #196651

Closed

gador added a commit to gador/nixpkgs that referenced this pull request Oct 19, 2022

Revert "Merge pull request NixOS#194035 from Ma27/show-option-quoting"

dedb37a

This reverts commit 6259b29, reversing changes made to 46e516e.

Ma27 deleted the show-option-quoting branch October 19, 2022 14:42

gador added a commit to gador/nixpkgs that referenced this pull request Oct 26, 2022

Revert "Revert "Merge pull request NixOS#194035 from Ma27/show-option…

8f200c3

…-quoting"" This reverts commit dedb37a.

ncfavier mentioned this pull request Oct 31, 2022

flake.lock: Update nix-community/home-manager#3234

Merged

ncfavier mentioned this pull request Feb 18, 2023

lib/options: update showOption comment #216983

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib/options/showOption: fix quoting of attr-names that are not identifiers #194035

lib/options/showOption: fix quoting of attr-names that are not identifiers #194035

Ma27 commented Oct 2, 2022 •

edited by roberth

Loading

roberth commented Oct 6, 2022

Ma27 commented Oct 6, 2022

roberth left a comment

Ma27 commented Oct 7, 2022

roberth left a comment

infinisil left a comment

Ma27 commented Oct 9, 2022

roberth Oct 9, 2022

Ma27 Oct 9, 2022

roberth Oct 9, 2022

Ma27 Oct 9, 2022

Ma27 Oct 13, 2022

roberth Oct 13, 2022

Ma27 commented Oct 16, 2022

lib/options/showOption: fix quoting of attr-names that are not identifiers #194035

lib/options/showOption: fix quoting of attr-names that are not identifiers #194035

Conversation

Ma27 commented Oct 2, 2022 • edited by roberth Loading

Description of changes

Things done

roberth commented Oct 6, 2022

Ma27 commented Oct 6, 2022

roberth left a comment

Choose a reason for hiding this comment

Ma27 commented Oct 7, 2022

roberth left a comment

Choose a reason for hiding this comment

infinisil left a comment

Choose a reason for hiding this comment

Ma27 commented Oct 9, 2022

roberth Oct 9, 2022

Choose a reason for hiding this comment

Ma27 Oct 9, 2022

Choose a reason for hiding this comment

roberth Oct 9, 2022

Choose a reason for hiding this comment

Ma27 Oct 9, 2022

Choose a reason for hiding this comment

Ma27 Oct 13, 2022

Choose a reason for hiding this comment

roberth Oct 13, 2022

Choose a reason for hiding this comment

Ma27 commented Oct 16, 2022

Ma27 commented Oct 2, 2022 •

edited by roberth

Loading