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/manual: remove md-to-db.sh #215324

Merged
merged 22 commits into from
Feb 10, 2023
Merged

nixos/manual: remove md-to-db.sh #215324

merged 22 commits into from
Feb 10, 2023

Conversation

pennae
Copy link
Contributor

@pennae pennae commented Feb 8, 2023

Description of changes

we already have a markdown renderer for module manual chapters to docbook. the remaining non-module manual chapters use a few more features that are not yet present in nixos-render-docs, so lets add those and make md-to-db.sh a thing of the past.

apart from actual bugfixes to the manual and smartquotes/replacements there are no rendering changes to the html manual. manpage is unaffected. unlike md-to-db.sh converting all chapters with nixos-render-docs is reasonably fast and doesn't need to be cached (nor can it be once we're done with the whole deal, so we didn't even try to add caching here).

this should not be considered the final form of manual rendering to docbook. for rendering to html we will need an external document structuring mechanism that we haven't fully worked out yet, but that mechanism should also be used for docbook once it's implemented to avoid duplication.

note: that the nixos manual check fails is expected since md-to-db.sh is gone. this pr includes removal of this workflow.

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.

@pennae pennae requested review from jtojnar, a team, Mic92 and zowoq as code owners February 8, 2023 15:18
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: policy discussion 8.has: documentation labels Feb 8, 2023
@@ -10,9 +10,7 @@ nativeBuildInputs = [ breakpointHook ];
When a build failure happens there will be an instruction printed that shows how to attach with `cntr` to the build sandbox.

::: {.note}
::: {.title}
Copy link
Member

Choose a reason for hiding this comment

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

Possibly because they were not nested properly. I would expect the following to be recognized by the DocBook Writer:

:::: {.note}
::: {.title}
Title
:::

Admonition body
::::

But I never liked the syntax, would be better served by a heading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we had the same thought but nope, that didn't show up in the docbook output. the title just ended up being a paragraph with no further adornment. :(

@@ -67,7 +67,13 @@ When using multiple modules, you may need to access configuration values
defined in other modules. This is what the `config` function argument is
for: it contains the complete, merged system configuration. That is,
`config` is the result of combining the configurations returned by every
module [^1] . For example, here is a module that adds some packages to
module. (If you're wondering how it's possible that the (indirect) *result*
Copy link
Member

Choose a reason for hiding this comment

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

I would like it if footnotes were used more often but I agree that it should not block this work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we're open to adding support for footnotes if there's the wish to keep them. we'll definitely have to add them if we ever decide to render the nixpkgs manual with nixos-render-docs, but this single occurences that's mostly a quip didn't justify the effort just yet.

@@ -68,7 +68,30 @@ let
optionIdPrefix = "test-opt-";
};

sources = lib.sourceFilesBySuffices ./. [".xml"];
sources = runCommand "manual-sources" {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this go in generatedSources?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we could probably merge the two into just generatedSources at this point, but for this PR we wanted to not disturb too much. once we get a different document layout scheme working the whole thing will have to be rewritten from scratch anyway.

CONTRIBUTING.md Show resolved Hide resolved
nixos/doc/manual/default.nix Outdated Show resolved Hide resolved
Copy link
Member

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

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

I really like PEP 634 if you have not figured out it yet 😆

Comment on lines 427 to 451
if (parsed_attrs := _parse_blockattrs(token.info)) is None:
# if we get here we've missed a possible case in the plugin validate function
raise RuntimeError("this should be unreachable")
kind, id, classes = parsed_attrs
if kind == 'admonition':
token.type = 'admonition_open'
token.meta['kind'] = classes[0]
stack.append('admonition_close')
else:
assert_never(kind)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (parsed_attrs := _parse_blockattrs(token.info)) is None:
# if we get here we've missed a possible case in the plugin validate function
raise RuntimeError("this should be unreachable")
kind, id, classes = parsed_attrs
if kind == 'admonition':
token.type = 'admonition_open'
token.meta['kind'] = classes[0]
stack.append('admonition_close')
else:
assert_never(kind)
match _parse_blockattrs(token.info)):
case None:
# if we get here we've missed a possible case in the plugin validate function
raise RuntimeError("this should be unreachable")
case ('admonition', id, classes):
token.type = 'admonition_open'
token.meta['kind'] = classes[0]
stack.append('admonition_close')
case (kind, _id, _classes):
assert_never(kind)

