-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Add warnings when rustdoc html rendering differs #41991
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
The error (you can see it on travis as well):
|
I think you need to add the following lines to html5ever and html-diff:
These are in log, which means that when building using Rustbuild, log is rustc_private. I think that therefore anything that has log as a dep (direct or transitive) must also have these lines. Note also that if you don't have these lines in html-diff, then you'll get a test error because the CI checks that every dependent crate has such lines. The motivation behind all this is that we don't want to put any stable (non-rustc_private) crates in the Rust sys_root. |
I don't think that should be necessary with #41847... that should've made these flags completely unnecessary. Perhaps it wasn't successful, though. |
@Mark-Simulacrum looks like this tool is compiled in stage0, which doesn't have those changes. @nrc yes for now that's what needs to be added, although those lines will no longer be necessary once the next stage0 passes due to the PR @Mark-Simulacrum mentioned. |
@GuillaumeGomez could we skip running these tests (and thus building with html-diff) in stage0? (I'm not sure if that makes sense) |
☔ The latest upstream changes (presumably #41843) made this pull request unmergeable. Please resolve the merge conflicts. |
I'll try then. Thanks! |
fe01c7c
to
b76a747
Compare
Ok, now new issue:
Anyone has an idea please? :) |
b76a747
to
21ef3d4
Compare
Looks like the problem is still there after a rebase. I tried reproducing locally, but my build got hit with a different error, which I think is just bad local state. Will try again to investigate tomorrow. |
cc @alexcrichton in case this is a Rustbuild bug |
☔ The latest upstream changes (presumably #42212) made this pull request unmergeable. Please resolve the merge conflicts. |
21ef3d4
to
15e017c
Compare
Delete these lines and you're good to go. |
2966be2
to
78eee68
Compare
Funny/interesting bug this time (we're getting close!). When I compile the stage1 version of rustdoc, the non-stage0 @alexcrichton: I confirm it works, thanks a lot! However why the removal of these lines fixed it? |
@nrc (maybe @alexcrichton too?): We have an issue with a duplicate I think:
|
There are two versions of bitflags - the one pulled in from crates.io, and the one in-tree (rustc_bitflags). There are three different version of the crates.io bitflags crate, but this PR does not add any more. |
This is a bug in rustbuild. The compiler already depends on |
Let's wait then. Remains now the problem with |
☔ The latest upstream changes (presumably #42246) made this pull request unmergeable. Please resolve the merge conflicts. |
@GuillaumeGomez The relevant bug in rustbuild has been fixed, so I think we should be able to land this now. Otherwise, you can wait for a week and we can avoid the stage0 mess I think once beta is branched and released. |
I still have the other bug anyway. Putting it again if someone has an explanation:
|
📌 Commit 76e3221 has been approved by |
⌛ Testing commit 76e3221 with merge 1c3f0f443c0721ebf124f01f58f9760dc0fd186b... |
💔 Test failed - status-appveyor |
AWS error? I guess we can retry? @bors: retry |
⌛ Testing commit 76e3221 with merge 7c078ac4504668573917c1064f37c84686280e6d... |
💔 Test failed - status-travis |
|
@bors: r=nrc |
📌 Commit b501d00 has been approved by |
Add warnings when rustdoc html rendering differs
☀️ Test successful - status-appveyor, status-travis |
YEEEEEEEEEEEEEEEEES!!!!! |
This is just undoing changes from rust-lang#41991 because we are not running markdown rendering twice.
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.
No description provided.