Skip to content
This repository has been archived by the owner on Mar 24, 2023. It is now read-only.

Migrate to mdBook #165

Merged
merged 13 commits into from
May 29, 2021
Merged

Migrate to mdBook #165

merged 13 commits into from
May 29, 2021

Conversation

adlerjohn
Copy link
Member

@adlerjohn adlerjohn commented May 29, 2021

Migrate to use mdBook instead of just Markdown. This has a few benefits:

  1. Nicer default theme.
  2. Built-in MathJax support, so inline math renders without additional configuration.
  3. Able to insert code snippets inline 95a69e2: this will allow executable specs (e.g. in Python Using Python syntax as pseudocode #108) to be referenced in the specs easily without having to duplicate anything. Much easier to maintain.

@adlerjohn adlerjohn added the documentation Improvements or additions to documentation label May 29, 2021
@adlerjohn adlerjohn self-assigned this May 29, 2021
@adlerjohn adlerjohn requested review from liamsi and musalbas May 29, 2021 18:33
Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

How do I build this? How do I view the rendered version?

README.md Show resolved Hide resolved
src/README.md Show resolved Hide resolved
src/SUMMARY.md Show resolved Hide resolved

- [Specification](./specs/README.md)
- [Architecture](./specs/architecture.md)
- [Data Structures](./specs/data_structures.md)
Copy link
Member

Choose a reason for hiding this comment

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

Didn't we remove the feeHeader btw?

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I just never updated that figure because it's hard to maintain two copies of the same thing 😬. Will fix in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's overkill to use tikz for these kind of diagrams btw. Way too time-consuming. Can't these be generated from markdown directly somehow?

Copy link
Member Author

Choose a reason for hiding this comment

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

This figure is actually generated from graphviz/dot, not tikz. Markdown doesn't have any figure support (bare Markdown has almost nothing).

Copy link
Member

Choose a reason for hiding this comment

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

How long does it take to create such a diagram in graphviz/dot?

Copy link
Member

@liamsi liamsi May 29, 2021

Choose a reason for hiding this comment

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

Will fix in a separate PR.

Also, you are making sure that this and the other comments are tracked somewhere (in issues ideally)?

Copy link
Member Author

Choose a reason for hiding this comment

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

It takes about 1 minute to update it each time I need to. Just have to add a field or remove one here: https://github.com/lazyledger/lazyledger-specs/blob/aa2abef998f264fb4ba52e2cb026eae712923701/specs/figures/block_data_structures.dot

Copy link
Member

Choose a reason for hiding this comment

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

Yeah OK. that indeed looks simple enough. Sorry, it's been a while since I've looked at this file.

src/SUMMARY.md Show resolved Hide resolved
Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

I like this. Can we automatically deploy this, please (cc @musalbas, #134).
I don't want to ask everyone to setup rust and install mdbook first.

@adlerjohn
Copy link
Member Author

@liamsi I'll add deployment to GitHub pages via CI in a future PR. See https://github.com/adlerjohn/mdbook-ci-test

Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏼

@adlerjohn adlerjohn merged commit a32cba7 into master May 29, 2021
@adlerjohn adlerjohn deleted the adlerjohn/mdbook branch May 29, 2021 19:21
@musalbas
Copy link
Member

Does this break compatibility with Vuepress? Ideally we want all documentation to be on the same website, so that it can be cross references etc.

@musalbas
Copy link
Member

Also, doesn't it make more sense to have all the theming information on another repo like the docs.lazyledger.org repo, and this repo should only contain md files? How will these changes effect the rest of the docs site? The goal is to have docs from multiple repos on the site. Vuepress seems more suitable for this (docs from multiple repos), as mdbook doesn't seem have collapsible sections.

@adlerjohn
Copy link
Member Author

Does this break compatibility with Vuepress?

Not inherently.

  1. If you don't want HTML, plain markdown can be generated from mdBook-flavored markdown (see the markdown plugin https://github.com/rust-lang/mdBook/tree/8201b411aba2f4957f19c4e146ceb54d3682ce39#3rd-party-plugins).
  2. The HTML output from mdBook can be styled with any CSS after the fact.

Ideally we want all documentation to be on the same website, so that it can be cross references etc.

Deployment to GitHub pages can be done to an external site: https://docs.github.com/en/pages/configuring-a-custom-domain-for-your-github-pages-site

Or another action can be made to additionally deploy the compiled HTML to another site. It's not one or the other.

Also, doesn't it make more sense to have all the theming information on another repo like the docs.lazyledger.org repo, and this repo should only contain md files?

That's essentially what's happening here? There's some base CSS that mdBook includes by default, but you can just add your own style on top of the HTML instead of using the auto-generated one. This PR doesn't touch the default CSS.

The ability to insert code snippets inline can't be duplicated by Vuepress alone (or it would look exactly like mdBook, so just use mdBook).

@musalbas
Copy link
Member

Using different doc software on the same site is not a good idea for developer experience, as it would effectively be two different websites. We should stick to one, and mdbook doesn't seem to be suitable for multiple docs. If you look at https://docs.tendermint.com/ for example, they have lots of different docs there, and Specs is a part of it.

The fact that vuepress doesn't support inline snippets means we can't use the source files for vuepress directly. Does the plain markdown generator solve this? (Even so, having Markdown that is specific to a HTML generator seems like poor design by mdbook.)

@adlerjohn
Copy link
Member Author

Using different doc software on the same site is not a good idea for developer experience, as it would effectively be two different websites.

Even so, having Markdown that is specific to a HTML generator seems like poor design by mdbook.

I think the confusion stems from a misunderstanding of how mdBook works. mdBook is a compiler, so it has a frontend (that reads, in this case mdBook-flavored markdown) and a backend (that writes, in this case HTML). If you're not happy with the generated HTML, you can simply configure mdBook to generated different HTML, including one that's plainer (i.e. no sidebar, etc.) so that it can be fed into Vuepress. That the source is mdBook-flavored markdown doesn't enter into the equation, neither does the default HTML output, neither does the default bundled CSS.

@liamsi
Copy link
Member

liamsi commented May 31, 2021

I think @musalbas has some good points. Mainly:

How will these changes effect the rest of the docs site? The goal is to have docs from multiple repos on the site. Vuepress seems more suitable for this (docs from multiple repos), as mdbook doesn't seem have collapsible sections.

We do not really have anything else yet on the docs site though. We could also decide to use mdbook for everything. I'm sure you could make it s.t. mdbook has collapsible sections as this is likely just done via css.

I was mainly happy about this because I could view an OK rendered version of the specs locally.

Independent of the pros/cons of vue and mdbooks, I'm mainly concerned about the fact that we spent time on this right now. And this discussion should have happened earlier in an issue before starting the PR (with buy-in from @musalbas as this has an impact on more than just the specs repo). It took obviously longer than 5 minutes to implement (even just reviewing took longer). Now that this is here, can we quickly make a decision how to proceed?

There are essentially 2 options:

  1. keep it as is, it's relatively easy to rollback later, if necessary (we need to focus on the contents of the specs instead of which renderer to use)
  2. ditch and revert it now and have the above discussion later

I think we should not spend more time on this and go with 1.

@musalbas
Copy link
Member

If you're not happy with the generated HTML, you can simply configure mdBook to generated different HTML, including one that's plainer (i.e. no sidebar, etc.) so that it can be fed into Vuepress.

VuePress is a static website generator that takes Markdown as input. Feeding HTML to VuePress will probably result in CSS theming and links being broken, at worse.

Practically all public developer documentation for crypto projects are using VuePress. Sure, you can probably modify mdBook's navigation system so that it's more suitable for docs from multiple sources, but that's not the purpose it was designed for: "create a book from markdown files".

This PR breaks docs.lazyledger.org, so I'm closing that site since the specs is the only docs on there right now.

Long term this might be ok if the src files can be compiled to plain markdown, by using the links preprocessor in mdBook, outputting to Markdown.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants