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

Convert from docbook to mdbook #233

Merged
merged 21 commits into from
Apr 8, 2024
Merged

Convert from docbook to mdbook #233

merged 21 commits into from
Apr 8, 2024

Conversation

noamraph
Copy link
Contributor

@noamraph noamraph commented Apr 3, 2024

Hi,

I find it significantly easier to read mdbook sites, such as the Nix Reference Manual, than the current form of nix-pills. I also think that having the docs as markdown makes it much easier for others to contribute.

So I decided to try and convert nix-pills into mdbook. This PR is the result. The rendered HTML can be viewed at https://noamraph.github.io/nix-pills/.

Most of the work was done by pandoc, and I added manual tweaks and fixes, as can be seen in the branch history.

What do you think?

Thanks,
Noam

TODO:

  • Make nix-build release.nix -I nixpkgs=channel:nixos-unstable build what's needed
  • Make old links continue to work

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/i-converted-nix-pills-from-docbook-xml-into-mdbook-what-do-you-think/42702/1

pills/00-preface.md Outdated Show resolved Hide resolved
@kubukoz
Copy link
Member

kubukoz commented Apr 3, 2024

frankly, I'd approve this just for the removal of all the XML. mdbook is just cherry on top :)

@noamraph
Copy link
Contributor Author

noamraph commented Apr 3, 2024

@jtojnar I followed the suggestions you linked to. I converted all code sections into fenced sections, with the appropriate syntax highlighting. I think it looks very good.

See the result here: https://noamraph.github.io/nix-pills/

One thing which is missing is syntax highlighting for nix code. I understand this is done by using a version of highlight.js that supports nix, but this will wait for now.

Thanks,
Noam

@noamraph
Copy link
Contributor Author

noamraph commented Apr 4, 2024

BTW, I now just took https://nixos.org/manual/nix/stable/highlight.js and put it in the gh-pages branch in my fork, and now nix code is highlighted - for example, see https://noamraph.github.io/nix-pills/08-generic-builders.html

Of course, this should be done in the builder automation, which currently doesn't exist.

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

(discussed in the docs team meeting today)

Thanks! We think this is good for a merge, but CI seems to be failing, please take a look and let us know when it's working.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2024-04-04-documentation-team-meeting-notes-117/42754/1

@noamraph
Copy link
Contributor Author

noamraph commented Apr 4, 2024

@infinisil thanks! I'm not sure what to do about the CI. I don't quite understand how the book is uploaded to https://nixos.org/guides/nix-pills/, and unfortunately I'm still a Nix newbie (that's why I converted the book, so it will be easier for me to learn...) so I don't quite understand how they work. I tried to write a build.yml file that would upload the book to my github-pages, but I tried to install mdbook using Nix and I didn't quite manage to make it work (See here).

So, bottom line - I would love to get some help in making the CI build the book using mdbook!

@DanielSidhion
Copy link
Member

@noamraph I'll help out here - will get to this tomorrow and make sure it outputs things as expected by the homepage. For reference, here is what it expects.

In the meantime, have you used mdbook to generate an epub before? I know there's https://github.com/Michael-F-Bryan/mdbook-epub but I never used it. It would be a good idea to be able to still provide an epub output.

@infinisil
Copy link
Member

I think it would be best to not bother with CI in this PR and instead just make sure that

nix-build release.nix -I nixpkgs=channel:nixos-unstable

works locally and produces the same structure. Keeping the same structure will probably be the trickiest bit, but it's very necessary because otherwise we break links.

@noamraph noamraph force-pushed the mdbook branch 3 times, most recently from 257fb70 to a67830b Compare April 4, 2024 20:10
@noamraph
Copy link
Contributor Author

noamraph commented Apr 4, 2024

Thanks @DanielSidhion !

I managed to make the github action work (by manually adding ~/.nix-profile/bin to the path), and then added epub generation using mdbook-epub. The result is at https://noamraph.github.io/nix-pills/nix-pills.epub , and I think it looks quite nice.