Comment on lines 279 to 301
if (m := _ATTR_BLOCK_PATTERN.fullmatch(info)) is None:
return None
if (parsed_attrs := _parse_attrs(m[1])) is None:
return None
id, classes = parsed_attrs
# check that we actually support this kind of block, and that is adheres to
# whetever restrictions we want to enforce for that kind of block.
if len(classes) == 1 and classes[0] in get_args(AdmonitionKind):
# don't want to support ids for admonitions just yet
if id is not None:
return None
return ('admonition', id, classes)
return None
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (m := _ATTR_BLOCK_PATTERN.fullmatch(info)) is None:
return None
if (parsed_attrs := _parse_attrs(m[1])) is None:
return None
id, classes = parsed_attrs
# check that we actually support this kind of block, and that is adheres to
# whetever restrictions we want to enforce for that kind of block.
if len(classes) == 1 and classes[0] in get_args(AdmonitionKind):
# don't want to support ids for admonitions just yet
if id is not None:
return None
return ('admonition', id, classes)
return None
if (m := _ATTR_BLOCK_PATTERN.fullmatch(info)) is None:
return None
# check that we actually support this kind of block, and that is adheres to
# whetever restrictions we want to enforce for that kind of block.
match _parse_attrs(m[1])):
case (id, [only_class]) \
if only_class in get_args(AdmonitionKind) \
and id is not None: # don't want to support ids for admonitions just yet
return ('admonition', id, classes)
return None

@pennae
Copy link
Contributor Author

pennae commented Feb 8, 2023

I really like PEP 634 if you have not figured out it yet laughing

we like it too! after some discussion on matrix we tried to make the switch to python 3.11, but that would require giving up python3Minimal for the environment so we ultimately didn't do it. switching to !Minimal is a good jump in closure size (and if we include the tests of markdown-it it's a huge jump in build closure size)

is it worth doing anyway? no idea. it would give us match statements and Self types though.

@jtojnar
Copy link
Member

jtojnar commented Feb 8, 2023

Is not match in 3.10, which is already python3Minimal?

@pennae
Copy link
Contributor Author

pennae commented Feb 8, 2023

Is not match in 3.10, which is already python3Minimal?

oh, indeed it is. and with our concerns about 3.9 needing support having been alleviated from those matrix discussion ... we'll change those to matches. tomorrow 😅

@pennae
Copy link
Contributor Author

pennae commented Feb 8, 2023

... or maybe not tomorrow. unfortunately mypy is not sophisticated enough to analyze match statements properly. in all instances we've tried it has failed to properly destructure match arguments and caused incorrect typecheck failures. guess we'll have to discount match again after all 🙁

@pennae
Copy link
Contributor Author

pennae commented Feb 9, 2023

rebased to master due to a conflict in nixos/doc/manual/from_md/release-notes/rl-2305.section.xml. no other changes

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/help-me-understand-why-md-to-db-sh/25288/3

it's not used. holdover from when manpages were written in docbook.
this should be placed before the appendices, not between them. might
even have a good place in the development part, but that's a decision
for another day.
userconfiguration.xml hasn't existed for a while, and this comment will
interfere with processing we'll be doing shortly.
pandoc renders these to multiple docbook paragraphs in a single
definition for the term, not multiple *actual* definitions for the same
term. this is most likely not what is intended here, so let's use
multiple paragraphs instead.
markdown-it parses deflists slitghtly differently than pandoc does. in
these two cases pandoc would find a deflist item while markdown-it would
not, instead it'd find a lone colon and the rest of the text.
the examples for mkPackageOption weren't terminated, leading to pretty
odd nesting of docbook (and thus html) elements. close them properly.

