Don't hard-code essential rustdoc files #1312
Conversation
jyn514
added
S-waiting-on-review
|
Example build: |
|
this is awesome :) |
|
Code changes look good to me, running a local build to confirm it (though I guess we have actual tests covering this code path since #1309 now 🥳). |
|
So, fun bug, this breaks searching because the "essential files" version of I assume the way docs.rs handles these versioned-essential-files as existing globally is a workaround for before |
This seems like the same rustdoc bug as rust-lang/rust#83094 - I don't think rustdoc should be using --static-root-path for search-index.js. |
|
Hmm actually that might be necessary. rustdoc uses but writes it to the shared file cache: https://github.com/rust-lang/rust/blob/e7e1dc158c3de232750b568163f6941a184ee8be/src/librustdoc/html/render/write_shared.rs#L381. I think that's because it re-uses the search index for all crates documented at the same time. I guess we could special-case it in docs.rs? |
|
I wonder whether rustdoc shouldn't be applying the suffix to it? What is the purpose of putting a per-toolchain suffix on a dynamically generated file? |
@GuillaumeGomez do you know why rustdoc uses a resource suffix for the search index? |
|
Because it allows you to have multiple search-index files in the same location I assume? I remember that it was done for docs.rs, but I don't remember why at all... |
|
Well, docs.rs isn't using it, so if that's the only reason I think we should remove the suffix. |
|
Let's make sure no one is actually using it first. I'll ask that on twitter and users.rust-lang.org. |
I didn't understand what you meant with this. |
|
The issue here is that the existence of the suffix is used to detect the "essential files" that are shared between all builds, so because it generates |
|
Big alternative: Can we just stop storing the essential files? It appears that we're saving the per-build versions of all these files anyway, so if we stop passing The main downside I see is that would probably affect browser caching, but given the likelihood of different docs you're looking at being built from the same nightly that doesn't seem like a big downside. Also if |
|
Lines 20 to 23 in 07396f1 This also seems like it should be updated to take into account the known shared static files, instead of being a hardcoded regex (e.g. it's copying the |
I'd be fine with this. |
|
That would work too I guess. Just a question: if the file already exists, wouldn't it cost money to copy it again? I guess we can prevent it by checking if the file already exists? |
Can we hold off on this for now and just fix the essential files to be tracked properly? I think the search-index thing could be fixed by special casing that file not to be copied. |
Not only browser-caching, but also CDN caching for us europeans :) But your point stands: the cache is only by nightly version.
That would IMHO be the best way forward. Then we could really cache the static files across a longer timespan in the CDN. In the end, we do host many docs, and they do share static files to an extend. So I would keep the shared-resource logic and try to improve it. |
|
These are the files that are different between the ones we hard-coded and the ones rustdoc adds a resource-suffix to: The new favicon files are correct; merging this will fix #1240. The crates.js and search-index.js files are not - I think it's a rustdoc bug that these files have a resource-suffix appended, because they can't be shared across crates. Here's so I expect it can't be shared across crates either. I'll see about fixing that in rustdoc. |
Yeah, rust-lang/rust#57011 explicitly says
|
|
I think rust-lang/rust#59776 was incorrect, it means that files with a resource-suffix will be shared across crates even if they're incompatible. That links to #316 (comment) which says
I don't know what 'the rocket problem' was by reading that - I would expect it to be the same as whatever's going on with tokio in that issue? @QuietMisdreavus sorry to bother you - do you still remember what you meant back then? I think either way it shouldn't matter because docs.rs never added those files to |
|
Also, it seems really strange to me that rustdoc would emit a file with a version suffix, but load it from |
|
My reading of the "rocket problem" is that it is essentially an issue of inconsistent caching. Imagine that we build the documentation for a crate with one version of rustdoc, that loads (I think the original issue was with semver-constraint-paths rather than rebuilt documentation, but I think that's no longer an issue because we never serve any files under semver-constraint-paths, just redirect to the current matching version). |
Hmm, when would search-index.js be cached but not main.js? Aren't they all loaded at the same time? |
|
If we didn't have resource suffixes on and after a rebuild: The browser when loading |
|
I think another possibility rather than messing with this in rustdoc is to invalidate the CloudFront cache whenever we rebuild a crate - that seems like good practice anyway since otherwise you'll see older versions of docs, and we rebuild infrequently enough it shouldn't be too expensive. |
|
Invalidating the CDN wouldn't be enough, when navigating to a doc page I see the One idea would be to have a different suffix that we could tell rustdoc to stick on dynamic subresources, we could then fill that suffix with something that is per-build so that the |
|
I mean, at that point I don't know if it's worth the effort - I would rather just store the extra 3 files per toolchain, it's trivial compared to our other costs (or we could special-case those three if @pietroalbini thinks the savings are worth it). The immediate problem should be solved by #1324 - even if we do copy files that shouldn't be shared, rustdoc will still load them per-crate, which should avoid any errors. |
|
So I don't forget: this is waiting on #1312 (comment). |
Done - I don't know a great way to test it, but it doesn't have any fewer tests than the rest of docbuilder. Notably, this also fixes #294. |
This avoids having to constantly update docs.rs when new files are added.
This avoids unnecessary upload costs for S3.
The intended use case is for docs.rs, which can now copy exactly the files it cares about, rather than having to guess based on whether they have a resource suffix or not. In particular, some files have a resource suffix but cannot be shared between crates: rust-lang/docs.rs#1312 (comment) The end goal is to fix rust-lang/docs.rs#1327 by reverting rust-lang/docs.rs#1324. This obsoletes `--print=unversioned-files`, which I plan to remove as soon as docs.rs stops using it.
…imulacrum rustdoc: Add unstable option to only emit shared/crate-specific files The intended use case is for docs.rs, which can now copy exactly the files it cares about, rather than having to guess based on whether they have a resource suffix or not. In particular, some files have a resource suffix but cannot be shared between crates: rust-lang/docs.rs#1312 (comment) The end goal is to fix rust-lang/docs.rs#1327 by reverting rust-lang/docs.rs#1324. This obsoletes `--print=unversioned-files`, which I plan to remove as soon as docs.rs stops using it. I recommend reviewing this one commit at a time. r? `@GuillaumeGomez` cc `@Nemo157` `@pietroalbini`
…imulacrum rustdoc: Add unstable option to only emit shared/crate-specific files The intended use case is for docs.rs, which can now copy exactly the files it cares about, rather than having to guess based on whether they have a resource suffix or not. In particular, some files have a resource suffix but cannot be shared between crates: rust-lang/docs.rs#1312 (comment) The end goal is to fix rust-lang/docs.rs#1327 by reverting rust-lang/docs.rs#1324. This obsoletes `--print=unversioned-files`, which I plan to remove as soon as docs.rs stops using it. I recommend reviewing this one commit at a time. r? ``@GuillaumeGomez`` cc ``@Nemo157`` ``@pietroalbini``
This avoids having to constantly update docs.rs when new files are
added.
Closes #1302, closes #1240, closes #294.
r? @Nemo157