I also enabled Nix syntax highlighting - it turns out you just need to put highlight.js in a theme directory, which I took from the nix reference manual.

custom.css Outdated Show resolved Hide resolved
custom.css Outdated Show resolved Hide resolved
@jtojnar jtojnar self-requested a review April 4, 2024 21:20
@noamraph
Copy link
Contributor Author

noamraph commented Apr 5, 2024

@infinisil I added redirects (just lines in book.toml) from the old html names, such as why-you-should-give-it-a-try.html, to the new ones, such as 01-why-you-should-give-it-a-try.html. Another alternative would have been to rename the markdown files to not have a preceding number, but I think this would have made editing significantly harder. (The XML files did have preceding numbers). So now this link works: https://noamraph.github.io/nix-pills/why-you-should-give-it-a-try

(Note: allowing links not ending with .html is a feature of github-pages, and it seems that also of the current nix-pills hosting. The generated files have a .html suffix)

How does this look to you?

@jtojnar
Copy link
Member

jtojnar commented Apr 5, 2024

Something like the following should work.

I want to look at the output in more detail in the evening but so far I have noticed two issues:

  • Linkchecking web addresses will not work in build sandbox.
  • There are some references to md files in the epub file.
diff --git a/book.toml b/book.toml
index 47af431..8891973 100644
--- a/book.toml
+++ b/book.toml
@@ -34,4 +34,4 @@ git-repository-url = "https://github.com/NixOS/nix-pills"
 "basic-dependencies-and-hooks.html" = "20-basic-dependencies-and-hooks.html"
 
 [output.linkcheck]
-follow-web-links = true
+# follow-web-links = true
diff --git a/default.nix b/default.nix
index aa4aae2..92a271a 100644
--- a/default.nix
+++ b/default.nix
@@ -5,65 +5,27 @@ let
   sources = let
 
       # We want nix examples, but not the top level nix to build things
-      noTopLevelNix = path: type: let
+      onlyBookFiles = path: type: let
           relPath = lib.removePrefix (toString ./. + "/") (toString path);
-        in builtins.match "[^/]*\.nix" relPath == null;
-
-      extensions = [ ".xml" ".txt" ".nix" ".bash" ];
+        in
+        relPath == "book.toml"
+        || relPath == "custom.css"
+        || builtins.match "(theme|pills)(/.*)?" relPath != null;
 
     in lib.cleanSourceWith {
-      filter = noTopLevelNix;
-      src = lib.sourceFilesBySuffices ./. extensions;
+      filter = onlyBookFiles;
+      src = ./.;
     };
 
