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

nixos/man: prevent duplication of options #166509

Merged
merged 1 commit into from
Apr 10, 2022
Merged

Conversation

jtojnar
Copy link
Member

@jtojnar jtojnar commented Mar 31, 2022

Description of changes

libxslt 1.1.35 fixed conflict resolution for templates to match the specification. This uncovered a bug in docbook-xsl (docbook/xslt10-stylesheets#240), which causes option names to be duplicated into the option descriptions.

Until it is resolved upstream, let’s fix the conflict by ambiguity by moving the link to a different element.

Fixes: #166304

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.

@dtzWill
Copy link
Member

dtzWill commented Mar 31, 2022

Thanks for tackling this!

I can confirm this resolves the reported issue. Although not quite out of the woods yet.

I'm not familiar with docbook really ("what other impact might moving this have?"), so instead of trying to sort that went and diff(oscope)d the manuals (man/html/epub) before and after.

And unfortunately this also changes the HTML and epub manuals.

The HTML one seems visually fine (although options are now blue instead of the red-ish 'code' styling), but comparing with old manual, there are lots of differences like this:

334c334
<                 <a class="xref" href="options.html#opt-boot.loader.grub.device"><code class="option">boot.loader.grub.device</code></a> to
---
>                 <a class="xref" href="options.html#opt-boot.loader.grub.device"><code class="option"><a class="option" href="options.html#opt-boot.loader.grub.device">boot.loader.grub.device</a></code></a> to
341c341
<                 <a class="xref" href="options.html#opt-boot.loader.systemd-boot.enable"><code class="option">boot.loader.systemd-boot.enable</code></a>
---
>                 <a class="xref" href="options.html#opt-boot.loader.systemd-boot.enable"><code class="option"><a class="option" href="options.html#opt-boot.loader.systemd-boot.enable">boot.loader.systemd-boot.enable</a></code></a>

So it seems we're now generating a nested <a> for options now (<a ...><code ...><a ...>$optionname</a></code></a>)?

The epub manual diffoscope appears to be the same sort of change, I haven't looked more closely.

For now, just reporting what I found. No answers/solutions yet 😇 .

@jtojnar
Copy link
Member Author

jtojnar commented Mar 31, 2022

Weird, building the manual with nix-build nixos/release.nix -A manualHTML.x86_64-linux and looking at result/share/doc/nixos/options.html, I see only single link:

image

@dtzWill
Copy link
Member

dtzWill commented Apr 2, 2022

Weird, building the manual with nix-build nixos/release.nix -A manualHTML.x86_64-linux and looking at result/share/doc/nixos/options.html, I see only single link:

Wow I wasn't sure how this was possible (some impurity from somewhere?) until realized: I was comparing the index.html's while you compared the options.html. Only you mentioned which you compared and I didn't 😇 .

With this detail realized, I agree options.html only has the one link. index.html has the change I mentioned, however.
And comparing against the manual before this, options.html has a change as well:

Before:

<code class="option">appstream.enable</code>

After:

<a class="option" href="options.html#opt-appstream.enable">appstream.enable</a>

Which changes the manual visually and makes each option a link to itself (same page, but with anchor for that option). This actually seems like reasonable or even desirable behavior for me-- it helps easily create links to options if you're browsing and want to share.

Anyway, mostly want to make sure we like the new output! 👍

@jtojnar
Copy link
Member Author

jtojnar commented Apr 2, 2022

Oh, that makes sense, the xrefs will transclude the target element contents by default.

@jtojnar jtojnar marked this pull request as draft April 2, 2022 15:59
@jtojnar
Copy link
Member Author

jtojnar commented Apr 2, 2022

I tried to move the id attribute to a separate link:

--- a/nixos/lib/make-options-doc/options-to-docbook.xsl
+++ b/nixos/lib/make-options-doc/options-to-docbook.xsl
@@ -22,11 +22,13 @@
         <xsl:for-each select="attrs">
           <xsl:variable name="id" select="concat('opt-', str:replace(str:replace(str:replace(str:replace(attr[@name = 'name']/string/@value, '*', '_'), '&lt;', '_'), '>', '_'), ':', '_'))" />
           <varlistentry>
-            <term xlink:href="#{$id}">
-              <xsl:attribute name="xml:id"><xsl:value-of select="$id"/></xsl:attribute>
-              <option>
-                <xsl:value-of select="attr[@name = 'name']/string/@value" />
-              </option>
+            <term>
+              <link xlink:href="#{$id}">
+                <xsl:attribute name="xml:id"><xsl:value-of select="$id"/></xsl:attribute>
+                <option>
+                  <xsl:value-of select="attr[@name = 'name']/string/@value" />
+                </option>
+              </link>
             </term>
 
             <listitem>

But that does not work either:

Don't know what gentext to create for xref to: "link"

Forgot that xref can only link to certain elements.

libxslt 1.1.35 fixed conflict resolution for templates to match the specification.
This uncovered a bug in docbook-xsl (docbook/xslt10-stylesheets#240),
which causes option names to be duplicated into the option descriptions.

Let’s resolve the conflict by patching the stylesheets.

Fixes: NixOS#166304
@jtojnar jtojnar marked this pull request as ready for review April 3, 2022 19:37
@jtojnar
Copy link
Member Author

jtojnar commented Apr 3, 2022

I applied a patch to the stylesheets that should resolve the issue. Limiting scope to the manual to avoid stdenv rebuild.

@jtojnar jtojnar mentioned this pull request Apr 3, 2022
@Lassulus Lassulus merged commit c274af4 into NixOS:master Apr 10, 2022
@jtojnar jtojnar deleted the man-opt-nodup branch April 10, 2022 10:36
@dtzWill
Copy link
Member

dtzWill commented May 1, 2022

Thanks!!

arcnmx pushed a commit to arcnmx/nmd that referenced this pull request Jul 21, 2022
Should fix duplicate option names into the manpages generated with nmd.
See:
- NixOS/nixpkgs#166304
- NixOS/nixpkgs#166509
- nix-community/home-manager#2820
rycee added a commit to nix-community/home-manager that referenced this pull request Jun 24, 2023
Removes the need to apply `fix-man-options-duplication.patch`. See,
e.g., NixOS/nixpkgs#166509.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nixos manual (man configuration.nix) duplicates option names into descriptions
3 participants