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

cargo should clear doc directory between versions #8461

Closed
ehuss opened this issue Jul 7, 2020 · 7 comments · Fixed by #8640
Closed

cargo should clear doc directory between versions #8461

ehuss opened this issue Jul 7, 2020 · 7 comments · Fixed by #8640
Labels
A-rebuild-detection Area: rebuild detection and fingerprinting C-bug Category: bug Command-doc

Comments

@ehuss
Copy link
Contributor

ehuss commented Jul 7, 2020

Rustdoc can fail if you build documentation with different versions of rustdoc. Rustdoc has some persistent files (like search-index.js) which are not internally versioned. If the format of these changes, this can cause things to break. Example:

In a project with at least one dependency:

  1. cargo +1.40.0 doc
  2. cargo +1.41.0 doc --no-deps --open

The result will have javascript errors in the console, and the sidebar and search will be broken.

cc rust-lang/rust#70781

To fix this, Cargo should retain a fingerprint somewhere that tracks the rustc version used when building documentation. Changes to that should cause Cargo to delete the doc directory.

@ehuss ehuss added C-bug Category: bug A-rebuild-detection Area: rebuild detection and fingerprinting Command-doc labels Jul 7, 2020
@CPerezz
Copy link
Contributor

CPerezz commented Jul 25, 2020

Hi @ehuss I'd like to work on that.

I've been checking the code and looks like we could apply this check and delete the doc/ folder here:

What I'm not sure about is the fingerprint and where/how it should be set.
Is there any cache or file we can attach the latest rustc version used to build the docs?

@ehuss
Copy link
Contributor Author

ehuss commented Jul 25, 2020

I don't think there is anything existing. I'm thinking there could probably just be a file stored in the .fingerprint directory that would be just for the doc output. I'm thinking maybe just a JSON file. I can only think of one field for now ("rustc"), and maybe a version field (set to 1? not sure if that's necessary). There are some other bugs with rustdoc fingerprinting, so it would be good to keep it flexible so that those can be more easily fixed in the future.

I'm not sure how much you know about the cargo codebase. Information about fingerprinting can be found in https://github.com/rust-lang/cargo/blob/master/src/cargo/core/compiler/fingerprint.rs. Unfortunately I don't think that can be used in this case because those fingerprints are unit-oriented, but this fingerprint needs to be triggered globally (otherwise, if two units are building docs concurrently, there would need to be some messy coordination).

There is a cache of rustc information at https://github.com/rust-lang/cargo/blob/master/src/cargo/util/rustc.rs, stored in the RustcTargetData which is stored in BuildContext.

@CPerezz
Copy link
Contributor

CPerezz commented Jul 26, 2020

Thanks for the info. I don't know much about the Cargo codebase as you'll probably witness.. hahah.

The JSON file looks like a good solution. Indeed we already have one in the package directory, inside of target/ folder which could maybe be used for this. It already contains the latest toolchain used to compile the lib/bin:

cat .rustc_info.json
{"rustc_fingerprint":5969268850994687783,"outputs":{"16929990295604814582":["___\n",""],"1164083562126845933":["rustc 1.46.0-nightly (346aec9b0 2020-07-11)\nbinary: r......................}

Therefore, we could simply take this version if target/ exists (otherways we directly build the docs) and if it exists, we check whether the version stated in the .rustc_info.json is the same as the one we're using now.

If it is we just let it follow the same logic it has now, if it is older, we delete the whole doc/ folder and we let cargo compile the docs again.

Does it make sense? Or you'll create a specific file for docs fingerprinting?

@ehuss
Copy link
Contributor Author

ehuss commented Jul 27, 2020

I don't think the .rustc_info.json file should be used for fingerprinting. It is just a cache for version information from the compiler. If other commands are run, it will get rebuilt, deleting any information. I think it would probably be best to use a dedicated fingerprint.

@CPerezz
Copy link
Contributor

CPerezz commented Jul 28, 2020

Makes sense. Therefore, as you mentioned, a JSON file under target/ sounds reasonable to you?

Then the logic would be something like:

  1. doc is triggered
  2. Check if target/ exists in the target directory.
    2a. If it doesn't, compile normally.
    2b. If it does, check fingerprint file and check latest cargo version used to compile the docs.
    2ba. If it is the same as the actual one, compile normally.
    2bb. If it's older than the actual cargo version, delete the doc/ folder and compile normally.

What does it seem to you @ehuss ?

@ehuss
Copy link
Contributor Author

ehuss commented Jul 28, 2020

Yea, generally seems correct. To be a little more precise:

Check if target/ exists in the target directory.

There probably isn't a need to specifically check for the directory. It can just try to open the fingerprint file, and if it doesn't exist, it can proceed as usual.

2b. If it does, check fingerprint file and check latest cargo version used to compile the docs.

It will probably be sufficient to check the version of rustc and assume rustdoc is the same. Rustc.verbose_version provides the version info, which can be found in RustcTargetData.

CPerezz added a commit to CPerezz/cargo that referenced this issue Aug 22, 2020
Before compiling, we need to make sure that if there
were any previous docs already compiled, they were
compiled with the same Rustc version that we're
currently using. Otherways we must remove the
`doc/` folder and compile again.

This is important because as stated in rust-lang#8461
the .js/.html&.css files that are generated
by Rustc don't have any versioning.
Therefore, we can fall in weird bugs and behaviours
if we mix different compiler versions of these
js/.html&.css files.
@bors bors closed this as completed in 8fa0827 Feb 13, 2021
@jyn514
Copy link
Member

jyn514 commented Apr 5, 2021

This was kind of a heavy hammer (and caused rustbuild trouble: rust-lang/rust#83530). What do you think about passing a --resource-suffix with the name of the toolchain instead? Then you won't need to remove the whole doc directory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rebuild-detection Area: rebuild detection and fingerprinting C-bug Category: bug Command-doc
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants