Implement Rustdoc versioning checks #8640

CPerezz · 2020-08-22T07:24:49Z

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 #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.

Closes #8461

This intends to be a helper structure similar to `RustTargetData` which includes helper functions which are useful for interacting with the rustdoc fingerprint files among other things.

Sometimes we need to remove the `doc/` directory (if exists) in order to not mix different versions of the js/html/css files that Rustc autogenerates and lack versioning.

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.

rust-highfive · 2020-08-22T07:24:51Z

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

ehuss · 2020-08-28T21:27:49Z

what to do with the possible error we might get back from this fn call

I think it is fine just to return an error. All other commands that delete or replace files will fail if there is something like a permission error. And if rustdoc needs to overwrite a file that it doesn't have permission for, it will also fail.

How am I supposed to test this inside the testing module?

I would make a rustc wrapper that reports a different version. Here is an example of a test that does exactly that. It sets the "RUSTC" environment variable to a custom executable which intercepts the -vV call to rustc and reports a different version, otherwise it calls the normal rustc. Perhaps that code could be factored out and reused.

There are many different approaches how to detect that the directory actually got deleted. I would probably just add a bogus file into the target/doc directory after the first call to cargo doc, and then check that the file was removed after the second call to cargo doc with a different rustc version.

Let me know if you have any other questions.

Is the user does not have permissions or a folder/file can't be created or deleted, Cargo will fail and return an Error indicating the problem.

CPerezz · 2020-09-20T08:30:01Z

Hey @ehuss. I've been working on that for some time and have finished the skeleton for the test of the rustdoc fingerprint. But for some reason, I can't open the fingerprint file inside the tests.
Idk if I'm missing something, but I would appreciate a lot if you could provide to me some extra help since it would be nice to close that.

Thanks!

ehuss · 2020-09-21T19:59:57Z

..build() for ProjectBuilder just writes the source files to disk (Cargo.toml and the .rs files), it doesn't run cargo build or anything like that. I think you need to call something like dummy_project.cargo("doc").run(); first to generate the fingerprint.

I also notice that the current PR won't work because it attempts to write to the fingerprint directory before it is created. The general implementation of path_to_fingerprint doesn't look quite right, either. I'm not sure exactly where, but the fingerprint reading/writing should be done with a Layout instance (which is in Context.files). The Layout "owns" the target directory, and manages its paths.

CPerezz · 2020-11-28T11:33:32Z

Thanks a lot for the suggestions @ehuss. Will take a look to the Layout structure and refactor the impl to work towards that direction.

bors · 2020-12-07T18:10:57Z

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

After refactoring what was done on the latest approach this time we included the rustdoc versioning logic inside the `compiler::rustdoc` fn. - Created `RustDocFingerprint` struct so that is easier to manage all of the logic & functions related to rustdoc versioning as well as scaling it in the future if needed.

- Check wether the fingerprint is created with the correct data after a `rustdoc` execution. - Check wether the previous docs are removed if the version used to compile them reported in the fingerprint is different from the actual one being used by the compiler.

CPerezz · 2020-12-16T08:41:14Z

Hey @ehuss I think I got it now. Thanks for the ideas on the Layout. That allowed me to see the Context and BuildContext which is what I went for in order to solve that.

I belive adding the logic inside the core::compiler::rustdoc() whas the best choice since I can control the creation & removal of the doc directory from there.

Thanks for your help and sorry for the delay.

EDIT:

The test failing has nothing to do with the changes added in this PR AFAIK. The tests I did are passing on my machine.

ehuss · 2020-12-16T18:26:53Z

tests/testsuite/doc.rs

-                    "crate-b",
-                ]
-            "#,
+              [workspace]


Seems like a bunch of unnecessary whitespace changes here.

Cargo fmt was probably who made that happen. Apologies.

ehuss · 2020-12-16T18:28:16Z

tests/testsuite/doc.rs

+            r#"
+        [package]
+        name = "foo"
+        version = "1.2.4"
+        authors = []
+    "#,


Suggested change

r#"

