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.

Sign up for GitHub

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

Jump to bottom

Migrate from hoedown to pulldown-cmark for markdown parsing #38400

Closed
frewsxcv opened this issue Dec 15, 2016 · 14 comments
Closed

Migrate from hoedown to pulldown-cmark for markdown parsing #38400

frewsxcv opened this issue Dec 15, 2016 · 14 comments
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@frewsxcv
Copy link
Member

For markdown parsing, we currently use rust-lang/hoedown (based on hoedown/hoedown). There's a (somewhat) new Markdown pull parser entirely written in Rust: google/pulldown-cmark. Some reasons to switch:

  • All the safety guarantees that come with anything written in Rust
  • Rust contributors don't need to know C if they want to make changes upstream
  • Don't need to maintain our own C hoedown bindings / pulldown-cmark has an idiomatic Rust API
  • It uses Cargo, which should make integration easy
@frewsxcv
Copy link
Member Author

cc @rust-lang/tools @rust-lang/docs

@GuillaumeGomez has already expressed interest in making the changes for this transition.

@frewsxcv frewsxcv added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Dec 15, 2016
This was referenced Dec 15, 2016
Closed
Closed
Closed
Closed
@frewsxcv
Copy link
Member Author

Via #29329 and critiqjo/rustdoc#2, it looks like @critiqjo has already made some progress on migrating to pulldown-cmark.

@alexcrichton
Copy link
Member

We're not quite ready to pull in crates.io dependencies into the build system yet as we've still got makefiles that don't work with that. On 2016-02-02, however, I'll be deleting the makefiles with glee at which point we are capable of pulling in dependencies from crates.io!

@frewsxcv
Copy link
Member Author

2016-02-02

I'm going to guess you meant 2017 here

@alexcrichton
Copy link
Member

Oops, yes!

@steveklabnik
Copy link
Member

I have been very interested in this for a long time.

@matklad
Copy link
Member

matklad commented Dec 16, 2016

Some random thoughts:

  1. Are these two implementations compatible? Probably not, and that means some docs might break a little bit.

  2. Perhaps this is the chance to specify what exactly is the markdown dialect of the docs?

  3. What's the current plan about using ::foo::bar::baz syntax for cross-link the docs? If we decide to add some kind of linking, will it be compatible with pulldown-mark?

@GuillaumeGomez GuillaumeGomez mentioned this issue Dec 16, 2016
Merged
@frewsxcv
Copy link
Member Author

frewsxcv commented Dec 16, 2016 •
edited
Loading

Are these two implementations compatible? Probably not, and that means some docs might break a little bit.

pulldown-cmark is an implementation of CommonMark.

Hoedown follows the original markdown document, but there are talks about switching to CommonMark.

From what I understand, one of the main reason CommonMark exists is because the original Markdown specification is so vague and there were many subtle differences in implementations. More info in this thread. I'm not familiar enough with either spec to know specific differences.

What's the current plan about using::foo::bar::baz syntax for cross-link the docs? If we decide to add some kind of linking, will it be compatible with pulldown-mark?

This is what led me to open this issue. There aren't any concrete plans right now for this syntax, but if this something that is decided cmark-pulldown can help. There are talks about adding a basic generic directives/plugins syntax into CommonMark which would be great since we wouldn't need to invent our own syntax. Though, if we did invent our own syntax, from what I've seen of the pulldown-cmark API, it seems it would be easier to implement in that than Hoedown.

@critiqjo
Copy link

critiqjo commented Dec 16, 2016 •
edited
Loading

Can we move out the book-generating feature from rustdoc? Since a new book in the making, which is soon to be The Rust Book, and may be this is a good time to do some cleanups?

@steveklabnik
Copy link
Member

steveklabnik commented Dec 16, 2016 •
edited
Loading

Can we move out the book-generating feature from rustdoc?

There is no book generating feature in Rustdoc.

