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

Migrate option documentation to nixosOptionsDoc #4215

Merged
merged 19 commits into from
Jul 19, 2023

Conversation

emilazy
Copy link
Contributor

@emilazy emilazy commented Jul 9, 2023

Description

Okay, here we go!

These changes move option documentation generation to nixosOptionsDoc and migrate all the existing documentation to Markdown. The output given the same input is almost identical or in some ways improved (e.g. no <quote> needed for smart quotes); the most notable non-positive changeis some unstripped empty lines at the end of literalExamples; these can also be seen in the NixOS manual and should be addressed upstream. All automatic conversions were verified by nix-doc-munge to produce no significant changes in output, and almost all manual conversions resulted in equivalent or improved semantics and formatting; a very small number had to be slightly downgraded (e.g. tables, multiple terms for one definition in a definition list), mostly due to easily-fixable things like "nobody added processing for the relevant Markdown node to the options documentation part of nixos-render-docs yet".

I won't go into justification too much, as it was already discussed extensively in #4158, but I will summarize that I think using the upstream NixOS tools will be more robust in the long run and make sharing code and contributors with NixOS easier. I admit that I prefer AsciiDoc to Markdown myself, but the benefits of using the same format and tooling as NixOS far outweigh that, and anyway NixOS's Markdown has added almost all of the semantic niceties you would want regardless. (And, of course, writing DocBook by hand is the worst of all options; in many cases I had to manually mark options as Markdown because they already contained Markdown syntax that wasn't rendering correctly but was much less unwieldy to write than the correct DocBook would have been.)

The commits have fairly detailed explanations, but the overview is:

  • Pin the last version of nixos-unstable where nixosOptionsDoc still supported DocBook.
  • Start using it and set up some infrastructure for the conversion.
  • Manually tweak or convert some options that can't be handled automatically.
  • Convert the rest with my version of nix-doc-munge.
  • Remove the pin.
  • Strip out all the lib.mdDoc calls, as all documentation is interpreted as Markdown in the latest Nixpkgs.

The last part could be omitted if desired; I chose reducing visual noise over ease of backporting, as the release branches of Home Manager don't seem to get many changes anyway. The presence of lib.mdDoc calls could also mislead people into thinking that unmarked documentation will still be interpreted as DocBook. Still, I can undo that change if it's not preferred.

The following commits were automated, whether by simple textual replacement or nix-doc-munge:

  • e04de5b treewide: mkPackageOption -> mkPackageOptionMD
  • b5a65b9 treewide: mkAliasOption -> mkAliasOptionMD
  • c1d8d2a treewide: adjust some DocBook for conversion (mostly; a few trivial "human-powered sed" tweaks)
  • 36a53d9 treewide: convert all option docs to Markdown
  • 9f9e277 treewide: remove now-redundant lib.mdDoc calls

The rest are all manually written and hopefully not too difficult to review.

This is only the first half of the migration: it still relies on the deprecated optionsDocBook output (hence the warnings when evaluating under 23.11) and has the full DocBook toolchain for manual generation. I have an almost-complete full conversion of the manual locally that preserves the content and styling, but there a few things that need addressing in upstream nixos-render-docs before it's ready to PR (mostly just some minor rendering issues and a couple additional customization points), so that's probably a few weeks off as I'll need to PR the changes upstream.

I tested all commits with the following:

emily@yuyuko ~/D/home-manager (option-docs-migration)> cat >test.bash
#!/usr/bin/env bash
set -eu
if uname | grep -q Darwin && ! grep -q nixosOptionsDoc docs/default.nix; then
	# Work around Nix sandbox bug on Darwin.
	nixpkgs=github:emilazy/nixpkgs/841136098f0065ebdd7121646165aedefb31e819
	legacyArgs=(--arg pkgs "import (builtins.getFlake \"$nixpkgs\") {}")
	flakeArgs=(--override-input nixpkgs "$nixpkgs")
else
	export NIX_PATH=nixpkgs=flake:nixpkgs/$(nix flake metadata --json | jq -r .locks.nodes.nixpkgs.locked.rev)
	legacyArgs=()
	flakeArgs=()
fi
set -x
./format -c >/dev/null
nix build "${flakeArgs[@]}" --no-link .#docs-{html,json,manpages}
nix-build "${legacyArgs[@]}" --quiet --no-out-link -A docs.html -A docs.manPages -A docs.json >/dev/null
nix-build "${legacyArgs[@]}" --quiet --no-out-link tests -A build.manual >/dev/null
emily@yuyuko ~/D/home-manager (option-docs-migration)> nix shell nixpkgs#bash -c git rebase --exec 'bash test.bash' upstream/master

and I checked that all tests pass successfully on the final commit.

I am happy to be pinged for any future issues with documentation processing and generation going forward.

P.S. It's not too hard to update this PR, as the most invasive changes are all automated, but it is a little fiddly, so I'll hold off on updating it for any other changes until this is considered ready for merge (and would appreciate holding off on merging any others from that point to the final merge). It will, of course, cause conflicts with many open PRs on merge, especially those that touch documentation, and PRs after merge will have to be checked for containing DocBook documentation, but that's unavoidable and should be simple to resolve for any given PR.

Checklist

  • Change is backwards compatible. (for users, not contributors)

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all or nix develop --ignore-environment .#all using Flakes.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

Maintainer CC

cc @rycee @ncfavier

@rycee
Copy link
Member

rycee commented Jul 9, 2023

Thanks and much appreciated! Will try to look through this as quickly as possible.

@rycee
Copy link
Member

rycee commented Jul 9, 2023

About the

Strip out all the lib.mdDoc calls, as all documentation is interpreted as Markdown in the latest Nixpkgs.

change, I'll have a think. I'm leaning towards it being fine to strip them out but, like you say, it makes backports a little more involved.

And to be clear, it is OK to merge new modules and such while this PR is open? We can consider the docs directory frozen until the merge but it would be a pity not to be able to merge module changes as usual.

@emilazy
Copy link
Contributor Author

emilazy commented Jul 9, 2023

Yes, that's totally fine of course; my intention is to leave this PR as-is during review, and then after it's done and this is considered ready to merge, I'll run the conversion one last time (and handle any options that don't convert automatically) and rebase the PR accordingly. I'd just appreciate it if I could have couple days of no merges at that point so I'm not having to play catch-up with master to update it :)

(FWIW, I suspect backports will be a bit manual either way, as anything adjacent to or touching documentation already likely won't be able to be automatically cherry-picked.)

@emilazy
Copy link
Contributor Author

emilazy commented Jul 9, 2023

Naturally I forgot the most ironic possible thing: updating the documentation. (Perhaps it could use a release note too? But it shouldn't be relevant to any user unless they're maintaining a custom module.)

default.nix Outdated Show resolved Hide resolved
@rycee
Copy link
Member

rycee commented Jul 15, 2023

I've looked through the commits now and checked the resulting output. Impressive work, looks very good in general! As far as I can tell the only negative is the lack of table support but I think using the description lists is a decent compromise.

I think we are good to merge this whenever you wish. From when would you like the PR merge freeze?

@ncfavier
Copy link
Member

Tested this branch, builds and looks fine.

Are these warnings expected (to go away)?

trace: warning: optionsDocBook is deprecated since 23.11 and will be removed in 24.05

@emilazy
Copy link
Contributor Author

emilazy commented Jul 16, 2023

I've pushed some changes per the review comments. Holding off on the final rebase until it's time.

I've looked through the commits now and checked the resulting output. Impressive work, looks very good in general! As far as I can tell the only negative is the lack of table support but I think using the description lists is a decent compromise.

I think we are good to merge this whenever you wish. From when would you like the PR merge freeze?

Yeah, the table support is a shame, but all the machinery is there upstream, it just needs plugging into option docs. I plan to do that but didn't want to hold this up for it. As far as freezing goes, I'm flexible; should be able to get this finished within 2-3 days of ping (and probably less) any time next week, since the actual work is simple. If there are any other big PRs that touch/add documentation that are approaching ready for merge it might be good to get those in first to save other people the conversion effort.

Tested this branch, builds and looks fine.

Are these warnings expected (to go away)?

trace: warning: optionsDocBook is deprecated since 23.11 and will be removed in 24.05

Yeah, this is expected and unfortunately unavoidable for now; we need the DocBook options output that Nixpkgs is getting rid of in order to plug into the existing nmd/DocBook pipeline. From the PR message (which is super long, so I don't blame you for missing it):

This is only the first half of the migration: it still relies on the deprecated optionsDocBook output (hence the warnings when evaluating under 23.11) and has the full DocBook toolchain for manual generation. I have an almost-complete full conversion of the manual locally that preserves the content and styling, but there a few things that need addressing in upstream nixos-render-docs before it's ready to PR (mostly just some minor rendering issues and a couple additional customization points), so that's probably a few weeks off as I'll need to PR the changes upstream.

The next steps will be me making some changes to nixos-render-docs upstream to adapt it to the Home Manager (and nix-darwin) context, and then I can finish the job off and we'll have a zero-DocBook documentation generator. Might take me a little bit to get around to, but certainly before the 23.11 release.

This checks that the documentation can be built in the context of a
Home Manager configuration.
.release Outdated Show resolved Hide resolved
@rycee
Copy link
Member

rycee commented Jul 16, 2023

Sounds excellent, then I propose to start the PR merge freeze on July 19 (Wednesday) with the hope that we can merge this before the end of the week.

I checked the repo settings and it seems like it is possible to lock a protected branch and as far as I can tell that will prevent any commits from being added to the branch. So on the morning of 19th (central European time) I'll mark the master branch as locked and add a comment in this PR. Unless some serious issue needs to be resolved (I'm mainly thinking a security or data integrity issue) then I will keep it locked until @emilazy writes a comment saying it is OK to merge.

I'll make an effort to be available in the Matrix chat during this time, at least during CEST working hours, when I'm on my computer anyways 🙂

This is the last bump of the `nixos-unstable` channel before the
DocBookalypse.
Output is mostly unchanged aside from some minor typographical and
formatting changes, along with better source links.

We temporarily export `options.docBookForMigration` to allow
`nix-doc-munge` to check its conversions.
The new options processor errors out on these by default, and it's
good for every option to have documentation in general.
`mkEnableOption` wraps its argument in a full sentence; its argument
should not include the start of a sentence or the final full stop.
These files all have options that trip up the `nix-doc-munge`
conversion tool for one reason or another (syntax that clashes with
Markdown, options that were already using Markdown syntax despite not
being marked that way, output that differs slightly after conversion,
syntax too elaborate to convert with some cheap regular expressions,
...). Translate them manually and do a little copyediting to options
in the vicinity while we're at it.
Parameterized documentation generators like this can't be converted
automatically.
`nix-doc-munge` can't tell what's going on with this pattern, so I
just handled them all manually.
`nix-doc-munge` can't handle these, which is understandable as I can
barely handle them either. There are a few infelicities here: the
current processor can't handle multiple terms to one description in
a description list so they get comma-separated in one case, and one
case that should ideally render as a `<figure>` with a `<figcaption>`
in HTML is reduced to a paragraph with some `<strong>` text. (Which, in
fairness, is how it rendered in practice with the DocBook anyway.) The
docs generator has since been updated to handle figures, but we can't
use it until moving off DocBook output.
The Markdown options processor cannot handle rendering tables
to DocBook.  This could be fixed, but as we won't be using the
DocBook output for long I just removed them for now in the interest
of expediency; they were all well-suited to being description lists
showing option types anyway, apart from one awkward case in the form
of trayer, which also had ad-hoc syntax for enumerating acceptable
values in the documentation. Since the types aren't actually used for
option processing anyway, I changed them to use `enum` and similar to
give a single description of the acceptable values without a big table.
The NixOS variant of Markdown doesn't make a distinction between
`<code>` and `<literal>` or `<quote>` and... quotes, and doesn't
support `<parameter>` or `<replaceable>`. These are infrequently used
(apart from `<code>`) and don't add much, so just convert them to
simpler forms to allow the options containing them to be converted
to Markdown automatically.

A few minor syntactic adjustments were also made to make
`nix-doc-munge`'s job easier.
This process was automated by [my fork of `nix-doc-munge`]. All
conversions were automatically checked to produce the same DocBook
result when converted back, modulo minor typographical/formatting
differences on the acceptable-to-desirable spectrum.

To reproduce this commit, run:

  $ NIX_PATH=nixpkgs=flake:nixpkgs/e7e69199f0372364a6106a1e735f68604f4c5a25 \
    nix shell nixpkgs#coreutils \
    -c find . -name '*.nix' \
    -exec nix run -- github:emilazy/nix-doc-munge/98dadf1f77351c2ba5dcb709a2a171d655f15099 \
    {} +
  $ ./format

[my fork of `nix-doc-munge`]: https://github.com/emilazy/nix-doc-munge/tree/home-manager
Now that the Markdown migration is complete, everything should work
with the latest `nixpkgs-unstable`.
These (and the `*MD` functions apart from `literalMD`) are now no-ops
in nixpkgs and serve no purpose other than to add additional noise and
potentially mislead people into thinking unmarked DocBook documentation
will still be accepted.

Note that if backporting changes including documentation to 23.05,
the `mdDoc` calls will need to be re-added.

To reproduce this commit, run:

    $ NIX_PATH=nixpkgs=flake:nixpkgs/e7e69199f0372364a6106a1e735f68604f4c5a25 \
      nix shell nixpkgs#coreutils \
      -c find . -name '*.nix' \
      -exec nix run -- github:emilazy/nix-doc-munge/98dadf1f77351c2ba5dcb709a2a171d655f15099 \
      --strip {} +
    $ ./format
@emilazy
Copy link
Contributor Author

emilazy commented Jul 17, 2023

Wednesday sounds good to me! I don't expect it will take long at all to update the PR; just a matter of being around to run the scripts and tests and handle any new option documentation needing manual conversion. I've rebased against latest master to get a head start on that and it was purely mechanical. I also added a note of this change to the release notes in case people need to update documentation in third-party modules (currently the local manual would not include them anyway, I believe, but as this could change in the future it's good to make sure people are using the right syntax).

@rycee
Copy link
Member

rycee commented Jul 18, 2023

@emilazy Great, then I'll lock the master branch tomorrow and you should be safe to do the migration without conflicts.

@ncfavier, @sumnerevans, @teto Please mind that @emilazy will be migrating the documentation to the Nixpkgs documentation generator starting tomorrow Wednesday. To avoid merge conflicts, please hold off on doing merged until this PR is complete. I'll try locking the master branch so I think it won't be possible to merge anything even if you try but I'm not fully certain that it works like that.

@teto
Copy link
Collaborator

teto commented Jul 18, 2023

acked ! Sounds like huge work. Thanks !

@emilazy
Copy link
Contributor Author

emilazy commented Jul 18, 2023

FWIW as long as there's no commits to master past 2f84579 then this is good to merge as-is. Otherwise I'll update it ASAP of course.

@rycee
Copy link
Member

rycee commented Jul 19, 2023

I locked master now. If I understand @emilazy correctly, then the PR is already up-to-date. If that is the case then we can merge in the evening ~10-15 hours from now.

If anybody wants to test and comment on this PR then now is the time 🙂

@emilazy
Copy link
Contributor Author

emilazy commented Jul 19, 2023

Yep, should be good to go!

@rycee rycee merged commit 7579044 into nix-community:master Jul 19, 2023
3 checks passed
@rycee
Copy link
Member

rycee commented Jul 19, 2023

Ok, had a look at the latest changes, the generated documentation, and the release notes. All looked just fine to me so I've merged now. Thanks a lot for the hard work @emilazy, much appreciated!

I'm sure there will be some fallout over the next few days/weeks with people having failed builds due to having their own modules with docbook descriptions. So please be mindful of this and let them know that they can change the descriptions to the markdown. Can point them to the release notes.

@emilazy
Copy link
Contributor Author

emilazy commented Jul 19, 2023

Wonderful, thank you for getting this reviewed and merged! I will be busy for the next short while but will try and work on nixos-render-docs when I can.

@rycee rycee mentioned this pull request Nov 24, 2023
5 tasks
magneticflux- added a commit to magneticflux-/home-manager that referenced this pull request Apr 13, 2024
This is an error as of NixOS/nixpkgs#303841

It seems to have been missed in nix-community#4215
diniamo added a commit to diniamo/home-manager that referenced this pull request Apr 16, 2024
spotify-player: tests

chore: format

fix: declare in modules.nix

spotify-player: add news entry

spotify-player: implement review suggestions

spotify-player: fix maintainers

Squashed commit of the following:

commit 1c43dcf
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Mon Apr 15 09:42:51 2024 +0200

    ci: bump peaceiris/actions-gh-pages from 3 to 4

    Bumps [peaceiris/actions-gh-pages](https://github.com/peaceiris/actions-gh-pages) from 3 to 4.
    - [Release notes](https://github.com/peaceiris/actions-gh-pages/releases)
    - [Changelog](https://github.com/peaceiris/actions-gh-pages/blob/main/CHANGELOG.md)
    - [Commits](peaceiris/actions-gh-pages@v3...v4)

    ---
    updated-dependencies:
    - dependency-name: peaceiris/actions-gh-pages
      dependency-type: direct:production
      update-type: version-update:semver-major
    ...

    Signed-off-by: dependabot[bot] <support@github.com>
    Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

commit 59d50bc
Author: Nathan Henrie <n8henrie@users.noreply.github.com>
Date:   Mon Apr 15 01:40:27 2024 -0600

    espanso: enable module on darwin

commit 9f32c66
Author: Jose Plana <Jose.Plana.Mario@telefonica.com>
Date:   Wed Apr 10 13:34:46 2024 +0200

    k9s: configuration files in Darwin without XDG

    Support alternate configuration files for k9s in darwin where XDG is
    not mandated and k9s expects configuration files in
    `~/Library/Application Support/k9s/`.

commit 76a1650
Author: Jose Plana <Jose.Plana.Mario@telefonica.com>
Date:   Wed Apr 10 10:24:46 2024 +0200

    k9s: fix typos in configuration file names

commit 630a099
Author: Philipp Mildenberger <philipp@mildenberger.me>
Date:   Sun Apr 14 08:58:16 2024 +0200

    nushell: fix nushell config path on darwin

commit 4cc3c91
Author: home-manager-bot <106474382+home-manager-bot@users.noreply.github.com>
Date:   Sun Apr 14 08:56:05 2024 +0200

    flake.lock: Update

    Flake lock file updates:

    • Updated input 'nixpkgs':
        'github:NixOS/nixpkgs/fd281bd6b7d3e32ddfa399853946f782553163b5' (2024-04-03)
      → 'github:NixOS/nixpkgs/1042fd8b148a9105f3c0aca3a6177fd1d9360ba5' (2024-04-10)

    Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

commit f33d508
Author: Mitchell Skaggs <skaggsm333@gmail.com>
Date:   Sat Apr 13 12:59:33 2024 -0500

    rio: remove redundant `lib.mdDoc` call

    This is an error as of NixOS/nixpkgs#303841

    It seems to have been missed in nix-community#4215

commit 8fdf329
Author: MiSumiSumi <dragon511southern@gmail.com>
Date:   Sat Apr 13 23:50:15 2024 +0900

    neovim: enable use of external package manager (nix-community#5225)

    * neovim: add extraWrapperArgs option

    pass external arguments to neovim-unwrapper
    this gives users more flexibility in managing neovim configuration

    * neovim: add test for `extraWrapperArgs`

commit 40ab43a
Author: Bryn Edwards <bryn@converge-digital.com>
Date:   Sat Apr 13 07:27:43 2024 +0100

    foot: set PATH in server's systemd unit file

    If not set, foot's terminal spawning shortcut will not work as the
    `footclient` binary is not on the server's PATH.

commit 3135748
Author: Ramses <ramses@well-founded.dev>
Date:   Wed Apr 10 16:39:52 2024 +0200

    fish: use the subcommand style for the status command (nix-community#4584)

    The flag style has been deprecated and will eventually be removed.

commit 18f89ef
Author: Tony Zorman <soliditsallgood@mailbox.org>
Date:   Wed Apr 10 08:29:32 2024 +0200

    firefox: add containersForce flag

    Firefox, upon exit, creates the default containers.json file in place of
    the one that home-manager created. This leads to errors when switching
    to a new profile, as home-manager is careful with overwriting existing
    files. The added option toggles that behaviour.

    Closes: nix-community#4989

commit b00d0e4
Author: Jack W <29169102+Jack5079@users.noreply.github.com>
Date:   Tue Apr 9 14:48:15 2024 -0400

    bun: add module

commit 40a9961
Author: Mario Rodas <marsam@users.noreply.github.com>
Date:   Tue Apr 9 01:57:29 2024 -0500

    fzf: add compatibility with fzf≥0.48.0

    fzf 0.48.0 [1] changed the way it integrates with shells.

    [1] https://github.com/junegunn/fzf/releases/tag/0.48.0

commit a561ad6
Author: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Date:   Sun Apr 7 03:59:32 2024 +0000

    flake.lock: Update

    Flake lock file updates:

    • Updated input 'nixpkgs':
        'github:NixOS/nixpkgs/d8fe5e6c92d0d190646fb9f1056741a229980089' (2024-03-29)
      → 'github:NixOS/nixpkgs/fd281bd6b7d3e32ddfa399853946f782553163b5' (2024-04-03)

commit b787726
Author: Smaug123 <patrick+github@patrickstevens.co.uk>
Date:   Fri Apr 5 14:05:00 2024 +0100

    home-manager: extract inline shell script to file
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.

None yet

4 participants