[package]

name = "foo"

version = "1.2.4"

authors = []

"#,

r#"

[package]

name = "foo"

version = "1.2.4"

authors = []

"#,

ehuss · 2020-12-16T18:29:15Z

tests/testsuite/doc.rs

+
+    dummy_project.cargo("doc").run();
+
+    // As the test shows avobe. Now we have generated the `doc/` folder and inside


Suggested change

// As the test shows avobe. Now we have generated the `doc/` folder and inside

// As the test shows above. Now we have generated the `doc/` folder and inside

ehuss · 2020-12-16T18:36:56Z

src/cargo/core/compiler/mod.rs

@@ -649,7 +662,11 @@ fn rustdoc(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult<Work> {
                &mut |line| on_stderr_line(state, line, package_id, &target, &mut output_options),
                false,
            )
-            .chain_err(|| format!("could not document `{}`", name))?;
+            .chain_err(|| format!("Could not document `{}`.", name))?;


This seems like an unrelated change (and the cause of the test error).

Weird. I don't remember changing that TBH. But thanks for the catchup!

ehuss · 2020-12-16T18:46:26Z

tests/testsuite/doc.rs

+
+    // Edit the fingerprint rustc version.
+    // Read fingerprint rustc version and return it as String
+    let get_fingerprint_version = |root_path: std::path::PathBuf| -> String {


I don't think this needs to be a closure. Also, it can all be replaced with dummy_project.read_file("target/doc/.rustdoc_fingerprint.json").

ehuss · 2020-12-16T18:59:53Z

src/cargo/core/compiler/mod.rs

+    // any versioning (See https://github.com/rust-lang/cargo/issues/8461).
+    // Therefore, we can end up with weird bugs and behaviours if we mix different
+    // compiler versions of these files.
+    let (fingerprint, doc_dir_removed) = RustDocFingerprint::check_rustdoc_fingerprint(


This probably isn't a safe place to put this, since we don't want building the Work closure to actually make any changes until it is executed.

I'm not sure where is a good location. It probably should be done once, before it starts draining the queue. Maybe somewhere like in Context::compile, just after the call to preprare? It could check if there are any rustdoc units to build, and if so, check the fingerprint.

I added it here since there's this comment where the doc folder is created. Therefore I assumed this would be the best place to check wether we want to remove it or not etc..

Also, it's important to highlight that inside of the Worker we only write the fingerprint in case we erased the previous one. But the directory removal/fingerprint reading is done outside. So I don't really know if it is unsafe (obviously, you're the expert here) but just writing the fingerprint in case it was previously erased inside of the Worker is unsafe?

This is called for every package that is being documented, so we probably don't want it to be called N times. By lifting it out to an earlier stage, that can help ensure that it only runs once.

ehuss · 2020-12-16T19:00:47Z

src/cargo/core/compiler/build_context/target_info.rs

+/// Structure used to deal with Rustdoc fingerprinting
+#[derive(Debug, Serialize, Deserialize)]
+pub struct RustDocFingerprint {
+    _rustc_verbose_version: String,


The leading _ here seems a little unusual.

I added it because it's never used per se. But ye, might be better to remove it.

ehuss · 2020-12-16T19:03:54Z

src/cargo/core/compiler/build_context/target_info.rs

+        };
+
+        // Check wether `.rustdoc_fingerprint.json exists
+        let mut fingerprint = match File::open(RustDocFingerprint::path_to_fingerprint(doc_dir)) {


I think it's a little more succinct to use std::fs::read_to_string instead of opening a file object. Also, I don't think this needs the extra complexity like from_file. If it is only read in one place, it can just be inlined.

ehuss · 2020-12-16T19:06:02Z

src/cargo/core/compiler/build_context/target_info.rs

+    _rustc_verbose_version: String,
+}
+
+impl RustDocFingerprint {


I'm not sure it's really necessary to have a struct to wrap these methods. From what I can see, this can all be done in a single free function, and it should be substantially smaller.

I did it on that way so that if in the future more things need to be added to it, we already have the struct and it's easier to add functionalities.

I can implement everything directly anyway if you prefer it. I was just thinking about the future, and more functionalities added to this fingerprinting feature for rustdoc.

What should I do then?

I think if it grows in complexity in the future, it can be reworked to be more abstract, but for the foreseeable future I wouldn't expect it to change much.

ehuss · 2020-12-16T19:07:01Z

src/cargo/core/compiler/build_context/target_info.rs

+            // exists neither, we simply do nothing and continue.
+            Err(_) => {
+                // We don't care if this suceeds as explained above.
+                let _ = std::fs::remove_dir_all(doc_dir);


Cargo has specific code for removing paths in util::paths::remove_dir_all.

CPerezz · 2020-12-17T08:37:11Z

rustdoc::rustdoc_target and rustdoc_flags::whitespace tests are failing. But I don't really see where my PR is affecting them. Any ideas @ehuss ?

For the rest, has been addressed. I'm waiting for your inputs on the discussions are still open to modify the code appropriately.

ehuss · 2020-12-18T16:28:06Z

src/cargo/core/compiler/mod.rs

@@ -649,7 +662,11 @@ fn rustdoc(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult<Work> {
                &mut |line| on_stderr_line(state, line, package_id, &target, &mut output_options),
                false,
            )
-            .chain_err(|| format!("could not document `{}`", name))?;
+            .chain_err(|| format!("could not document `{}`.", name))?;


This still has an unrelated change here, which is causing the test failure.

bors · 2020-12-18T18:51:56Z

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

ehuss · 2021-02-13T21:56:57Z

Thanks @CPerezz, I appreciate you sticking with this!

@bors r+

bors · 2021-02-13T21:56:58Z

📌 Commit 61c2332 has been approved by ehuss

bors · 2021-02-13T21:57:07Z

⌛ Testing commit 61c2332 with merge 8fa0827...

bors · 2021-02-13T22:32:31Z

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 8fa0827 to master...

CPerezz · 2021-02-14T12:57:14Z

Thanks @CPerezz, I appreciate you sticking with this!

Thanks to you, I wouldn't have been consistent without your help patience and support. I really appreciate the work you're doing guys from the cargo-team to give mentorship and help and also maintain this repo among others. ❤️

Update cargo 8 commits in ab64d1393b5b77c66b6534ef5023a1b89ee7bf64..bf5a5d5e5d3ae842a63bfce6d070dfd438cf6070 2021-02-10 00:19:10 +0000 to 2021-02-18 15:49:14 +0000 - Propagate `lto=off` harder (rust-lang/cargo#9182) - refactor: make deref intentions more straightforward (rust-lang/cargo#9183) - Update link for no_std attribute. (rust-lang/cargo#9174) - Remove mention of --message-format taking multiple values (rust-lang/cargo#9173) - Emit warning on env variable case mismatch (rust-lang/cargo#9169) - Implement Rustdoc versioning checks (rust-lang/cargo#8640) - Bump to 0.53.0, update changelog (rust-lang/cargo#9168) - Prevent testsuite from loading config out of sandbox. (rust-lang/cargo#9164)

guswynn · 2021-02-24T22:18:22Z

src/cargo/core/compiler/build_context/target_info.rs

+/// Structure used to deal with Rustdoc fingerprinting
+#[derive(Debug, Serialize, Deserialize)]
+pub struct RustDocFingerprint {
+    pub rustc_vv: String,


curious, why are just using the rust version, as opposed to the full fingerprint of the build?

Because the docfiles only depend on the compiler version, if it changes we always rebuild deleting the entire docs folder. Otherways, if the compiler version is the same, we know 100% sure that the docfiles are still valid and will not introduce any errors on the browser console or malfunctions.

We simply don't need anymore data than the version, that's why we don't use the full fingerprint of the build.

What about if people change the feature they are building with, which causes different items to be built? Also is docs only depending on compiler version and no flags something that we guarantee?

If you check the issue from which the PR was originated. The goal is to ensure that the html, css and js files are consistent with the compiler version currently used and not mix it up.

Any other thing stills being handled on the same way it was before. This has nothing to do with the content that you're documenting ie. target, features etc. But with the actual js, css and html code in the files of the docs and which version should they be used on.

Hope that makes it a bit more clear.

@guswynn Each crate has a "fingerprint" stored in the .fingerprint directory. The things it tracks are listed here. The fingerprint will detect if an individual crate needs to be rebuilt, but it doesn't detect when shared files (like the search index) need to be deleted and rebuilt. This adds a "global" fingerprint for building docs to detect when the rust toolchain changes (and may introduce an incompatible search index), and clears out the entire docs directory to deal with those changes. Changes to things like features only trigger the individual crate to be rebuilt, but it retains the shared files (which rustdoc updates incrementally).

1.52 Cargo adds rust-lang/cargo#8640 which means that cargo will try to purge the doc directory caches for us. In theory this may mean that we can jettison the clear_if_dirty for rustdoc versioning entirely, but for now just workaround the effects of this change in a less principled but more local way.

This reverts commit 8fa0827, reversing changes made to b842849. # Conflicts: # src/cargo/core/compiler/build_context/target_info.rs # tests/testsuite/doc.rs

beta: revert #8640 - rustdoc versioning checks #8640 has been causing some problems with rustbuild (see rust-lang/rust#83914 and rust-lang/rust#83530 (comment)). In order to give more time to figure out a solution, this reverts the change on beta. Will need to take some time to figure out what to do on master. Also cherry picked the following to get tests passing: * #9316 — Fix semver docs for 1.51. * #9307 — tests: Tolerate "exit status" in error messages

[beta] Update cargo 2 commits in 90691f2bfe9a50291a98983b1ed2feab51d5ca55..d70308ef052397c7d16f881c2d973a8c5b4c23d9 2021-03-16 21:36:55 +0000 to 2021-04-06 17:32:27 +0000 - beta: revert rust-lang/cargo#8640 - rustdoc versioning checks (rust-lang/cargo#9332) - [beta] Fix publication of packages with metadata and resolver (rust-lang/cargo#9304)

…chton Some changes to rustdoc fingerprint checking. #8640 introduced a check which deletes the `doc` directory if cargo detects it has stale contents from a different toolchain version. Rustdoc has some shared files (js and css for example) that can get corrupted between versions. Unfortunately that caused some problems with rustbuild which does a few unusual things. Rustbuild will: * Create the `doc` directory before running `cargo doc` and places a `.stamp` file inside it. * Creates symlinks of the `doc` directory so that they can be shared across different target directories (in particular, between rustc and rustdoc). In order to address these issues, this PR does several things: * Adds `-Z skip-rustdoc-fingerprint` to disable the `doc` clearing behavior. * Don't delete the `doc` directory if the rustdoc fingerprint is missing. This is intended to help with the scenario where the user creates a `doc` directory ahead of time with pre-existing contents before the first build. The downside is that cargo will not be able to protect against switching from pre-1.53 to post-1.53. * Don't delete the `doc` directory itself (just its contents). This should help if the user created the `doc` directory as a symlink to somewhere else. * Don't delete hidden files in the `doc` directory. This isn't something that rustdoc creates. Only the `-Z` change is needed for rustbuild. The others I figured I'd include just to be on the safe side in case there are other users doing unusual things (and I had already written them thinking they would work for rustbuild). Hopefully the rustbuild `.stamp` mechanism will be enough protection there. Fixes #9336

- Fix clippy warnings - Fix MSRV task to use latest stable to install `cargo-sweep` Before, it would use the lower version to install, which broke when cargo-sweep updated its own MSRV. - Fix tests not to conflict with one another They were using the same target directory, which caused them to fail for some reason. I expect this is related to rust-lang/cargo#8640 somehow but it doesn't seem worth investigating.

CPerezz added 3 commits August 22, 2020 09:12

Implement RustDocTargetData

5035321

This intends to be a helper structure similar to `RustTargetData` which includes helper functions which are useful for interacting with the rustdoc fingerprint files among other things.

Impl remove_doc_directory function

c70c2cc

Sometimes we need to remove the `doc/` directory (if exists) in order to not mix different versions of the js/html/css files that Rustc autogenerates and lack versioning.

rust-highfive assigned alexcrichton Aug 22, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 22, 2020

CPerezz mentioned this pull request Aug 28, 2020

Display builtin aliases with cargo --list #8542

Merged

ehuss assigned ehuss and unassigned alexcrichton Aug 28, 2020

ehuss removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 31, 2020

CPerezz added 2 commits September 20, 2020 09:57

Fail when dealing with io fails

e38c3ac

Is the user does not have permissions or a folder/file can't be created or deleted, Cargo will fail and return an Error indicating the problem.

Add skeleton for testing doc-fingerprint

7a5f784

ehuss added the S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. label Oct 7, 2020

CPerezz and others added 4 commits December 16, 2020 09:21

Remove previous solution

87c1f6c

Merge branch 'master' into doc_versioning

31ef7af

CPerezz marked this pull request as ready for review December 16, 2020 08:39

Apply rustfmt

ebd0d58

ehuss reviewed Dec 16, 2020

View reviewed changes

Address @ehuss suggestions/nits

994ccec

ehuss reviewed Dec 18, 2020

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Feb 13, 2021

bors merged commit 8fa0827 into rust-lang:master Feb 13, 2021

bors mentioned this pull request Feb 13, 2021

Run rustdoc doctests relative to the workspace #9105

Merged

CPerezz deleted the doc_versioning branch February 14, 2021 12:57

ehuss mentioned this pull request Feb 18, 2021

Update cargo rust-lang/rust#82275

Merged

ehuss mentioned this pull request Feb 24, 2021

cargo doc in workspaces includes removed crates' documentation if their previously built documentation is still in target/doc #6783

Open

guswynn reviewed Feb 24, 2021

View reviewed changes

Mark-Simulacrum mentioned this pull request Apr 4, 2021

Bump bootstrap to 1.52 beta rust-lang/rust#83530

Merged

ehuss mentioned this pull request Apr 6, 2021

beta: revert #8640 - rustdoc versioning checks #9332

Merged

ehuss mentioned this pull request Apr 6, 2021

[beta] Update cargo rust-lang/rust#83946

Merged

ehuss mentioned this pull request Apr 8, 2021

Rustdoc fingerprinting issues #9336

Closed

ehuss mentioned this pull request Apr 24, 2021

Some changes to rustdoc fingerprint checking. #9404

Merged

jyn514 mentioned this pull request Aug 30, 2021

Fix CI deadlinks/cargo-deadlinks#150

Merged

ehuss added this to the 1.52.0 milestone Feb 6, 2022


		dummy_project.cargo("doc").run();

		// As the test shows avobe. Now we have generated the `doc/` folder and inside

	// As the test shows avobe. Now we have generated the `doc/` folder and inside
	// As the test shows above. Now we have generated the `doc/` folder and inside

Implement Rustdoc versioning checks #8640

Implement Rustdoc versioning checks #8640

Conversation

CPerezz commented Aug 22, 2020 • edited Loading

rust-highfive commented Aug 22, 2020

ehuss commented Aug 28, 2020 • edited Loading

CPerezz commented Sep 20, 2020

ehuss commented Sep 21, 2020

CPerezz commented Nov 28, 2020

bors commented Dec 7, 2020

CPerezz commented Dec 16, 2020 • edited Loading

EDIT:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CPerezz commented Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

bors commented Dec 18, 2020

ehuss commented Feb 13, 2021

bors commented Feb 13, 2021

bors commented Feb 13, 2021

bors commented Feb 13, 2021

CPerezz commented Feb 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CPerezz commented Aug 22, 2020 •

edited

Loading

ehuss commented Aug 28, 2020 •

edited

Loading

CPerezz commented Dec 16, 2020 •

edited

Loading

CPerezz commented Dec 17, 2020 •

edited

Loading