rustdoc: Restore --default-theme, etc, by restoring varname escaping by ijackson · Pull Request #87288 · rust-lang/rust

ijackson · 2021-07-19T17:13:08Z

cd0f93193c84ddc6698f9b65909da71c084dcb74
Use Tera templates for rustdoc.

dropped the following transformation from the keys of the default settings element's data- attribute names:

.map(|(k, v)| format!(r#" data-{}="{}""#, k.replace('-', "_"), Escape(v)))

The Escape part is indeed no longer needed, because Tera does that for us. But the massaging of - to _ is needed, for the (bizarre) reasons explained in the new comments.

I have tested that the default theme function works again for me. I have also verified that passing (in shell syntax)

'--default-theme="zork&"'

escapes the value in the HTML.

Closes#87263

rust-highfive · 2021-07-19T17:13:11Z

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

rust-highfive · 2021-07-19T17:13:12Z

r? @CraftSpider

(rust-highfive has picked a reviewer for you, use r? to override)

ijackson · 2021-07-19T17:14:50Z

@jsha would you care to confirm my suppositions in my commit message, that your removal of the - to _ transformation wasn't for some good reason that I haven't spotted?

@jyn514 Not sure if you want to self-assign this for review...

GuillaumeGomez · 2021-07-19T17:54:34Z

Thanks for this fix! Could you add a test to prevent this regression to happen again please? If it cannot be checked in src/test/rustdoc, I'm sure it can be checked in src/test/rustdoc-gui.

jyn514 · 2021-07-19T21:29:27Z

@GuillaumeGomez I think it would need to be in src/test/run-make because it needs a compiletime flag. But that seems painful since I don't know how you'd test the JavaScript from the command line. Do you think it makes sense to add support for cli args to rustdoc-gui?

GuillaumeGomez · 2021-07-20T08:20:04Z

We already support to pass some arguments in rustdoc-gui, so it can be extended without problem.

ijackson · 2021-07-20T10:54:30Z

But that seems painful since I don't know how you'd test the JavaScript from the command line.

I thought about adding a test for this. It seems quite difficult.

In my game system Otter I need to do this kind of testing and I have an arrangement involving firefox, geckodriver, bubblewrap, webdriver/thirtyfour, etc. etc. etc. I have found this very complex (and it is also very slow, although this may matter less for rust-lang/rust whose tests already take quite a while). It is a pain to work with. Making it reliable has been really hard work (and even so I still seem t have a ~1/1000 probability of random lossage which I haven't diagnosed - some race, probably). Having built and worked with such a thing, I think it would be a net disbenefit to add a webdriver-based in-browser test to rust-lang/rust for the benefit of rustdoc.

This particular regression would only have been detected by a test which assembled an HTML DOM from the rustdoc-generated HTML, and then ran the Javascript, and then looked at the resulting effective CSS to see what the style was. I.e. it demands an HTML DOM implementation, which is basically a browser. I'm not aware of anything significantly short of a browser for this, but perhaps someone can suggest something.

GuillaumeGomez · 2021-07-20T10:56:37Z

Like said previously, take a look at src/test/rustdoc-gui. 😉

ijackson · 2021-07-20T10:58:45Z

Like said previously, take a look at src/test/rustdoc-gui. wink

I went and looked. Good lord! You've already got a thing. Wow. OK, I will see what I can do.

It might not happen right away. Would it be OK with you to land the fix without the test?

ijackson · 2021-07-20T11:01:43Z

 * branch dd7f545a69e4b720407e458bf4ade0b207bbf9ee -> FETCH_HEAD
fatal: remote error: upload-pack: not our ref 9ad6e5b8f68ee4bcd85886e50b8b0a70cbb91a52
Errors during submodule fetch:
src/tools/cargo
Error: Process completed with exit code 1.

???

GuillaumeGomez · 2021-07-20T11:02:38Z

Like said previously, take a look at src/test/rustdoc-gui. wink
I went and looked. Good lord! You've already got a thing. Wow. OK, I will see what I can do.
It might not happen right away. Would it be OK with you to land the fix without the test?

We had a lot of regressions because we didn't add tests, so I'd really prefer to have the test included with the fix (makes it easier when going through PRs as well).

ijackson · 2021-07-20T11:04:08Z

Like said previously, take a look at src/test/rustdoc-gui. wink
I went and looked. Good lord! You've already got a thing. Wow. OK, I will see what I can do.
It might not happen right away. Would it be OK with you to land the fix without the test?
We had a lot of regressions because we didn't add tests, so I'd really prefer to have the test included with the fix (makes it easier when going through PRs as well).

:-(. I think this test is rather more work than the fix. I would also like to point out that I am fixing a thing that someone else broke (doing a thing I approve of, but, still...) and now a thing I actually use day to day is broken.

ijackson · 2021-07-20T11:04:52Z

FTAOD I definitely see the value in the test. It's in my interests to do the test so no-one breaks this again!

GuillaumeGomez · 2021-07-20T11:05:31Z

I can write it for you if you prefer? But I'll need a "scenario" and what to check to ensure everything is working as expected.

jyn514 · 2021-07-20T13:12:34Z

@GuillaumeGomez I don't think it makes sense to block the fix on the test. It's enough to open an E-needs-test isuse.

ijackson · 2021-07-20T13:38:37Z

I can write it for you if you prefer?

That would be awesome!

But I'll need a "scenario" and what to check to ensure everything is working as expected.

So I think the test would look something like this:

Run cargo doc --default-theme=ayu (or maybe rustdoc instead of cargo) on some crate (the detail of what is documented doesn't matter)
Load the result in a browser session (any page will do, eg the crate index.html). (The browser session ought to have a fresh profile, so no existing web storage, but I guess your test isolation would ensure that anyway.)
Wait for page rendering to complete and the JS to run. I am hoping your test framework has a thing already to do this.
Do "some check" that the display is as expected. Either check the average luminance of a screenshot :-) or more sensibly check the computed style (or maybe, the declared style) of the <body> element. Specifically, background-color which should be white with the default (light) theme and something dark for ayu (currently, #0f1419, apparently, but I suggest checking that every element is less than 0x40, or some such).

ijackson · 2021-07-20T14:00:51Z

I have double checked my test cases and the way to run it has to be either rustdoc --default-theme=ayu on a plain rust file with no dependencies (in my test, rustdoc t.rs generated a doc/ directory containing a useable doc/src/t/t.rs.html reflecting the selected theme).

Or using RUSTDOCFLAGS='--default-theme=ayu' cargo doc

ijackson · 2021-07-20T14:01:49Z

Squashed and force pushed in the hope that the retry won't hit that submodule problem.

GuillaumeGomez · 2021-07-20T14:20:31Z

@GuillaumeGomez I don't think it makes sense to block the fix on the test. It's enough to open an E-needs-test isuse.

Generally I agree but such issues tend to take very long to get tests, which is why I'm not super eager to merge without tests. The beta is "far away" so I prefer to write the test directly in the PR (even if I'm the one writing the test, doesn't matter).

ijackson · 2021-07-20T15:13:04Z

The job mingw-check failed!

I could swear I had run rustfmt all this already. Apparently I botched that. Fixed now.

GuillaumeGomez · 2021-07-20T15:17:21Z

It happens to me all the time as well. Welcome to the club I guess?

GuillaumeGomez · 2021-07-20T19:51:03Z

Ok, seems like the CI is happy now. I'll add the CI test tomorrow.

In rust-lang#86157 cd0f931 Use Tera templates for rustdoc. dropped the following transformation from the keys of the default settings element's `data-` attribute names: .map(|(k, v)| format!(r#" data-{}="{}""#, k.replace('-', "_"), Escape(v))) The `Escape` part is indeed no longer needed, because Tera does that for us. But the massaging of `-` to `_` is needed, for the (bizarre) reasons explained in the new comments. I have tested that the default theme function works again for me. I have also verified that passing --default-theme="zork&" escapes the value in the HTML. Closesrust-lang#87263. CC: Jacob Hoffman-Andrews <github@hoffman-andrews.com> Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

…rgo config not being applied

GuillaumeGomez · 2021-07-22T10:59:40Z

Ok, I found the issue: when I added the cargo config, it wasn't added because it's part of .gitignore and I only noticed it now... Fun times. :)

GuillaumeGomez · 2021-07-22T11:38:08Z

Ah finally!

@bors: r+

bors · 2021-07-22T11:38:09Z

📌 Commit df6bdd7 has been approved by GuillaumeGomez

…laumeGomez Rollup of 6 pull requests Successful merges: - rust-lang#87270 (Don't display <table> in item summary) - rust-lang#87281 (Normalize generic_ty before checking if bound is met) - rust-lang#87288 (rustdoc: Restore --default-theme, etc, by restoring varname escaping) - rust-lang#87307 (Allow combining -Cprofile-generate and -Cpanic=unwind when targeting MSVC.) - rust-lang#87343 (Regression fix to avoid further beta backports: Remove unsound TrustedRandomAccess implementations) - rust-lang#87357 (Update my name/email in .mailmap) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

ijackson · 2021-07-22T17:12:09Z

Thanks, especially for writing the test!

rust-highfive assigned CraftSpider Jul 19, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 19, 2021