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

Remove hoedown from rustdoc #48274

Merged
merged 4 commits into from
Feb 18, 2018
Merged

Conversation

GuillaumeGomez
Copy link
Member

Finally the time has come!

r? @QuietMisdreavus

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 16, 2018
@steveklabnik
Copy link
Member

+106 −1,358 Nice!

I'm good with this, but I'll let @QuietMisdreavus deal the killing blow; you two have been fighting this for a long time, I don't want to steal the thunder 😉

@QuietMisdreavus
Copy link
Member

Closes #44229

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:

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.


I'll give this a real review shortly, i just wanted to add the history because this feels like a big deal. >_>

@QuietMisdreavus
Copy link
Member

QuietMisdreavus commented Feb 16, 2018

[00:50:46] rustdoc: Unrecognized option: 'deny-render-differences'
[00:50:46] Try 'rustdoc --help' for more information.

Bootstrap adds this flag whenever it renders documentation, because difference warnings kept coming back up in the standard library. Here's where it's added:

// While we can assume that `-Z unstable-options` is set, let's also force rustdoc to panic
// if pulldown rendering differences are found
cmd.arg("--deny-render-differences");

@killercup
Copy link
Member

Speaking of historic events, to capture this one I made a short video of @GuillaumeGomez submitting this PR!

//!
//! [foo]: url 'title & <stuff> & "things"'

// @has 'foo/index.html' 'title &amp; &lt;stuff&gt; &amp; &quot;things&quot;'
Copy link
Member

Choose a reason for hiding this comment

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

While i know why this test was put in (a change related to intra-doc-links changed how we handled link titles in Hoedown), i don't think this test needs to be deleted? It's still something worth verifying, in case we mess with the rendering process in a similar way with Pulldown. Just take out the compile-flags directive, IMO.

@QuietMisdreavus
Copy link
Member

Barring the travis error and my one comment, this PR is good! Because of how much effort has gone into this process, i'm a little wary of "just" pushing the button, but as far as i'm concerned, we're ready.

@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 16, 2018
@Mark-Simulacrum
Copy link
Member

I'd like the history from #48274 (comment) to be added to the commit we're landing or be put into the PR description so we can keep it in git history as it seems useful to explaining what this commit does.

@GuillaumeGomez
Copy link
Member Author

@Mark-Simulacrum: Ok! I'll reword the commit once updated. :)

@GuillaumeGomez GuillaumeGomez force-pushed the remove-hoedown branch 2 times, most recently from 5ec00e2 to 186384e Compare February 16, 2018 20:37
@QuietMisdreavus
Copy link
Member

Looks like error_index_generator loads librustdoc from the sysroot and uses the Markdown and RenderType types:

[01:00:24]    Compiling error_index_generator v0.0.0 (file:///checkout/src/tools/error_index_generator)
[01:00:25] error[E0432]: unresolved import `rustdoc::html::markdown::RenderType`
[01:00:25]   --> tools/error_index_generator/main.rs:27:53
[01:00:25]    |
[01:00:25] 27 | use rustdoc::html::markdown::{Markdown, PLAYGROUND, RenderType};
[01:00:25]    |                                                     ^^^^^^^^^^ no `RenderType` in `html::markdown`
[01:00:25] 
[01:00:25] error[E0061]: this function takes 2 parameters but 3 parameters were supplied
[01:00:25]    --> tools/error_index_generator/main.rs:103:52
[01:00:25]     |
[01:00:25] 103 |             Some(ref desc) => write!(output, "{}", Markdown(desc, &[], RenderType::Hoedown))?,
[01:00:25]     |                                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected 2 parameters
[01:00:25] 
[01:00:25] error: aborting due to 2 previous errors
[01:00:25] 
[01:00:25] error: Could not compile `error_index_generator`.

@GuillaumeGomez
Copy link
Member Author

@killercup: Be careful on your way to the bed. 😛

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.
@QuietMisdreavus
Copy link
Member

Tests failed in... RBE? Did it have spurious code blocks?

[01:26:02] failures:
[01:26:02] 
[01:26:02] ---- /checkout/src/doc/rust-by-example/src/macros/dsl.md - Domain_Specific_Languages__DSLs_ (line 33) stdout ----
[01:26:02] 	error[E0618]: expected function, found `{integer}`
[01:26:02]  --> /checkout/src/doc/rust-by-example/src/macros/dsl.md:34:9
[01:26:02]   |
[01:26:02] 3 |   1 + 2 = 3
[01:26:02]   |  _________^
[01:26:02] 4 | | (1 + 2) * (3 / 4) = 0
[01:26:02]   | |_______^ not a function
[01:26:02] 
[01:26:02] error[E0070]: invalid left-hand side expression
[01:26:02]  --> /checkout/src/doc/rust-by-example/src/macros/dsl.md:34:9
[01:26:02]   |
[01:26:02] 3 |   1 + 2 = 3
[01:26:02]   |  _________^
[01:26:02] 4 | | (1 + 2) * (3 / 4) = 0
[01:26:02]   | |_____________________^ left-hand of expression not valid
[01:26:02] 
[01:26:02] error[E0308]: mismatched types
[01:26:02]  --> /checkout/src/doc/rust-by-example/src/macros/dsl.md:34:9
[01:26:02]   |
[01:26:02] 3 |   1 + 2 = 3
[01:26:02]   |  _________^
[01:26:02] 4 | | (1 + 2) * (3 / 4) = 0
[01:26:02]   | |_____________________^ expected integral variable, found ()
[01:26:02]   |
[01:26:02]   = note: expected type `{integer}`
[01:26:02]              found type `()`
[01:26:02] 
[01:26:02] error[E0070]: invalid left-hand side expression
[01:26:02]  --> /checkout/src/doc/rust-by-example/src/macros/dsl.md:34:1
[01:26:02]   |
[01:26:02] 3 | / 1 + 2 = 3
[01:26:02] 4 | | (1 + 2) * (3 / 4) = 0
[01:26:02]   | |_____________________^ left-hand of expression not valid
[01:26:02] 
[01:26:02] thread 'rustc' panicked at 'couldn't compile the test', librustdoc/test.rs:295:13
[01:26:02] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:26:02] 
[01:26:02] 
[01:26:02] failures:
[01:26:02]     /checkout/src/doc/rust-by-example/src/macros/dsl.md - Domain_Specific_Languages__DSLs_ (line 33)

