Skip to content
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

rustdoc: add hash to filename of toolchain files #101702

Merged
merged 4 commits into from
Nov 5, 2022

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Sep 11, 2022

All static files used by rustdoc are now stored in static.files/ and their filenames include a hash of their contents. Their filenames no longer include the contents of the --resource-suffix flag. This clarifies caching semantics. Anything in static.files can use Cache-Control: immutable because any updates will show up as a new URL.

Invocation-specific files like crates-NN.js, search-index-NN.js, and sidebar-items-NN.js still get the resource suffix.

This has a useful side effect: once toolchain files aren't affected by resource suffix, it will become possible for docs.rs to include crate version in the resource suffix. That should fix a caching issue with /latest/ URLs: rust-lang/docs.rs#1593. My goal is that it should be safe to serve all rustdoc JS, CSS, and fonts with infinite caching headers, even when new versions of a crate are uploaded in the same place as old versions.

The --disable-minification flag is removed because it would vary the output of static files based on invocation flags. Instead, for rustdoc development purposes it's preferable to symlink static files to a non-minified copy for quick iteration.

Example listing:

$ cd build/x86_64-unknown-linux-gnu/doc/ && find . | egrep 'js$|css$' | egrep -v 'sidebar-items|implementors' | sort
./crates1.65.0.js
./rust.css
./search-index1.65.0.js
./source-files1.65.0.js
./static.files/ayu-2bfd0af01c176fd5.css
./static.files/dark-95d11b5416841799.css
./static.files/light-c83a97e93a11f15a.css
./static.files/main-efc63f77fb116394.js
./static.files/normalize-76eba96aa4d2e634.css
./static.files/noscript-5bf457055038775c.css
./static.files/rustdoc-7a422337900fa894.css
./static.files/scrape-examples-3dd10048bcead3a4.js
./static.files/search-47f3c289722672cf.js
./static.files/settings-17b08337296ac774.js
./static.files/settings-3f95eacb845293c0.css
./static.files/source-script-215e9db86679192e.js
./static.files/storage-26d846fcae82ff09.js

Fixes #98413

@jsha jsha added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-rustdoc-ui Area: Rustdoc UI (generated HTML) labels Sep 11, 2022
@rustbot
Copy link
Collaborator

rustbot commented Sep 11, 2022

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @Folyd, @jsha

@rust-highfive
Copy link
Collaborator

r? @GuillaumeGomez

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 11, 2022
@rust-log-analyzer

This comment has been minimized.

@Nemo157
Copy link
Member

Nemo157 commented Sep 11, 2022

It will be necessary to figure out how docs.rs will handle this. Currently the shared resource handling is a massive hack that allows a static resource request from anywhere but will only lookup the file from / (so won't work if those files are in a sub-directory). This is probably a good opportunity to figure out how to improve that for future doc builds.

https://github.com/rust-lang/docs.rs/blob/4abe7e9c9bd1298ec3af01878af554a5b60797e7/src/web/mod.rs#L165-L177

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Sep 16, 2022

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

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@jsha
Copy link
Contributor Author

jsha commented Sep 29, 2022

It will be necessary to figure out how docs.rs will handle this. Currently the shared resource handling is a massive hack that allows a static resource request from anywhere but will only lookup the file from / (so won't work if those files are in a sub-directory). This is probably a good opportunity to figure out how to improve that for future doc builds.

https://github.com/rust-lang/docs.rs/blob/4abe7e9c9bd1298ec3af01878af554a5b60797e7/src/web/mod.rs#L165-L177

I filed rust-lang/docs.rs#1863 documenting my current understanding of how this hack works.

Assuming that's correct, I think there's a fairly straightforward way to make docs.rs' SharedResourceHandler work with this change: We can add code such that if the path begins with /static.files/, SharedResourceHandler looks up the full path in storage, rather than just the last component of the path. So long as that change rolls out to docs.rs before this PR lands, we should be good.

@jsha
Copy link
Contributor Author

jsha commented Sep 29, 2022

Another possible way to interface with docs.rs is demonstrated here: ea51f8a

In other words, the default value of --static-root-path is ../static.files/ (insert appropriate number of ../ depending on depth). The nice thing about this approach is that it makes it much harder to miss a mistake like the one fixed in #83094 (confusing "root path" with "static root path" because they default to the same value).

If we adopt this approach, docs.rs would have to make some changes in sync with the rustdoc release:

  • pass --static-root-path /rustdoc.static/ instead of --static-root-path /.
  • for each new toolchain, after running with --emit=toolchain-shared-resources, copy the contents of doc/static.files/ to /rustdoc.static/ in storage.

(note I specifically chose the different directory name rustdoc.static to make it clear it doesn't have to have the same name).

@Nemo157
Copy link
Member

Nemo157 commented Sep 29, 2022

I like being able to configure the static-root-path explicitly. To make synchronization easier I wonder if we could land that flag first (with it being a no-op), then update docs.rs to use it and copy doc/static.files if it exists, then land this change using the flag.

(I'll also try and come up with a way we can test the behavior of docs.rs against this branch so we can confirm the changes work before it's merged).

@jsha
Copy link
Contributor Author

jsha commented Sep 30, 2022

I'm a little confused - --static-root-path already exists, and docs.rs sets it to /. So, no need to land a flag.

That said, we could make a change to docs.rs ahead of this one:

  • start storing essential files in /rustdoc.static/
  • start passing --static-root-path /rustdoc.static/
  • adjust SharedResourceHandler to treat any paths beginning with /rustdoc.static/ as full paths rather than just filenames.

@Nemo157
Copy link
Member

Nemo157 commented Sep 30, 2022

Ah, for some reason I thought the current flag was --root-path. So IIUC with this change nothing would actually need to change on the docs.rs side (we would just start getting files with the hash in the "essential files" instead of the resource-suffix). But it would probably still be good for us to change the path to make it easier to apply caching headers and make it more understandable.

With ea51f8a we would have to change something because --static-root-path would correspond to docs/static.files/* rather than docs/* (we could pre-prepare for that by detecting whether the static.files directory exists and either copy from in there or from the root).

@GuillaumeGomez
Copy link
Member

I think there are a few issues coming up from this change. If I understand correctly, the --resource-suffix option will be added to the filename in addition to the file's hash. For predictability, I'm not sure if it's a good thing (or if we even care).

The content of static_files.rs is duplicated between the array and the struct, which isn't great. I don't think it can be done much better than that though. Maybe we could make a macro I guess so everything would be set in one place to prevent a mismatch between the struct fields and the array elements. What do you think?

I'm really not a big fan to have to update css files manually when a static file is updated. The AUTOREPLACE thing wasn't great but at least it was automated. It should be pretty rare though since we don't update them often so I guess it's a minor concern.

Another very minor concern: why static.files as folder name and not static_files for example?

One last point: I think we'll need a perf check to see if there is any impact with having such a big struct (I think not but better be aware of it if there is an impact).

Overall it seems like a really nice improvement. :)

@jsha
Copy link
Contributor Author

jsha commented Oct 5, 2022

If I understand correctly, the --resource-suffix option will be added to the filename in addition to the file's hash. For predictability, I'm not sure if it's a good thing (or if we even care).

No, there are two types of files now:

  • static files (they get a hash)
  • invocation specific files (they get a --resource-suffix)

Another very minor concern: why static.files as folder name and not static_files for example?

I wanted a name that couldn't conflict with a package or module name. I don't feel strongly about the choice and am happy to take suggestions.

Maybe we could make a macro I guess so everything would be set in one place to prevent a mismatch between the struct fields and the array elements. What do you think?

This is a good idea. I'll see what I can do.

@GuillaumeGomez
Copy link
Member

Thanks for the answers, it all make more sense. Keeping the . in the folder name is the simplest solution then I think.

@bors
Copy link
Contributor

bors commented Oct 6, 2022

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

@rustbot rustbot added this to the 1.67.0 milestone Nov 5, 2022
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 7, 2022
…th, r=notriddle

Fix invalid background-image file name

This is a follow-up of rust-lang#101702.

Apparently the image hash was the wrong one. You can see the error in https://doc.rust-lang.org/nightly/core/primitive.u16.html?search=hello too.

I really need to check if I can adds check for resources load errors in `browser-ui-test`.

cc `@jsha`
r? `@notriddle`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 7, 2022
…th, r=notriddle

Fix invalid background-image file name

This is a follow-up of rust-lang#101702.

Apparently the image hash was the wrong one. You can see the error in https://doc.rust-lang.org/nightly/core/primitive.u16.html?search=hello too.

I really need to check if I can adds check for resources load errors in `browser-ui-test`.

cc ``@jsha``
r? ``@notriddle``
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 8, 2022
…th, r=notriddle

Fix invalid background-image file name

This is a follow-up of rust-lang#101702.

Apparently the image hash was the wrong one. You can see the error in https://doc.rust-lang.org/nightly/core/primitive.u16.html?search=hello too.

I really need to check if I can adds check for resources load errors in `browser-ui-test`.

cc ```@jsha```
r? ```@notriddle```
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 8, 2022
…th, r=notriddle

Fix invalid background-image file name

This is a follow-up of rust-lang#101702.

Apparently the image hash was the wrong one. You can see the error in https://doc.rust-lang.org/nightly/core/primitive.u16.html?search=hello too.

I really need to check if I can adds check for resources load errors in `browser-ui-test`.

cc ````@jsha````
r? ````@notriddle````
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 8, 2022
…th, r=notriddle

Fix invalid background-image file name

This is a follow-up of rust-lang#101702.

Apparently the image hash was the wrong one. You can see the error in https://doc.rust-lang.org/nightly/core/primitive.u16.html?search=hello too.

I really need to check if I can adds check for resources load errors in `browser-ui-test`.

cc `````@jsha`````
r? `````@notriddle`````
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 8, 2022
…th, r=notriddle

Fix invalid background-image file name

This is a follow-up of rust-lang#101702.

Apparently the image hash was the wrong one. You can see the error in https://doc.rust-lang.org/nightly/core/primitive.u16.html?search=hello too.

I really need to check if I can adds check for resources load errors in `browser-ui-test`.

cc ``````@jsha``````
r? ``````@notriddle``````
notriddle added a commit to notriddle/rust that referenced this pull request Nov 10, 2022
…-errors, r=notriddle

Add check in GUI test for file loading failure

Since rust-lang#101702, some resources location need to be updated in case their content changed because then their hash will change too. This will prevent errors like rust-lang#104114 to happen again.

The second commit is to prevent CORS errors: when a file is linked from a file itself imported, the web browser considers they come from a different domain and therefore triggers the error. The option tells the web browser to ignore this case.

cc `@jsha`
r? `@notriddle`
Manishearth added a commit to Manishearth/rust that referenced this pull request Nov 10, 2022
…-errors, r=notriddle

Add check in GUI test for file loading failure

Since rust-lang#101702, some resources location need to be updated in case their content changed because then their hash will change too. This will prevent errors like rust-lang#104114 to happen again.

The second commit is to prevent CORS errors: when a file is linked from a file itself imported, the web browser considers they come from a different domain and therefore triggers the error. The option tells the web browser to ignore this case.

cc ``@jsha``
r? ``@notriddle``
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Nov 10, 2022
…-errors, r=notriddle

Add check in GUI test for file loading failure

Since rust-lang#101702, some resources location need to be updated in case their content changed because then their hash will change too. This will prevent errors like rust-lang#104114 to happen again.

The second commit is to prevent CORS errors: when a file is linked from a file itself imported, the web browser considers they come from a different domain and therefore triggers the error. The option tells the web browser to ignore this case.

cc ```@jsha```
r? ```@notriddle```
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 15, 2022
…ation, r=notriddle

Fix missing minification for static files

It's a fix for rust-lang#101702.

The problem was that `Path::ends_with` doesn't do what we thought it does: it checks if the entire item is the last path part, no just if the "path string" ends with the given argument. So instead, I just used the `extension()` method to get the information we want.

cc `@jsha`
r? `@notriddle`

PS: Is it worth it to add a CI test to ensure that the minification was performed on JS and CSS files or not?
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Nov 15, 2022
…r=notriddle

Add check in GUI test for file loading failure

Since rust-lang/rust#101702, some resources location need to be updated in case their content changed because then their hash will change too. This will prevent errors like rust-lang/rust#104114 to happen again.

The second commit is to prevent CORS errors: when a file is linked from a file itself imported, the web browser considers they come from a different domain and therefore triggers the error. The option tells the web browser to ignore this case.

cc ```@jsha```
r? ```@notriddle```
@jsha jsha deleted the static-files2 branch January 16, 2023 18:08
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 8, 2023
…anup, r=GuillaumeGomez

rustdoc: remove unused ID `mainThemeStyle`

This was added in rust-lang#47620 and used to build the URL of the theme stylesheets. It isn't used any more, because rust-lang#101702 changed it so that the URL was supplied in a `<meta>` tag, which also provides the hashes of the files.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 8, 2023
Rollup merge of rust-lang#115655 - notriddle:notriddle/rustdoc-fe-cleanup, r=GuillaumeGomez

rustdoc: remove unused ID `mainThemeStyle`

This was added in rust-lang#47620 and used to build the URL of the theme stylesheets. It isn't used any more, because rust-lang#101702 changed it so that the URL was supplied in a `<meta>` tag, which also provides the hashes of the files.
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this pull request Oct 18, 2023
The static files placement by `rustdoc` changed in Rust 1.67.0 [1],
but the custom code we have to replace the logo in the generated
HTML files did not get updated.

Thus update it to have the Linux logo again in the output.

Hopefully `rustdoc` will eventually support a custom logo from
a local file [2], so that we do not need to maintain this hack
on our side.

Link: rust-lang/rust#101702 [1]
Link: rust-lang/rfcs#3226 [2]
Fixes: 3ed03f4 ("rust: upgrade to Rust 1.68.2")
Cc: stable@vger.kernel.org
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
ojeda added a commit to Rust-for-Linux/linux that referenced this pull request Oct 18, 2023
The static files placement by `rustdoc` changed in Rust 1.67.0 [1],
but the custom code we have to replace the logo in the generated
HTML files did not get updated.

Thus update it to have the Linux logo again in the output.

Hopefully `rustdoc` will eventually support a custom logo from
a local file [2], so that we do not need to maintain this hack
on our side.

Link: rust-lang/rust#101702 [1]
Link: rust-lang/rfcs#3226 [2]
Fixes: 3ed03f4 ("rust: upgrade to Rust 1.68.2")
Cc: stable@vger.kernel.org
Tested-by: Benno Lossin <benno.lossin@proton.me>
Link: https://lore.kernel.org/r/20231018155527.1015059-1-ojeda@kernel.org
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
ojeda added a commit to Rust-for-Linux/linux that referenced this pull request Oct 19, 2023
The static files placement by `rustdoc` changed in Rust 1.67.0 [1],
but the custom code we have to replace the logo in the generated
HTML files did not get updated.

Thus update it to have the Linux logo again in the output.

Hopefully `rustdoc` will eventually support a custom logo from
a local file [2], so that we do not need to maintain this hack
on our side.

Link: rust-lang/rust#101702 [1]
Link: rust-lang/rfcs#3226 [2]
Fixes: 3ed03f4 ("rust: upgrade to Rust 1.68.2")
Cc: stable@vger.kernel.org
Tested-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Andreas Hindborg <a.hindborg@samsung.com>
Link: https://lore.kernel.org/r/20231018155527.1015059-1-ojeda@kernel.org
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Kaz205 pushed a commit to Kaz205/linux that referenced this pull request Oct 25, 2023
[ Upstream commit cfd9672 ]

The static files placement by `rustdoc` changed in Rust 1.67.0 [1],
but the custom code we have to replace the logo in the generated
HTML files did not get updated.

Thus update it to have the Linux logo again in the output.

Hopefully `rustdoc` will eventually support a custom logo from
a local file [2], so that we do not need to maintain this hack
on our side.

Link: rust-lang/rust#101702 [1]
Link: rust-lang/rfcs#3226 [2]
Fixes: 3ed03f4 ("rust: upgrade to Rust 1.68.2")
Cc: stable@vger.kernel.org
Tested-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Andreas Hindborg <a.hindborg@samsung.com>
Link: https://lore.kernel.org/r/20231018155527.1015059-1-ojeda@kernel.org
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
DawnBreather pushed a commit to DawnBreather/linux-kernel that referenced this pull request Oct 25, 2023
[ Upstream commit cfd9672 ]

The static files placement by `rustdoc` changed in Rust 1.67.0 [1],
but the custom code we have to replace the logo in the generated
HTML files did not get updated.

Thus update it to have the Linux logo again in the output.

Hopefully `rustdoc` will eventually support a custom logo from
a local file [2], so that we do not need to maintain this hack
on our side.

Link: rust-lang/rust#101702 [1]
Link: rust-lang/rfcs#3226 [2]
Fixes: 3ed03f4 ("rust: upgrade to Rust 1.68.2")
Cc: stable@vger.kernel.org
Tested-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Andreas Hindborg <a.hindborg@samsung.com>
Link: https://lore.kernel.org/r/20231018155527.1015059-1-ojeda@kernel.org
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 20, 2024
…r=notriddle

Add check in GUI test for file loading failure

Since rust-lang/rust#101702, some resources location need to be updated in case their content changed because then their hash will change too. This will prevent errors like rust-lang/rust#104114 to happen again.

The second commit is to prevent CORS errors: when a file is linked from a file itself imported, the web browser considers they come from a different domain and therefore triggers the error. The option tells the web browser to ignore this case.

cc ```@jsha```
r? ```@notriddle```
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
…r=notriddle

Add check in GUI test for file loading failure

Since rust-lang/rust#101702, some resources location need to be updated in case their content changed because then their hash will change too. This will prevent errors like rust-lang/rust#104114 to happen again.

The second commit is to prevent CORS errors: when a file is linked from a file itself imported, the web browser considers they come from a different domain and therefore triggers the error. The option tells the web browser to ignore this case.

cc ```@jsha```
r? ```@notriddle```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-ui Area: Rustdoc UI (generated HTML) perf-regression Performance regression. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustdoc: adjust static file names for better cache configuration
10 participants