Add new tool to check HTML #84480
Add new tool to check HTML #84480
Conversation
GuillaumeGomez
added
the
T-rustdoc
rust-highfive
added
the
S-waiting-on-review
src/bootstrap/test.rs
Outdated
| { | ||
| eprintln!("not running HTML-check tool because `tidy` is missing"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Can you check if this is running in CI and give a hard error instead? You can do that with crate::CiEnv::current() != crate::CiEnv::None.
There was a problem hiding this comment.
Yup, this was skipped in the PR run:
Set({"src/tools/html-checker"}) not skipped for "bootstrap::test::HtmlCheck" -- not in ["src/tools/tidy"]
not running HTML-check tool because `tidy` is missing
There was a problem hiding this comment.
Yes since the command is not installed. I'll do something something similar to my eslint check that I added a while ago or for rustdog-gui.
There was a problem hiding this comment.
What's the point of a check that only works locally? We should test this in CI or it will constantly regress.
There was a problem hiding this comment.
We only need it to work on at least one CI, no need to run it everywhere.
There was a problem hiding this comment.
You're not testing if it runs on at least one CI. If you forget to install tidy, CI will silently pass. I really think it's easier to just require it unconditionally.
There was a problem hiding this comment.
I think this is likely going to want the same resolution we're (coming to) on the browser-ui-test thread, which is that we don't run it by default unless tidy is available (e.g., this check passes). We'd still run it if tidy is available, and if it is explicitly requested.
In the future we'll want to develop a solution that works better for these 'opt-in' style tests, but this should be OK for now.
jyn514
added
the
T-bootstrap
|
cc @Mark-Simulacrum - this is still pretty far from ready to merge but I imagine you'll want to take a look once it's closer to finished. |
jyn514
added
S-waiting-on-author
|
Can the PR description be expanded to indicate what kind of bugs this is trying to address? Additionally, it would be good to provide some indication of the expected error message quality and how many people will hit them (just rustdoc developers changing templates? all rustc developers? random book authors?)... Also, what tool are we using? Tidy is a pretty generic name -- is that available in distros (including e.g. homebrew)? On Windows? I admit that I have no context here, so maybe this is all extensively documented; I just don't know where to look. |
|
I have to admit that I didn't expect to get this much attention on a draft so I documented to the strict minimum... The goal here is to check that the HTML generated by rustdoc is valid (no unclosed tags or equivalent for example). As for It'll be useful only for rustdoc contributors who change the output of the HTML layout (or if someone editing documentation is using plain HTML and does bad stuff). |
|
@Mark-Simulacrum This is addressing #84356. I don't know if
Only rustdoc developers should hit them unless people are using raw HTML in their markdown, in which case it's a good thing they see the errors. Here's some example error (from docs generated by rustdoc): |
GuillaumeGomez
force-pushed
the
html-checker
branch
from
b119a3a to
dbfc2ef
Compare
This comment has been minimized.
This comment has been minimized.
GuillaumeGomez
force-pushed
the
html-checker
branch
2 times, most recently
from
6d0f56c to
5654e86
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
GuillaumeGomez
force-pushed
the
html-checker
branch
from
5654e86 to
a48ba9e
Compare
This comment has been minimized.
This comment has been minimized.
GuillaumeGomez
force-pushed
the
html-checker
branch
2 times, most recently
from
ec2298e to
0d45212
Compare
This comment has been minimized.
This comment has been minimized.
GuillaumeGomez
force-pushed
the
html-checker
branch
from
0d45212 to
02ac73e
Compare
|
I fixed some HTML issues, and ignored some others that seem to require a bit more work. I also added the installation of cc @Mark-Simulacrum (at least checking if bootstrap and Dockerfile changes are good for you) |
This comment has been minimized.
This comment has been minimized.
GuillaumeGomez
force-pushed
the
html-checker
branch
from
02ac73e to
d849094
Compare
|
Ping from triage: |
|
@Mark-Simulacrum The HTML changes are being done in #84703. @JohnCSimon It's waiting for #84703 to be merged. |
Clean up dom The commits come from rust-lang#84480. They were errors reported by the `tidy` script that we will use to ensure that the HTML generated by rustdoc is valid. I checked carefully that there were no difference so in principle it should be exactly the same rendering but a double-check would be very appreciated in case I missed something. Extra note: `<h4>` and some `<h3>` tags were replaced by `<div>` because they're not supposed to contain tags as they currently do. r? `@jsha`
GuillaumeGomez
force-pushed
the
html-checker
branch
from
05e85a4 to
1ae3cd7
Compare
|
Since #84703 was merged, I used this opportunity to rebase and clean things up a bit by removing some checks and only run |
| @@ -58,12 +46,21 @@ fn check_html_file(file: &Path) -> usize { | |||
| } | |||
| } | |||
|
|
|||
| const DOCS_TO_CHECK: &[&str] = | |||
| &["alloc", "core", "proc_macro", "implementors", "src", "std", "test"]; | |||
There was a problem hiding this comment.
Rather than only checking specific crates, could you instead filter out the books you don't want to check?
There was a problem hiding this comment.
There are A LOT of different ones and I can't know for sure which one will be added. I initially filtered out but it was a huge mess. We don't generate that many crates in the end so I preferred going this way.
EDIT: and some of them don't have "book" in their name, making things funnier, like "rustdoc". :3
|
Ah and the check was supposed to fail so I guess it isn't run by default, gonna need to fix that too. |
GuillaumeGomez
force-pushed
the
html-checker
branch
from
eb1a9d0 to
a876f42
Compare
This comment has been minimized.
This comment has been minimized.
GuillaumeGomez
added
S-waiting-on-review
|
@Mark-Simulacrum I made a change suggested by @jyn514, but that would force all CI checks on linux to have |
|
I would expect this to use the same method used by the rustdoc-gui tests: we run by default if tidy is installed in the environment and explicitly request a run on one of the builders. If the html check is explicitly requested we would assert tidy is installed. |
|
Ok, gonna use the same mechanism then. |
GuillaumeGomez
force-pushed
the
html-checker
branch
from
a876f42 to
13615e1
Compare
GuillaumeGomez
force-pushed
the
html-checker
branch
from
13615e1 to
82feb9c
Compare
|
Updated it, but with this change, it's not run anymore on the three "fast" CIs. Considering that we want to run this check on every PR, I suppose it's not an issue? |
…r=jsha Rustdoc html fixes rust-lang#84480 latest update allowed me to fix the remaining issues. The last one is coming from `pulldown-cmark` so I'll send them a fix soon. r? `@jsha`
|
No idea why it was closed and I can't re-open this one so I'm simply gonna open another one... |
…k-Simulacrum Add new tool to check HTML Re-opening of rust-lang#84480. r? `@Mark-Simulacrum`
This PR adds a new tool to validate the HTML generated by rustdoc. To be noted, for now it has a lot of errors so opening it as a draft until I fix all reported issues.
Helps with #84356.
r? @jyn514