-  combined = pkgs.runCommand "nix-pills-combined"
-    {
-      buildInputs = [ pkgs.libxml2 ];
-      meta.description = "Nix Pills with as a single docbook file";
-      inherit revCount shortRev;
-    }
-    ''
-      cp -r ${sources} ./sources
-      chmod -R u+w ./sources
-
-      cd sources
-
-      printf "%s-%s" "$revCount" "$shortRev" > version
-      xmllint --xinclude --output "$out" ./book.xml
-    '';
-
-  toc = builtins.toFile "toc.xml"
-    ''
-      <toc role="chunk-toc">
-        <d:tocentry xmlns:d="http://docbook.org/ns/docbook" linkend="book-nix-pills"><?dbhtml filename="index.html"?>
-        </d:tocentry>
-      </toc>
-    '';
-
-  manualXsltprocOptions = toString [
-    "--param section.autolabel 1"
-    "--param section.label.includes.component.label 1"
-    "--stringparam html.stylesheet style.css"
-    "--param xref.with.number.and.title 1"
-    "--param toc.section.depth 3"
-    "--stringparam admon.style ''"
-    "--stringparam callout.graphics.extension .svg"
-    "--stringparam current.docid nix-pills"
-    "--param chunk.section.depth 0"
-    "--param chunk.first.sections 1"
-    "--param use.id.as.filename 1"
-    "--stringparam generate.toc 'book toc appendix toc'"
-    "--stringparam chunk.toc '${toc}'"
-  ];
-
 in {
   html-split = pkgs.stdenv.mkDerivation {
     name = "nix-pills";
 
     src = sources;
 
-    buildInputs = with pkgs; [
-      libxslt
+    nativeBuildInputs = with pkgs; [
+      mdbook
+      mdbook-linkcheck
     ];
 
     installPhase = ''
@@ -71,17 +33,10 @@ in {
 
       # Generate the HTML manual.
       dst=$out/share/doc/nix-pills
-      mkdir -p "$dst"
-      xsltproc \
-        ${manualXsltprocOptions} \
-        --nonet --output "$dst/" \
-        "${pkgs.docbook-xsl-ns}/xml/xsl/docbook/xhtml/chunk.xsl" \
-        "${combined}"
+      mkdir -p "$(dirname "$dst")"
 
-      mkdir -p "$dst/images/callouts"
-      cp -r "${pkgs.docbook-xsl-ns}/xml/xsl/docbook/images/callouts"/*.svg "$dst/images/callouts"
-
-      cp "${./style.css}" "$dst/style.css"
+      mdbook build
+      mv "book/html" "$dst"
 
       mkdir -p "$out/nix-support"
       echo "nix-build out $out" >> "$out/nix-support/hydra-build-products"
@@ -96,9 +51,9 @@ in {
 
     src = sources;
 
-    buildInputs = with pkgs; [
-      libxslt
-      zip
+    nativeBuildInputs = with pkgs; [
+      mdbook
+      mdbook-epub
     ];
 
     installCheckInputs = with pkgs; [
@@ -113,23 +68,10 @@ in {
       # Generate the EPUB manual.
       dst=$out/share/doc/nix-pills
       mkdir -p "$dst"
-      xsltproc \
-        ${manualXsltprocOptions} \
-        --nonet --output "$dst/epub/" \
-        "${pkgs.docbook-xsl-ns}/xml/xsl/docbook/epub3/chunk.xsl" \
-        "${combined}"
-
-      mkdir -p "$dst/epub/OEBPS/images/callouts"
-      cp -r "${pkgs.docbook-xsl-ns}/xml/xsl/docbook/images/callouts"/*.svg "$dst/epub/OEBPS/images/callouts"
-      cp "${./style.css}" "$dst/epub/OEBPS/style.css"
 
-      echo "application/epub+zip" > mimetype
+      mdbook-epub --standalone .
       manual="$dst/nix-pills.epub"
-      zip -0Xq "$manual" mimetype
-      pushd "$dst/epub" && zip -Xr9D "$manual" *
-      popd
-
-      rm -rf "$dst/epub"
+      mv "book/epub/Nix Pills.epub" "$manual"
 
       mkdir -p "$out/nix-support"
       echo "nix-build out $out" >> "$out/nix-support/hydra-build-products"

@DanielSidhion
Copy link
Member

@noamraph I just pushed a commit that reverts the changes on CI and allows the book to be built with Nix. Seems like GitHub is having some issues right now, so the commit is still being processed on their end and should show up whenever they fix their stuff.

@jtojnar I also found it hard to get epubcheck to pass. Even removing the references to md files, there are extra errors being caught by epubcheck so I decided to turn it off and add a comment in the nix code about it. If you want to take a look at it later, go for it.

@jtojnar
Copy link
Member

jtojnar commented Apr 5, 2024

That just means the epub is broken. Maybe we should try upgrading mdbook-epub, the version in Nixpkgs appears to be ancient.

@noamraph
Copy link
Contributor Author

noamraph commented Apr 5, 2024

@DanielSidhion thanks a lot! That's great!

Would it make sense to make the nix build script modify book.toml to have follow-web-links = false instead of follow-web-links = true, so when running outside of the sandbox external links would be checked?

@jtojnar
Copy link
Member

jtojnar commented Apr 5, 2024

Would it make sense to make the nix build script modify book.toml to have follow-web-links = false instead of follow-web-links = true, so when running outside of the sandbox external links would be checked?

Sounds like a good idea.

@DanielSidhion
Copy link
Member

@jtojnar for what it's worth, I opened the epub on my reader and it "works". Not the greatest experience because some pages have almost no content in them, but at least the content seems to be there. I agree with what you said though. Unfortunately I don't have more time today to dedicate to this and try to update mdbook-epub on Nixpkgs, but if this is a blocker and is not resolved in the next days, I'll take a look at it early next week.

@noamraph yep! Sounds good to me. Can you make this change? It should be an extra command in the buildPhase of both outputs.

@jtojnar
Copy link
Member

jtojnar commented Apr 5, 2024

@DanielSidhion I will be able to continue in the evening if no one beats me to it.

@noamraph
Copy link
Contributor Author

noamraph commented Apr 8, 2024

@jtojnar ok, I squashed the commits as you suggested. The final result is exactly the same as was the previous, real, history.

(I feel that I wasted an hour of my life, but whatever 😅 )

Can you merge the PR?

@jtojnar
Copy link
Member

jtojnar commented Apr 8, 2024

Sorry, did not think it would be so bad.

Anyway, thank you, you did a great job.

@jtojnar
Copy link
Member

jtojnar commented Apr 8, 2024

Oh, looking at the nixos-homepage repo, this will still not build since we removed some arguments:

https://github.com/NixOS/nixos-homepage/blob/e691ed43624b2357acca1dcc86e740635e3dab20/flake.nix#L58-L62

@NixOS/documentation-team do you think it is meaningful to include build revision/date in the document and we should re-add it, or should we remove it in nixos-homepage?

@DanielSidhion
Copy link
Member

Huge thanks to both @noamraph and @jtojnar for your work to get a clean epub build 🙏

I don't think it's meaningful to include the build revision and number (where we used the date as a proxy), since sometimes it sends the wrong signal about the "freshness" of the content both when the content is still relevant, but not updated for a while, and when the content is not relevant anymore, but another unrelated change makes it seem like it's fresh.

This PR had way too much churn, so I'll merge it and then make a PR on the homepage to remove those attributes. If there's a good argument to put the build revision and date back into the outputs, we can work that on a different PR.

@DanielSidhion DanielSidhion merged commit 4033045 into NixOS:master Apr 8, 2024
1 check passed
jtojnar added a commit that referenced this pull request Apr 8, 2024
This reverts commit 4033045.

To preserve history.
@jtojnar
Copy link
Member

jtojnar commented Apr 8, 2024

Merged the PR manually to preserve the history.

@DanielSidhion
Copy link
Member

Homepage PR is NixOS/nixos-homepage#1385
Also created a PR to add credit for the current port: #234

thilobillerbeck pushed a commit to NixOS/nixos-homepage that referenced this pull request Apr 9, 2024
Since NixOS/nix-pills#233 got merged, the nix
pills are now built with mdBook. In this process, we removed the
`revCount` and `shortRev` attributes from the mdBook output.

This PR removes those attributes from here to ensure the nix pills can
still be built with the new changes.
@noamraph
Copy link
Contributor Author

noamraph commented Apr 9, 2024

Sorry, did not think it would be so bad.

Anyway, thank you, you did a great job.

@jtojnar Thank you! It's OK, thank you for your hard work!

I realized that the history clean-up gives you less credit than you deserve. It happened because I cherry-picked to the working directory all the commits that I wanted to squash. Is there a way to preserve the credit next time?

Anyway, I appreciate your dedication!

@fricklerhandwerk
Copy link
Contributor

Thanks everyone for making this land, great work!

@jtojnar
Copy link
Member

jtojnar commented Apr 9, 2024 via email

@noamraph
Copy link
Contributor Author

noamraph commented Apr 9, 2024

@jtojnar thanks, I learned something!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants