Stabilise disable minification

[GuillaumeGomez]

Fixes#54819.
Reopening of #54827.

r? @QuietMisdreavus

[ollie27]

What is the use case for this option? Surely if people want to see the none minified versions they can just look at rustdoc's source code.

[GuillaumeGomez]

Well, rustdoc messes with JS content by adding stuff in it. So it's not that simple to get a view of what's really generated unless you disable the minification.

[QuietMisdreavus]

Rustdoc also minifies any user-supplied themes as well as the search index. It also adds an extra line to storage.js with the resource suffix, before minifying that.

This is a debugging flag that @GuillaumeGomez uses personally, and the original PR for this was among a handful of no-comment stabilization PRs that he started at the same time, alongside threads for --themes and --resource-suffix.

It looks like you've added docs since the original PR, but i'm not aware of any tests in this repo for the minifier functionality. (How much is the minifier crate itself tested? That could offer a stand-in for testing the functionality of this feature, though not the flag.) I still think it would be good to have a run-make-fulldeps test that passes this flag to generate docs for a demo crate (probably an empty crate, but it could have anything), then makes sure that rustdoc.css and main.js have the same file contents as their original files in the repo.

Regardless, we should hold an FCP before merging this, since it's a feature stabilization.

[GuillaumeGomez]

Testing is complicated directly on rustdoc since everytime there is an update, we'll have to update the minification tests as well. I added tests (maybe not enough?) in the minifier crate directly. I'll let you judge if there are enough, and if not, I'll add even more! :D

[bors]

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

[Centril]

Ping from triage, @QuietMisdreavus and @GuillaumeGomez, any progress?

[GuillaumeGomez]

Well, I'm still waiting for a definitive opinion. I'll rebase though.

[GuillaumeGomez]

Need to check on gtk-rs if cargo workspaces could fix my issue.

[QuietMisdreavus]

We discussed this at the Rust All-Hands. @GuillaumeGomez discussed a use case for this in gtk-rs, where there were difficulties in writing all the docs at once. However, it seems like putting them into a Cargo workspace could solve the issue, so imperio needs to investigate that. If that fixes that issue, the only remaining use case is debugging, and it's not pressing to have a debugging flag stabilized. If that's the only remaining use, i would move to close this PR.

[bors]

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

[Dylan-DPC-zz]

ping from triage @QuietMisdreavus waiting for your review on this

[QuietMisdreavus]

I still want to see whether @GuillaumeGomez was able to solve the issues in gtk-rs with workspaces instead of using this flag. If workspaces work for gtk-rs, then i don't think this needs to be stabilized.

[GuillaumeGomez]

I didn't take time to do it yet. ><

[bors]

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

[TimNN]

Ping from triage @GuillaumeGomez: What is the status of this PR?

[GuillaumeGomez]

I still need to check on gtk-rs...

[Dylan-DPC-zz]

ping from triage @GuillaumeGomez any updates? is this blocked on anything?

[GuillaumeGomez]

I still need to check the workspace story but it'll need to wait for a gtk-rs update which is taking longer than expected.

[rfcbot]

Team member @QuietMisdreavus has proposed to merge this. The next step is review by the rest of the tagged team members:

• @GuillaumeGomez
• @QuietMisdreavus
• @ollie27
• @onur

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[GuillaumeGomez]

@QuietMisdreavus I applied the changes you suggested.

[QuietMisdreavus]

I was able to write a small test to ensure that the --disable-minification flag runs the minifier as expected:

// compile-flags:--disable-minification// @has search-index.js// @!has - 'var R'/// The quick brown fox jumps over the lazy dog.pubstructSomeStruct;/// The quick brown fox jumps over the lazy dog.pubfnsome_fn(){}

This just checks for the R string array that the minifier adds to the search index. There need to be some items in the crate, otherwise the minifier doesn't have anything useful to run on and skips collapsing the strings. This has the risk of being flaky if the minifier changes later on, but we can at least be sure that it's running in the first place.

[QuietMisdreavus]

I'd like to see if @ollie27 wants to weigh in, but otherwise, if you add that test it LGTM.

[GuillaumeGomez]

I'll add your test.

[GuillaumeGomez]

Test added!

[QuietMisdreavus]

Thanks! Now i just want to wait for ollie27's comment, and we can ride the FCP out.

[ollie27]

What exactly are the issues with the minifier? I am aware of #58849 and I've just opened #61216 but the way I see it, if there are bugs with this feature then it should be fixed or removed. It's just an optimization after all.

[euclio]

The issues with the minifier stem from the fact that it is run on every invocation of rustdoc, instead of after all the documentation has been collected.

For example, since the JavaScript minification only adds to the minified string index (the R array), it's sensitive to the order that the crates are documented. This is fixable by modifying the minifier to re-read the contents R if present, add any new string candidates, sort them, and then fix up any existing R indexing operations to use the new indices. This is complex, and also requires that the minifier has special knowledge of the variables used for minification.

Even with this fixed, there's still a lot of useless work going on per-invocation. A simple solution to both issues would be to wait for all the rustdoc invocations to finish, and then run the minifier once over all the artifacts. However, I think this would require cargo integation, since there otherwise isn't a way to know that all the rustdoc invocations have finished.

However, if minification happened after rustdoc finishes, it would be possible to run a mature, off-the-shelf minifier instead of the built-in one. Third-party minifiers may offer better minification, though obviously they wouldn't be automatic or included for all users.

I understand that saving bandwidth is important, especially to users who have poor internet connections. But, I'm skeptical that minification is rustdoc's responsibility. How much bandwidth/disk space is actually saved? Does it justify the performance cost to all users of rustdoc, including those running it locally? If minification is necessary, I think offloading minification to a step before deploying the documentation in CI or docs.rs is better than minifying inside rustdoc.

[bors]

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

[GuillaumeGomez]

@euclio: Considering the size of the search-index.js file, I think it's necessary. Give it a try on a big library without the minification enabled and you'll see. Otherwise I agree about the string indexation: it should re-read the R index and remake the job.

[QuietMisdreavus]

I think over time i've come around to @euclio's position. I'm aware that rustdoc generates a large file for the search index, but that doesn't stop it from being a performance bottleneck or occasionally generating faulty or inconsistent JS.

I believe our niche as a tool doesn't need to include making sure that we serve optimally-small resources. If some site hosting docs wishes to shrink the resources it loads, it can minify and/or gzip them after the fact. This means we waste less time in the average case (user generating local docs for themselves). I feel like the best place for the minifier is as an external tool that can be called as a separate script - cargo minidoc perhaps - that minifies resources separately. When built into rustdoc and called repeatedly, it seems to create more problems than it solves.

[Dylan-DPC-zz]

Closing this after a discussion with @GuillaumeGomez

[crlf0710]

@rustbot modify labels to -S-inactive +S-inactive-closed