The current book uses a tool called "rustbook", and the new book uses "mdBook". Both are built on top of rustdoc's support for standalone markdown files, but that's it. I wouldn't want to move that out of rustdoc; as it'll be used in other places (like standalone documentation that's not book-like). It would also be a big breaking change, so that's not acceptable.

@critiqjo
Copy link

I see... I think I misinterpreted certain parts of librustdoc the last time went through the source (9 months ago) when I was refactoring it. I'll take a better look soon, and I've yet to look at rustbook.

@Ericson2314
Copy link
Contributor

I see a src/vendor directory? Can it be used until that magic februrary date?

I've wanted to get rid of hoedown for like a year and half now, very excited this is imminent!

@frewsxcv frewsxcv mentioned this issue Mar 7, 2017
Merged
@frewsxcv
Copy link
Member Author

frewsxcv commented Mar 7, 2017 •
edited
Loading

FYI, @GuillaumeGomez has started work on this.

@QuietMisdreavus QuietMisdreavus mentioned this issue Mar 16, 2017
Merged
@frewsxcv
Copy link
Member Author

Completed in #40338 by @GuillaumeGomez! 🎊

There are a few remaining things to take care of after this migration: #40912

For all intents and purposes, this issue is complete though

@polarathene polarathene mentioned this issue Mar 31, 2017
Closed
@Susurrus Susurrus mentioned this issue Dec 4, 2017
Merged
@QuietMisdreavus QuietMisdreavus mentioned this issue Feb 16, 2018
Merged
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Feb 16, 2018
@GuillaumeGomez
Remove hoedown from rustdoc
5bd5bc3 
Is it really time? Have our months, no, *years* of suffering come to an end? Are we finally able to cast off the pall of Hoedown? The weight which has dragged us down for so long?

-----

So, timeline for those who need to catch up:

* Way back in December 2016, [we decided we wanted to switch out the markdown renderer](rust-lang#38400). However, this was put on hold because the build system at the time made it difficult to pull in dependencies from crates.io.
* A few months later, in March 2017, [the first PR was done, to switch out the renderers entirely](rust-lang#40338). The PR itself was fraught with CI and build system issues, but eventually landed.
* However, not all was well in the Rustdoc world. During the PR and shortly after, we noticed [some differences in the way the two parsers handled some things](rust-lang#40912), and some of these differences were major enough to break the docs for some crates.
* A couple weeks afterward, [Hoedown was put back in](rust-lang#41290), at this point just to catch tests that Pulldown was "spuriously" running. This would at least provide some warning about spurious tests, rather than just breaking spontaneously.
* However, the problems had created enough noise by this point that just a few days after that, [Hoedown was switched back to the default](rust-lang#41431) while we came up with a solution for properly warning about the differences.
* That solution came a few weeks later, [as a series of warnings when the HTML emitted by the two parsers was semantically different](rust-lang#41991). But that came at a cost, as now rustdoc needed proc-macro support (the new crate needed some custom derives farther down its dependency tree), and the build system was not equipped to handle it at the time. It was worked on for three months as the issue stumped more and more people.
  * In that time, [bootstrap was completely reworked](rust-lang#43059) to change how it ordered compilation, and [the method by which it built rustdoc would change](rust-lang#43482), as well. This allowed it to only be built after stage1, when proc-macros would be available, allowing the "rendering differences" PR to finally land.
  * The warnings were not perfect, and revealed a few [spurious](rust-lang#44368) [differences](rust-lang#45421) between how we handled the renderers.
  * Once these were handled, [we flipped the switch to turn on the "rendering difference" warnings all the time](rust-lang#45324), in October 2017. This began the "warning cycle" for this change, and landed in stable in 1.23, on 2018-01-04.
  * Once those warnings hit stable, and after a couple weeks of seeing whether we would get any more reports than what we got from sitting on nightly/beta, [we switched the renderers](rust-lang#47398), making Pulldown the default but still offering the option to use Hoedown.

And that brings us to the present. We haven't received more new issues from this in the meantime, and the "switch by default" is now on beta. Our reasoning is that, at this point, anyone who would have been affected by this has run into it already.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@steveklabnik @alexcrichton @frewsxcv @Ericson2314 @critiqjo @matklad