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: markdown option docs #176146

Merged
merged 1 commit into from
Jun 21, 2022
Merged

treewide: markdown option docs #176146

merged 1 commit into from
Jun 21, 2022

Conversation

pennae
Copy link
Contributor

@pennae pennae commented Jun 3, 2022

Description of changes

just an attempt at getting option descriptions to markdown (cf #175586).

the conversion is obviously not completely faithful (since representing the difference between <filename> and <literal> isn't easy to represent in markdown, let alone very useful?), but minimizing the diff between the current options.xml and the one after md conversions is an explicit goal. once all descriptions have been converted we can drop mdDoc again in a mechanical patch.

on top of the syntax extensions the manual already uses this introduces two new ones: {command}`foo` and {option}`foo`. both render to their docbook <tag>foo</tag> equivalent to keep the conversion between docbook descriptions and md as faithful as possible. in the future we may want to use {option} to auto-link option uses to their descriptions, but right now {command} and {option} have no such special functions.

one problem we see with these extensions is that the processor used for the options manual would have to support them, which at least for mddoc seems very hard to achieve at first glance. keeping {option} doesn't seem that important, but {command} is rendered differently from regular literals in the manpage build (so we should probably keep that, somehow). keeping the admonitions also seems very useful, not least because they stand out nicely even in the manpage build.

cc @roberth

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.11 Release Notes (or backporting 22.05 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 Jun 3, 2022
@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 Jun 3, 2022
@roberth
Copy link
Member

roberth commented Jun 4, 2022

This fits all the requirements expressed in #175586 at this point, with the exception of @edolstra's dislike of mdDoc, although that was in conjunction with pandoc.

I suppose another potential complaint is that we're using a markdown processor, mistune, that isn't the one used for the rest of the manual, and isn't the one used in the Nix (package) manual either. It is "compatible with sane CommonMark rules."
We may want to review the plugins but otherwise it seems we're good.
If someone can find a good reason to switch to a different markdown processor, they are free to do so during or after this PR, but that should not block migration progress.

The implementation looks ok so far. Will give this a try later.

Looks like we can finally complete the markdown migration! 🎉


the difference between <filename> and <literal> isn't easy to represent in markdown

tl;dr not a problem.
I am not aware of anything that uses this information. The only potential use I can think of is producing links somehow, but it doesn't really hold enough information to produce such links. If we want links, we should just add links.

nixos/lib/make-options-doc/mergeJSON.py Outdated Show resolved Hide resolved
Comment on lines 81 to 91
# also keep trailing spaces so we can diff the generated XML to check for
# conversion bugs.
Copy link
Member

Choose a reason for hiding this comment

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

Diffing seems like a good idea.

Instead of trying to match every quirk, we can choose to canonicalize both inputs to the diff. We'll need something like that for <filename> anyway.

@pennae
Copy link
Contributor Author

pennae commented Jun 4, 2022

I suppose another potential complaint is that we're using a markdown processor, mistune, that isn't the one used for the rest of the manual, and isn't the one used in the Nix (package) manual either. It is "compatible with sane CommonMark rules."
We may want to review the plugins but otherwise it seems we're good.

we really only chose mistune because it's a commonmark parser available in python that doesn't enlarge the docs closure by too much. anything else that can parse markdown into an ast would be fine. even shelling out to the favored markdown processor and have it dump an ast wouldn't be a problem, we think.

@pennae
Copy link
Contributor Author

pennae commented Jun 4, 2022

  • added literalMD for examples and defaults
  • fixed link handling and escaping
  • keep language attrs on programlisting (no option uses this so far)
  • added lists and emphasis

things we're not sure how to convert:

  • <option>? probably best to turn them into local links
  • <replaceable>? would suggest <replaceable>X</replaceable> → «X» since this matches how attrset placeholders are rendered
  • <note>/<warning>/etc? nixos markdown has admonitions that would be useful here, maybe mistune can be made to support them with plugins/parser rules. haven't checked that yet

@pennae
Copy link
Contributor Author

pennae commented Jun 4, 2022

admonitions are indeed possible :)

@github-actions github-actions bot added the 8.has: documentation This PR adds or changes documentation label Jun 5, 2022
@pennae
Copy link
Contributor Author

pennae commented Jun 5, 2022

updated description to the current state of the patch set. apart from a few kinds of admonitions the conversion script should be mostly complete (if still very experimental).

@pennae pennae marked this pull request as ready for review June 5, 2022 17:54
@pennae pennae requested review from edolstra, nbp and infinisil as code owners June 5, 2022 17:54
@pennae
Copy link
Contributor Author

pennae commented Jun 10, 2022

does this need more modules converted as examples? 🤔

@roberth
Copy link
Member

roberth commented Jun 10, 2022

  • converted entries render as usual
  • non-converted entries render as usual
  • implementation looks good

does this need more modules converted as examples?

I don't think it helps with review and merge. If you're not confident that others can contribute conversion PRs, you could do some more, but leave them for a follow-up PR.

Some documentation will be helpful:

When those are done and it's ready for other to start contributing, I think we're ready for a merge.

tag = admonitions[kind]
# we don't keep whitespace here because usually we'll contain only
# a single paragraph and the original docbook string is no longer
# available to restore the trailer.
Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by trailer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

trailing whitespace that is not rendered. we keep trailing whitespace at the end of a description to make diffing options.xml before and after a migration easier, but that isn't possible here. apart from that there is not reason to care about whitespace and if we use a different diffing process we can also remove this whitespace handling entirely.

@pennae
Copy link
Contributor Author

pennae commented Jun 11, 2022

we've added some documentation to the nixos and nixpkgs manuals clarifying which extensions are available in options docs. not entirely sure which sections to link for the final nixos manual chapter though 😕

@roberth
Copy link
Member

roberth commented Jun 12, 2022

which sections to link for the final nixos manual chapter though

I was also thinking about documenting the transition process, but it's perhaps better to write an issue for that, similar to the other tracking issues we have, like #24288.

@roberth
Copy link
Member

roberth commented Jun 12, 2022

I've tried it and it worked well.

My only findings: if you get invalid xml errors in "intermediate.xml", you've forgot mdDoc.

@pennae
Copy link
Contributor Author

pennae commented Jun 12, 2022

I've tried it and it worked well.

🍾!

My only findings: if you get invalid xml errors in "intermediate.xml", you've forgot mdDoc.

not sure we can do anything about that, unfortunately. though what we probably could do is print a warning (error?) if a level of options is not exclusive mdDoc or exclusively docbook. not sure how useful that would be in the face of option merging though

@roberth
Copy link
Member

roberth commented Jun 12, 2022

if a level of options is not exclusive mdDoc or exclusively docbook

I'm not sure if it's worth the effort. Option merging is quite rare in NixOS, so I wouldn't expect much trouble from false positives if the error message mentions the possibility. I think it's ok to mention the "bad" error message in the tracking issue. It's a temporary state of affairs anyway, during the transition.

@pennae
Copy link
Contributor Author

pennae commented Jun 12, 2022

invalid xml errors are probably most often caused by <⋯> links, those are easy to spot. the rest of the MD syntax should usually be valid xml, but hopefully we'll catch that in review. and as you say, temporart state—worst case, it's slightly less sightly until we complete the migration. (which hopefully happens before 22.11!)

@roberth roberth changed the title treewide: attempt at markdown option docs treewide: markdown option docs Jun 21, 2022
@roberth roberth merged commit e2c261f into NixOS:master Jun 21, 2022
@peterhoeg
Copy link
Member

For reasons unknown (to me), I'm getting this:

error: builder for '/nix/store/ajymzc86n3gqanmcj2kiwqra49r825g5-lazy-options.json.drv' failed with exit code 1;                                                                                                  last 10 log lines:                                                                                                                                                                                        >                                                                                                                                                                                                  
       >           252|         type = nullOr str; 
       >           253|         description = mdDoc ''
       >              |         ^
       >           254|           Address to listen on. Listen on `0.0.0.0`/`::`
       > Cacheable portion of option doc build failed.   
       > Usually this means that an option attribute that ends up in documentation (eg `default` or `description`) depends on the restricted module arguments `config` or `pkgs`.

If I drop all the mdDoc references from nixos/modules/services/networking/mosquitto.nix, I can use nixos-rebuild again.

@pennae
Copy link
Contributor Author

pennae commented Jun 27, 2022

are you backporting newer modules to older revisions? does it work with documentation.nixos.options.splitBuild = false?

@roberth
Copy link
Member

roberth commented Jun 27, 2022

@pennae, found some improvements #179351
(not related to the previous two messages)

@peterhoeg
Copy link
Member

are you backporting newer modules to older revisions?

This is admittedly with a custom nixpkgs, but it's building the latest unstable. The problem starting showing about a week ago.

does it work with documentation.nixos.options.splitBuild = false?

I'll try to create the smallest possible example that reproduces it and let you guys know. In the meantime I'm just patching out mdDoc.

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: documentation This PR adds or changes documentation 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.

5 participants