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

rustdoc: update pulldown + fix spurious rendering difference around footnotes #45421

Merged
merged 2 commits into from
Oct 28, 2017

Conversation

QuietMisdreavus
Copy link
Member

fixes #45420

@rust-highfive
Copy link
Collaborator

r? @GuillaumeGomez

(rust_highfive has picked a reviewer for you, use r? to override)

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 20, 2017
@steveklabnik
Copy link
Member

This does not fix the issue:

WARNING: documentation for this crate may be rendered differently using the new Pulldown renderer.
    See https://github.com/rust-lang/rust/issues/44229 for details.
WARNING: rendering difference in `# The Rust Core Library`
   --> src\libcore\lib.rs:11:0
    /html[0]/body[1]/p[1] Attributes differ in `sup`: expected: `{"id": "supref1"}`, found: `{"id": "fnref1"}`
WARNING: rendering difference in `# The Rust Core Library`
   --> src\libcore\lib.rs:11:0
    /html[0]/body[1]/div[7]/ol[1] Attributes differ in `li`: expected: `{"id": "ref1"}`, found: `{"id": "fn1"}`

@steveklabnik
Copy link
Member

(We should still land this, as I believe it does fix rendering issues, or so I hear; I haven't validated that myself yet. But it doesn't fix the warning)

@QuietMisdreavus QuietMisdreavus changed the title rustdoc: update pulldown renderer rustdoc: update pulldown + don't report differences in footnote link IDs Oct 20, 2017
@QuietMisdreavus
Copy link
Member Author

I've just pushed an update that adds a check to bypass reporting a warning if the difference was with id attributes that follow the pattern pulldown and hoedown use for footnotes. Some structural differences were still fixed with the version bump, but the last rendering difference surrounding footnotes should be gone with that check.

@steveklabnik
Copy link
Member

With my PR + this PR, we have no warnings 🎊

@bors: r+

@bors
Copy link
Contributor

bors commented Oct 21, 2017

📌 Commit cee1991 has been approved by steveklabnik

@kennytm kennytm 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 Oct 21, 2017
@ollie27
Copy link
Member

ollie27 commented Oct 21, 2017

The ids are generated by rustdoc so they could have been changed to match hoedown:

let reference = format!("<sup id=\"supref{0}\"><a href=\"#ref{0}\">{0}\

@frewsxcv
Copy link
Member

@bors rollup

bors added a commit that referenced this pull request Oct 21, 2017
Rollup of 6 pull requests

- Successful merges: #45227, #45356, #45407, #45411, #45418, #45419
- Failed merges: #45421
@QuietMisdreavus
Copy link
Member Author

@ollie27 D'oh! Since this is already in the middle of a rollup, i'll grab that in a separate PR. Nice catch!

@bors
Copy link
Contributor

bors commented Oct 21, 2017

☔ The latest upstream changes (presumably #45430) made this pull request unmergeable. Please resolve the merge conflicts.

@kennytm
Copy link
Member

kennytm commented Oct 21, 2017

@QuietMisdreavus No it failed to make into the rollup 😄, so that change could be put in this PR.

@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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 21, 2017
@QuietMisdreavus QuietMisdreavus changed the title rustdoc: update pulldown + don't report differences in footnote link IDs rustdoc: update pulldown + fix spurious rendering difference around footnotes Oct 21, 2017
@QuietMisdreavus
Copy link
Member Author

@kennytm Oh, didn't notice that, oops. >_>;;

Take two! I just force-pushed a different update that uses @ollie27's suggestion rather than writing in a warning suppression. I ran it locally and it fixed the rendering differences in libcore the same as the other way, only this time it's because it's actually the same.

@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 Oct 22, 2017
@kennytm
Copy link
Member

kennytm commented Oct 25, 2017

@GuillaumeGomez @ollie27 Is this PR good to go?

@GuillaumeGomez
Copy link
Member

Seems good for me.

@QuietMisdreavus
Copy link
Member Author

r? @steveklabnik

Since the footnote mitigation changed after the initial approval, this is worth another look before i blindly re-r+ it.

@steveklabnik
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Oct 27, 2017

📌 Commit af09ba8 has been approved by steveklabnik

@kennytm kennytm 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 Oct 27, 2017
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Oct 28, 2017
…steveklabnik

rustdoc: update pulldown + fix spurious rendering difference around footnotes

fixes rust-lang#45420
kennytm added a commit to kennytm/rust that referenced this pull request Oct 28, 2017
…steveklabnik

rustdoc: update pulldown + fix spurious rendering difference around footnotes

fixes rust-lang#45420
bors added a commit that referenced this pull request Oct 28, 2017
Rollup of 7 pull requests

- Successful merges: #45421, #45449, #45505, #45535, #45549, #45574, #45585
- Failed merges:
@bors bors merged commit af09ba8 into rust-lang:master Oct 28, 2017
@QuietMisdreavus QuietMisdreavus deleted the update-pulldown branch February 16, 2018 15:15
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Feb 16, 2018
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 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.

Footnotes don't display properly under CommonMark
8 participants