Hoedown big comeback! #41290
Hoedown big comeback! #41290
Conversation
src/librustdoc/test.rs
Outdated
| let _ = writeln!(&mut io::stderr(), | ||
| "WARNING: {} test will be run in the next rustdoc version. If it's \ | ||
| not supposed to, please update your documentation and make it \ | ||
| compliant to common mark specifications.", |
There was a problem hiding this comment.
I may not have been present for the discussion, but this leaves a question: What's the timeline for this changeover now? Are we expecting this to follow some kind of stabilization FCP process to turn the commonmark tests back on?
There's also the matter of how loud this might need to be. I wonder how many people manually run the test suites on their projects, or just leave it to CI. If CI reports success even though these warnings come out, how much progress have we made?
There was a problem hiding this comment.
@steveklabnik said "a few releases". And the point is to prevent to avoid current massive breakage. After that, we remove the warning and the tests will fail.
There was a problem hiding this comment.
If that's the case, and we're leaving this on long enough, then I think it'll be fine, as long as we raise a big enough clamor about it one way or another. We may want the warning text to say something like "a later rustdoc version", though, since it's not technically the "next" one.
| } | ||
| } | ||
|
|
||
| pub fn old_find_testable_code(doc: &str, tests: &mut ::test::Collector, position: Span) { |
There was a problem hiding this comment.
I take it this is very similar to the previous hoedown "find testable code" function, just updated to spit them into old_tests instead?
There was a problem hiding this comment.
Absolutely. A bit shorter.
Mark-Simulacrum
added
the
S-waiting-on-author
|
Tidy fixed. |
GuillaumeGomez
force-pushed
the
put-back-hoedown
branch
from
068a8ab
to
eb62d4d
Compare
This would be great, but @brson can't even complete the regular cargobomb run until next week. At that point, we already have a release going. So, I'm not sure :( (I couldn't get cargobomb working personally) |
|
There was a PR for |
| [submodule "src/rt/hoedown"] | ||
| path = src/rt/hoedown | ||
| url = https://github.com/rust-lang/hoedown.git | ||
| branch = rust-2015-09-21-do-not-delete |
There was a problem hiding this comment.
review note: this is the same branch as was here previously. leaving a comment for myself so I don't forget that I checked!
|
This overall looks good to me. Two things:
I think I would write Or something along those lines. I really wish we had a way to provide more and better information for people to help ease the transition, idk. @rust-lang/compiler you have some experience with this kind of thing, any suggestions? |
|
I don't understand; what's the point of this PR? didn't we replace hoedown with pulldown-cmark? |
|
The issue is, the change caused many things that weren't tested as rust code before to be tested now, and those tests failed. Take this: with When we announced the change, nobody really commented, or rather, we had four or five crates that needed updating to fix this. But a cargobomb run identified a lot of crates, way more than we are willing to regress on, that ended up failing their test suites because of this kind of thing. So we need to spread this out over a longer period of time than just one release. Does that make sense? |
|
yes, completely! I didn't even know this was a problem. yikes |
|
Update: I don't think the flag is needed, as once your stuff runs without any warnings, you're good. So maybe focus the error message on that?
|
GuillaumeGomez
force-pushed
the
put-back-hoedown
branch
from
f47f1ee
to
bee0291
Compare
|
@bors: r+ thanks! |
|
📌 Commit bee0291 has been approved by |
…steveklabnik
Hoedown big comeback!
```bash
> cargo +local test
Compiling libc v0.2.20
Compiling sysinfo v0.3.4 (file:///Users/imperio/rust/sysinfo)
Finished dev [unoptimized + debuginfo] target(s) in 3.2 secs
Running target/debug/deps/disk_list-dbd70897f1f7e080
running 1 test
test test_disks ... ok
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured
Running target/debug/deps/sysinfo-8ad11103abdf5941
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured
Doc-tests sysinfo
WARNING: src/sysinfo.rs - (line 45) test will be run in the next rustdoc version. If it's not supposed to, please update your documentation and make it compliant to common mark specifications.
WARNING: src/sysinfo.rs - (line 48) test will be run in the next rustdoc version. If it's not supposed to, please update your documentation and make it compliant to common mark specifications.
running 1 test
test src/sysinfo.rs - (line 14) ... ok
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured
```
r? @rust-lang/docs
|
My personal hope was that we would bring hoedown back in full as part of this, not just for finding testable code. I realize that the only visible regressions are those related to doctests but are there not a large number of rendering regressions as well? Both related to changing to CommonMark but also bugs in the implementation? My assumption would be that the new pulldown-cmark implementation would be behind a flag by default. We would print warnings indicating breakage on doctests immediately and otherwise strongly encourage authors to test their documentation rendering (via something like I fear that this PR currently only fixes cargobomb, and still leaves the door open to a lot of breakage wrt rendering. |
|
@alexcrichton: If we start putting this change behind a flag, no one will use it and the time of adoption will only grow. I was against putting back hoedown (even for this case) and more in favor for a hard break. |
|
@GuillaumeGomez there is some truth to that -- I agree that most people will not opt in, even if we advertise the change widely. Nonetheless, the "best case" procedure is to go through several stages:
Very often, this requires keeping two copies of the code around (one that does the old thing, one that does the new thing), and then diffing the results. In some cases, it's just not possible to get precise warnings -- in that case, we typically try very hard to do warnings where we can. In very few cases, we skip the early stages because there doesn't seem to be much effect in practice. It is true that people often ignore the warnings. But doing it in several stages has the advantage that we at least give people a chance to respond, which means that when the breakage finally comes, it is less frustrating (you know it was coming, after all, or at least you can see that we made an effort to let you know). In short, breaking changes always disturb people, and the best thing is to disturb them as little as we can (btw, if we can come up with ways to do this better and/or with less effort, I'm all ears! Avoiding breakage is a constant struggle.) The other advantage of warnings is that we can sometimes detect them automatically, of course, and hence we can offer up PRs or at least ping people to bring their attention to the fact that warnings will be transitioning into harder errors. |
|
(An example of this procedure is the forwards compatibility lints, which start out as warnings, proceed to deny-by-default, and then make a hard error.) |
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.
r? @rust-lang/docs