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

rustdoc: Don't counts ids twice when using --enable-commonmark #44368

Merged

bors merged 1 commit into rust-lang:master from ollie27:rustdoc_pulldown_ids

Sep 15, 2017

Member

ollie27 commented Sep 6, 2017 •

edited

Loading

cc @GuillaumeGomez
r? @QuietMisdreavus


          rustdoc: Don't counts ids twice when using --enable-commonmark

55f9087

rust-highfive assigned QuietMisdreavus

ollie27 mentioned this pull request

Improve how rustdoc warnings are displayed #44350

Merged

Member

GuillaumeGomez commented Sep 6, 2017

Oh awesome! I think the checks will fail because of one test I updated to make the original html-diff PR get merged though. If not, it means the PR is incomplete.

Member

QuietMisdreavus commented Sep 6, 2017

In terms of "reducing false-positive render difference warnings", this has the same effect as #44350. What test are you thinking of that would be affected by this?

Member Author

ollie27 commented Sep 6, 2017

I think the test change you're referring to was reverted in https://github.com/rust-lang/rust/pull/44238/files#diff-f6ee607baf038ab9752a5e4a57e92d4d so there's no problem.

Member

QuietMisdreavus commented Sep 6, 2017

This passed travis, that's good enough for me. :D See #44350 (comment) for where this leaves that other PR.

Contributor

bors commented Sep 6, 2017

📌 Commit 55f9087 has been approved by QuietMisdreavus

Member

GuillaumeGomez commented Sep 6, 2017

@ollie27: Perfect then!

alexcrichton added the S-waiting-on-bors label

frewsxcv reviewed

View reviewed changes

src/librustdoc/html/render.rs

+                      // Save the state of USED_ID_MAP so it only gets updated once even
+                      // though we're rendering twice.
+                      let orig_used_id_map = USED_ID_MAP.with(|map| map.borrow().clone());
+                      let hoedown_output = format!("{}", Markdown(md_text, RenderType::Hoedown));

Member

frewsxcv Sep 10, 2017

don't need to change it, but in the future, using .to_string() is the same as format!("{}", ..)

Member

frewsxcv commented Sep 10, 2017

@bors rollup

frewsxcv added a commit to frewsxcv/rust that referenced this pull request


          Rollup merge of rust-lang#44368 - ollie27:rustdoc_pulldown_ids, r=Qui…

…etMisdreavus

rustdoc: Don't counts ids twice when using --enable-commonmark

cc @GuillaumeGomez
r? @QuietMisdreavus

frewsxcv mentioned this pull request

Rollup of 8 pull requests #44482

Closed

frewsxcv added a commit to frewsxcv/rust that referenced this pull request


          Rollup merge of rust-lang#44368 - ollie27:rustdoc_pulldown_ids, r=Qui…

b6cc030

…etMisdreavus

rustdoc: Don't counts ids twice when using --enable-commonmark

cc @GuillaumeGomez
r? @QuietMisdreavus

frewsxcv mentioned this pull request

Rollup of 8 pull requests #44483

Closed

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request


          Rollup merge of rust-lang#44368 - ollie27:rustdoc_pulldown_ids, r=Qui…

6a6190f

…etMisdreavus

rustdoc: Don't counts ids twice when using --enable-commonmark

cc @GuillaumeGomez
r? @QuietMisdreavus

GuillaumeGomez mentioned this pull request

Rollup of 18 pull requests #44550

Closed

frewsxcv added a commit to frewsxcv/rust that referenced this pull request


          Rollup merge of rust-lang#44368 - ollie27:rustdoc_pulldown_ids, r=Qui…

…etMisdreavus

rustdoc: Don't counts ids twice when using --enable-commonmark

cc @GuillaumeGomez
r? @QuietMisdreavus

frewsxcv mentioned this pull request

Rollup of 20 pull requests #44561

Closed

QuietMisdreavus mentioned this pull request

Tracking issue (Rustdoc): hoedown -> pulldown migration #44229

Closed

10 tasks

frewsxcv added a commit to frewsxcv/rust that referenced this pull request


          Rollup merge of rust-lang#44368 - ollie27:rustdoc_pulldown_ids, r=Qui…

9761a8d

…etMisdreavus

rustdoc: Don't counts ids twice when using --enable-commonmark

cc @GuillaumeGomez
r? @QuietMisdreavus

frewsxcv mentioned this pull request

Rollup of 23 pull requests #44585

Merged

bors added a commit that referenced this pull request


          Auto merge of #44585 - frewsxcv:rollup, r=frewsxcv

d86a7f7

Rollup of 23 pull requests

- Successful merges: #44131, #44254, #44368, #44374, #44378, #44388, #44430, #44450, #44453, #44472, #44476, #44477, #44485, #44497, #44521, #44534, #44536, #44541, #44552, #44559, #44563, #44569, #44572
- Failed merges:

bors merged commit 55f9087 into rust-lang:master

ollie27 deleted the rustdoc_pulldown_ids branch

September 15, 2017 20:30

QuietMisdreavus mentioned this pull request

Remove hoedown from rustdoc #48274

Merged

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request


          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

Labels

S-waiting-on-bors