also turn the (likewise unclosed) fenced div containing just an anchor
id and a class that will be silently dropped to an inline anchor while
we're here. we'd have to convert it anyway later.
pandoc drops .title classes when rendering to docbook, so these are
effectively just paragraphs anyway. without support for including them
in a table of contents the complexity of parsing them in
nixos-render-docs won't be warranted.
pandoc would drop these when converting to docbook, just like it dropped
.title block classes.
nixos-render-docs supports inline anchors, but not ids for blocks. it
seems wise to reserve blocks for special cases that don't have other
syntax already, like admonitions.
there's one remaining instance of literal docbook tags in the manual.
replace it with a literal (as has been done for package tags everywhere else).
this is a lot easier than adding footnote support for just the one
instance. if a use case for footnotes appears in the future (e.g. if we
wanted to render the nixpkgs manual with nixos-render-docs as well) this
decision should be reevaluated.
since support for kbd elements was added with explicit intent in #175128
it seems like a good idea to support this in nixos-render-docs instead
of just dropping it in favor of `*F12*` etc. since it's a very rare
thing in the manual and purely presentational it makes sense to use
bracketed spans instead of a new myst role.

the html-elements.lua plugin is now somewhat misnamed, but it'll go away
very soon so we don't want to bother renaming it.
this is pretty much what pandoc calls bracketed spans. since we only
want to support ids and classes it doesn't seem fair to copy the name,
so we'll call them "attributed span" for now. renderers are expected to
know about *all* classes they could encounter and act appropriately, and
since there are currently no classes with any defined behavior the most
appropriate thing to do for now is to reject all classes.
this lets us parse the `[F12]{.keycap}` syntax we recently introduced to
the nixos manual markdown sources. the docbook renderer emits the keycap
element for this class, the manpage renderer will reject it because it's
not entirely clear what to do with it: while html has <kbd> mandoc has
nothing of the sort, and with no current occurences in options doc we
don't have to settle on a (potentially bad) way to render these.
this should've been a core rule from the beginning. not being a core
rule made it always run after smartquotes and replacements, which
could've wrecked the id.
this is used by release notes (and we don't want to break links to
those), and is also technically allowed anyway. we will *not* extend the
regex to allow more characters just yet due to a mozilla recommendation
against it (cf https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/id)
rules are a better place for this. since _post_parse is now empty (and
presumably will never grow) we'll remove that as well.
this is a subset of pandoc's fenced divs. currently we only use this for
admonitions (which get a new name to differentiate them from other kinds
of blocks), but more users will appear soon.
this is currently only supported by the docbook exporter, and even the
docbook exporter doesn't do much with them. we mirror the conversion
pandoc did for consistency with the previous manual chapter conversion,
which is to add just an anchor with the given id. future exporters that
go directly to html might want to do more.
we'll soon add another docbook converter that does not emit a section as
a collection of chapters, but sections or chapters on their own. this
should clarify naming a bit before there can be any confusion.
render all manual chapters to docbook from scratch every time the manual
is built. nixos-render-docs is quick enough at this to not worry about
the cost (needing only about a second), and it means we can remove
md-to-db.sh in the next commit.

no changes to the rendered html manual except for replacements and smartquotes.
with manual chapters no longer needing pandoc for their conversion to
xml we can get rid of this source of confusion, and its huge cache of
xml files.
@pennae
Copy link
Contributor Author

pennae commented Feb 10, 2023

another update for release notes conflicts. going to wait for CI and then merge since this has been approved twice and is liable to produce conflicts at a very regular pace.

@pennae pennae merged commit cdabe91 into NixOS:master Feb 10, 2023
@pennae pennae deleted the md-to-db-- branch February 10, 2023 06:10
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.

4 participants