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

nixosOptionsDoc.optionsMarkdown: init, with anchors #300309

Closed

Conversation

roberth
Copy link
Member

@roberth roberth commented Mar 30, 2024

Description of changes

This adds a new optionsMarkdown attribute, that's compatible with the HTML (and DocBook) outputs.

This functionality needs anchor syntax, which is not part of the CommonMark specification, as of version 0.31.2.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

This adds a new optionsMarkdown, that's compatible with the HTML
(and DocBook) outputs.

This needs anchor syntax, which is not part of the CommonMark
specification, as of version 0.31.2.
@roberth roberth requested a review from pennae March 30, 2024 19:06
@github-actions github-actions bot added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Mar 30, 2024
Comment on lines +113 to +124
optionsMarkdown = pkgs.runCommand "options.md" {
nativeBuildInputs = [ pkgs.nixos-render-docs ];
} ''
nixos-render-docs -j $NIX_BUILD_CORES options commonmark \
--manpage-urls ${pkgs.path + "/doc/manpage-urls.json"} \
--revision ${lib.escapeShellArg revision}${
lib.optionalString markdownAnchors '' \
--render-anchors \
--anchor-prefix ${lib.escapeShellArg optionIdPrefix}''} \
${optionsJSON}/share/doc/nixos/options.json \
$out
'';
Copy link
Contributor

Choose a reason for hiding this comment

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

this mostly duplicates optionsCommonMark, it would make sense to have a function that encapsulates commonmark-plus-extensions in some way. that would also spare you the indignity of turning your own feature on while advising all other new features to be turned off.

Copy link
Member Author

Choose a reason for hiding this comment

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

In #370352 I avoid this duplication by introducing extraArgs which can be overridden. That's much simpler and doesn't cause duplication.

Comment on lines +413 to +422
_id_translate_table = {
ord('*'): ord('_'),
ord('<'): ord('_'),
ord(' '): ord('_'),
ord('>'): ord('_'),
ord('['): ord('_'),
ord(']'): ord('_'),
ord(':'): ord('_'),
ord('"'): ord('_'),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

that's the same table as xml uses, duplicated. it's not a great transform to begin with, we should be applying something more like url-encoding instead (but see later comments for that also).

Copy link
Member Author

Choose a reason for hiding this comment

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

It now reuses that routine. I agree it's not a great transform, lacking injectivity, but it's the one that's been used all this time in DocBook and HTML option docs.
A better transform could be added by extending the enum in #370352, if anyone wants or needs that.

@@ -437,11 +455,15 @@ def _decl_def_entry(self, href: Optional[str], name: str) -> list[str]:
def _decl_def_footer(self) -> list[str]:
return []

def make_id (self, loc: list[str]) -> str:
return '.'.join([to_id(x) for x in loc])
Copy link
Contributor

Choose a reason for hiding this comment

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

that's not necessarily a unique transform any more if any module chooses to use a . in its keys. existing code gets a pass on this by virtue if being inherited bad behavior, new code should not make such mistakes if at all possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Compatibility is my explicit goal with the legacy anchor style. I think I've done a good job at reusing existing code in #370352. A line of code like this is unavoidable for that. Better anchor styles can be added, as mentioned.

@roberth
Copy link
Member Author

roberth commented Apr 21, 2024

it would make sense to have a function that encapsulates commonmark-plus-extensions in some way.

I agree, but I couldn't find a good place to put that function. For that I blame the structure of nixosOptionsDoc.
I'd like to replace it by a "normal" library that doesn't put an arbitrary set of parameters into the closure of an attrset.
While originally i was hoping to make a quick prototype of that, I ended up in rabbit hole #305847.

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 9, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 2, 2025
@roberth
Copy link
Member Author

roberth commented Jan 2, 2025

I've opened #370352 to replace this without force pushing.
Thank you @pennae for the review

@roberth roberth closed this Jan 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants