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/make-options-doc: replace xslt with python where possible, remove olinkdb #212289

Merged
merged 4 commits into from
Jan 25, 2023

Conversation

pennae
Copy link
Contributor

@pennae pennae commented Jan 23, 2023

Description of changes

don't use xsltproc to convert options.json into docbook xml. this also lets us skip using nix to convert json to xml, and using python to sort said XML for xsltproc. this one change saves about 10 seconds in the docs build on our machine and is thus probably good enough to include before the MD rendering code is generalized and stuck into a new tool.

also remove the olinkdb because it's not used at all and rather costly to build (taking 10 seconds on our system).

rendered output is largely unchanged except for a single option that used hard tabs for indentation. that one has more spaces now.

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/)
  • 23.05 Release Notes (or backporting 22.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.

this doesn't seem to be set anywhere, not from make-options-doc nor
anywhere else.
@pennae pennae changed the title nixos/make-options-doc: replace xslt with python where possible nixos/make-options-doc: replace xslt with python where possible, remove olinkdb Jan 23, 2023
@pennae pennae requested a review from jtojnar January 23, 2023 19:16
Copy link
Member

@ncfavier ncfavier left a comment

Choose a reason for hiding this comment

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

Very excited to get rid of the XSLT pile of crap.

nixos-search is happy.

nixos/lib/make-options-doc/default.nix Outdated Show resolved Hide resolved
this restores mergeJSON to its former glory if…merging json, and
extracts the MD rendering into a new script that will run instead of the
py+nix+xslt pipeline we previously ran to convert options.json to docbook.

this change alone gives a noticable performance boost when building
docs (18s instead of 27s to build optionsDocBook).

no changes to rendered output, except for a single example in the
rsnapshot module that uses hard tabs for indentation instead of spaces.
this probably isn't important.

docbook warnings remain with mergeJSON since the other processing steps
output single files instead of directories. since we'll only keep the
check until 23.11 this is probably also not important to fix.

also contains a few improvements to error reporting in the MD renderers.
as far as we can tell nixos has only ever had a total of one olink, and
currently has no olinks at all. we can't currently represent olinks in
markdown docs, and if we re-add support for cross-document links they
will take a different form (and not use docbook, which will have to be
phased out before we re-add anything).

the olinkdb is thus unused and takes 10 seconds on our machine to build,
holding up the rest of the manual for no benefit.
@roberth
Copy link
Member

roberth commented Jan 23, 2023

This would break the table of contents in flake.parts. I'm looking into what causes that.

the XSLT pile of crap.

At least it lets me generate a table of contents easily, and manipulate the structure of the generated document. Doing that without docbook is going to be painful, so I'm glad docbook is still involved - for the time being.

@roberth
Copy link
Member

roberth commented Jan 23, 2023

Pushed a fix. Wouldn't have been necessary with a proper xml library, but maybe that's ok.

@pennae
Copy link
Contributor Author

pennae commented Jan 23, 2023

At least it lets me generate a table of contents easily, and manipulate the structure of the generated document

the rendering tool this will all become in due course can have many output formats. we could keep docbook export support around for probably quite a long time, but we'd rather not use it as an intermediate for other outputs if at all possible. ideally we'd add converters targeting other formats that have all the necessary features, but aren't quite as far from the in-tree inputs.

@pennae pennae merged commit d255493 into NixOS:master Jan 25, 2023
@pennae pennae deleted the optionstodb-py branch January 25, 2023 23:33
sternenseemann added a commit to openlab-aux/vuizvui that referenced this pull request Feb 2, 2023
NixOS/nixpkgs#212289 removes the xls files we
relied on in favor of a new solution that goes from options.json to
docbook using a python script. This commit is a band-aid approach of
pulling in the necessary files from an old nixpkgs revision to fix the
build of the manual until we can solve it properly.

Since the intention of upstream is to get rid of docbook for good, it
probably makes sense to port our module documentation to markdown
already when porting our manual build process to the new toolchain.
sternenseemann added a commit to openlab-aux/vuizvui that referenced this pull request Feb 2, 2023
NixOS/nixpkgs#212289 removes the xls files we
relied on in favor of a new solution that goes from options.json to
docbook using a python script. This commit is a band-aid approach of
pulling in the necessary files from an old nixpkgs revision to fix the
build of the manual until we can solve it properly.

Since the intention of upstream is to get rid of docbook for good, it
probably makes sense to port our module documentation to markdown
already when porting our manual build process to the new toolchain.
@sandydoo
Copy link
Contributor

sandydoo commented Feb 2, 2023

- json.dump(convertMD(unpivot(options)), fp=sys.stdout)
+ json.dump(unpivot(options), fp=sys.stdout)

@pennae, this is a breaking change for the commonmark and asciidoc generators. Much like the docbook generator, these worked on the expectation that all of the markdown in optionsJSON would be pre-rendered.

Now that the markdown machinery is in the docbook generator, could we merge all three generators into one?

@pennae
Copy link
Contributor Author

pennae commented Feb 2, 2023

@sandydoo working on it. ultimately all formatters for option docs should go into nixos-render-docs, but we have only so much time in a day. help from others is certainly welcome if you want to jump in 🙂

@sandydoo
Copy link
Contributor

sandydoo commented Feb 3, 2023

@pennae, awesome! What's the plan for the markdown renderer? Are we writing our own or should we try integrating MDRenderer from mdformat?

@pennae
Copy link
Contributor Author

pennae commented Feb 3, 2023

@sandydoo somewhat inclined to write out own actually. mdformat looks like it'd make a good starting point, but nixpkgs markdown has a bunch of extensions that need to be converted or stripped in various ways. mdformat also does things we don't need (like line wrapping) and for asciidoc we'll need to do much of the indentation planning anyway, so the benefit of using an outside library may be negligible. haven't looked yet so can't say for certain

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