@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 17, 2018
@QuietMisdreavus
Copy link
Member

Got one in the unstable book:

[01:28:48] failures:
[01:28:48] 
[01:28:48] ---- /checkout/src/doc/unstable-book/src/language-features/macro-at-most-once-rep.md - macro_at_most_once_rep (line 9) stdout ----
[01:28:48] 	error[E0658]: Using the `?` macro Kleene operator for "at most one" repetition is unstable (see issue #48075)
[01:28:48]  --> /checkout/src/doc/unstable-book/src/language-features/macro-at-most-once-rep.md:11:17
[01:28:48]   |
[01:28:48] 4 |     (something $(,)?) // `?` indicates `,` is "optional"...
[01:28:48]   |                 ^^^
[01:28:48]   |
[01:28:48]   = help: add #![feature(macro_at_most_once_rep)] to the crate attributes to enable
[01:28:48] 
[01:28:48] thread 'rustc' panicked at 'couldn't compile the test', librustdoc/test.rs:295:13
[01:28:48] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:28:48] 
[01:28:48] 
[01:28:48] failures:
[01:28:48]     /checkout/src/doc/unstable-book/src/language-features/macro-at-most-once-rep.md - macro_at_most_once_rep (line 9)

@mark-i-m
Copy link
Member

Ah, sorry! That was me 😛

Copy link
Member

@QuietMisdreavus QuietMisdreavus left a comment

Choose a reason for hiding this comment

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

One very small nit, and we're ready to roll!

Hoedown,
Pulldown,
}

/// A unit struct which has the `fmt::Display` trait implemented. When
/// formatted, this struct will emit the HTML corresponding to the rendered
/// version of the contained markdown string.
/// The second parameter is a list of link replacements
// The third parameter is whether we need a shorter version or not.
Copy link
Member

Choose a reason for hiding this comment

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

The comment line about a "third parameter" is not useful now.

@QuietMisdreavus
Copy link
Member

The preparations are complete. It is time...

Begone, demon of the foul C! Your presence is no longer wanted here! With this strike, I commit you to the depths of history, never to torment our fair land again!

@bors r+

@bors
Copy link
Contributor

bors commented Feb 18, 2018

📌 Commit 6661ebb has been approved by QuietMisdreavus

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 18, 2018
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Feb 18, 2018
…ietMisdreavus

Remove hoedown from rustdoc

Finally the time has come!

r? @QuietMisdreavus
bors added a commit that referenced this pull request Feb 18, 2018
Rollup of 6 pull requests

- Successful merges: #48194, #48273, #48274, #48275, #48282, #48312
- Failed merges:
@bors bors merged commit 6661ebb into rust-lang:master Feb 18, 2018
@GuillaumeGomez GuillaumeGomez deleted the remove-hoedown branch February 18, 2018 21:11
@killercup
Copy link
Member

yay!

@Mark-Simulacrum Mark-Simulacrum added the relnotes Marks issues that should be documented in the release notes of the next release. label Feb 18, 2018
bors added a commit that referenced this pull request Mar 3, 2018
[beta] properly run doctests in standalone markdown files with pulldown

This is a beta-specific fix for #48327, since a different fix landed in nightly (#48274) that is infeasible to backport.

The nature of the issue was that when running doctests on standalone Markdown files, rustdoc names the tests based on the headings in the files. Therefore, with the following `a.md`:

``````markdown
# My Cool Library

This is my cool library!

## Examples

Here's some cool code samples!

```rust
assert_eq!(2+2, 4);
```
``````

Running this file with `rustdoc --test a.md` would show a test named `a.md - my_cool_library::examples (line 9)`. So far, this works just fine between Hoedown and Pulldown. But it gets murkier when you introduce markup into your headings. Consider the following `b.md`:

``````markdown
# My Cool Library

This is my cool library!

## `libcool`

```rust
assert_eq!(2+2, 4);
```
``````

The code surrounding the different renderers handles this differently. Pulldown handles just the first `Text` event after seeing the header, so it names the test `b.md - my_cool_library::libcool (line 9)`. Hoedown, on the other hand, takes all the test within the heading, which Hoedown renders before handing to library code. Therefore, it will name the test `b.md - my_cool_library::_code_libcool__code_ (line 9)`. (Somewhere between rustdoc and libtest, the `</>` characters are replaced with underscores.)

This causes a problem with another piece of code: The one that checks for whether Pulldown detected a code block that Hoedown didn't. The test collector groups the "old tests" listing by the full test name, but it *inserts* with the Hoedown name, and *searches* for the Pulldown name! This creates a situation where when `b.md` from above is run, it can't find a matching test from the ones Hoedown extracted, so it discards it and emits a warning.

On nightly, this has been fixed by... ditching Hoedown entirely. This also removed the code that tracked the different test listings, and made it run the test anyway. Since backporting the Hoedown removal is infeasible (i'm personally relying on the change to ride the trains to give the stabilization enough time to complete), this instead chooses to group the test by the filename, instead of the full test name as before. This means that the test extractor finds the test properly, and properly runs the test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants