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

Implement Rustdoc versioning checks #8640

Merged
merged 25 commits into from
Feb 13, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
5035321
Implement `RustDocTargetData`
CPerezz Aug 22, 2020
c70c2cc
Impl remove_doc_directory function
CPerezz Aug 22, 2020
51d3050
Implement Rustdoc versioning checks
CPerezz Aug 22, 2020
e38c3ac
Fail when dealing with io fails
CPerezz Sep 20, 2020
7a5f784
Add skeleton for testing doc-fingerprint
CPerezz Sep 20, 2020
87c1f6c
Remove previous solution
CPerezz Dec 16, 2020
65f17e1
Implement rustdoc versioning
CPerezz Dec 16, 2020
2fea14e
Add tests for rustdoc fingerprint impl
CPerezz Dec 16, 2020
31ef7af
Merge branch 'master' into doc_versioning
CPerezz Dec 16, 2020
ebd0d58
Apply rustfmt
CPerezz Dec 16, 2020
994ccec
Address @ehuss suggestions/nits
CPerezz Dec 17, 2020
33a5248
Move rustdoc fingerprint checks earlier
CPerezz Jan 25, 2021
94519f2
Create dirs if needed before f_p write call
CPerezz Jan 25, 2021
27987b6
Add rustdoc fingerprint exception for clean
CPerezz Jan 25, 2021
47e19f0
Merge branch 'master' into doc_versioning
CPerezz Jan 25, 2021
8ddc56f
Export `RustDocFingerprint` from `core/compiler`
CPerezz Jan 26, 2021
d2572a2
Address latest reivew suggestions
CPerezz Jan 27, 2021
7c45021
Check target-dependant doc folders
CPerezz Jan 31, 2021
cb21e64
Improve & fix doc dir removal process
CPerezz Feb 1, 2021
3c93f6c
Merge branch 'master' into doc_versioning
CPerezz Feb 3, 2021
1af0027
Fix upstream changes of #9122
CPerezz Feb 3, 2021
7bfb3d0
Fix spelling
CPerezz Feb 3, 2021
e78f91c
Refactor doc_dirs obtention
CPerezz Feb 3, 2021
4afa585
Improve RustDocFingerprint::remove_doc_dirs
CPerezz Feb 4, 2021
61c2332
Update src/cargo/core/compiler/build_context/target_info.rs
CPerezz Feb 13, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/cargo/core/compiler/build_context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ use std::collections::{HashMap, HashSet};
use std::path::PathBuf;

mod target_info;
pub use self::target_info::{FileFlavor, FileType, RustcTargetData, TargetInfo};
pub use self::target_info::{
FileFlavor, FileType, RustDocFingerprint, RustcTargetData, TargetInfo,
};

/// The build context, containing all information about a build task.
///
Expand Down
83 changes: 80 additions & 3 deletions src/cargo/core/compiler/build_context/target_info.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
use crate::core::compiler::{BuildOutput, CompileKind, CompileMode, CompileTarget, CrateType};
use crate::core::compiler::{
BuildOutput, CompileKind, CompileMode, CompileTarget, Context, CrateType,
};
use crate::core::{Dependency, Target, TargetKind, Workspace};
use crate::util::config::{Config, StringList, TargetConfig};
use crate::util::{CargoResult, CargoResultExt, ProcessBuilder, Rustc};
use crate::util::{paths, CargoResult, CargoResultExt, ProcessBuilder, Rustc};
use cargo_platform::{Cfg, CfgExpr};
use serde::{Deserialize, Serialize};
use std::cell::RefCell;
use std::collections::hash_map::{Entry, HashMap};
use std::env;
use std::path::PathBuf;
use std::path::{Path, PathBuf};
use std::str::{self, FromStr};

/// Information about the platform target gleaned from querying rustc.
Expand Down Expand Up @@ -748,3 +751,77 @@ impl RustcTargetData {
self.target_config(kind).links_overrides.get(lib_name)
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

}

impl RustDocFingerprint {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/// Read the `RustDocFingerprint` info from the fingerprint file.
fn read<'a, 'cfg>(cx: &Context<'a, 'cfg>) -> CargoResult<Self> {
let rustdoc_data = paths::read(&cx.files().host_root().join(".rustdoc_fingerprint.json"))?;
serde_json::from_str(&rustdoc_data).map_err(|e| anyhow::anyhow!("{:?}", e))
}

/// Write the `RustDocFingerprint` info into the fingerprint file.
fn write<'a, 'cfg>(&self, cx: &Context<'a, 'cfg>) -> CargoResult<()> {
paths::write(
&cx.files().host_root().join(".rustdoc_fingerprint.json"),
serde_json::to_string(&self)?.as_bytes(),
)
}

fn remove_doc_dirs(doc_dirs: &Vec<&Path>) -> CargoResult<()> {
doc_dirs
.iter()
.filter(|path| path.exists())
.map(|path| paths::remove_dir_all(&path))
.collect::<CargoResult<()>>()
}

/// This function checks whether the latest version of `Rustc` used to compile this
/// `Workspace`'s docs was the same as the one is currently being used in this `cargo doc`
/// call.
///
/// In case it's not, it takes care of removing the `doc/` folder as well as overwriting
/// the rustdoc fingerprint info in order to guarantee that we won't end up with mixed
/// versions of the `js/html/css` files that `rustdoc` autogenerates which do not have
/// any versioning.
pub fn check_rustdoc_fingerprint<'a, 'cfg>(cx: &Context<'a, 'cfg>) -> CargoResult<()> {
let actual_rustdoc_target_data = RustDocFingerprint {
rustc_vv: cx.bcx.rustc().verbose_version.clone(),
};

// Collect all of the target doc paths for which the docs need to be compiled for.
let doc_dirs: Vec<&Path> = cx
.bcx
.all_kinds
.iter()
.map(|kind| cx.files().layout(*kind).doc())
.collect();

// Check wether `.rustdoc_fingerprint.json` exists
match Self::read(cx) {
Ok(fingerprint) => {
// Check if rustc_version matches the one we just used. Otherways,
// remove the `doc` folder to trigger a re-compilation of the docs.
if fingerprint.rustc_vv != actual_rustdoc_target_data.rustc_vv {
Self::remove_doc_dirs(&doc_dirs)?;
actual_rustdoc_target_data.write(cx)?
}
}
// If the file does not exist, then we cannot assume that the docs were compiled
// with the actual Rustc instance version. Therefore, we try to remove the
// `doc` directory forcing the recompilation of the docs. If the directory doesn't
// exists neither, we simply do nothing and continue.
Err(_) => {
// We don't care if this succeeds as explained above.
let _ = Self::remove_doc_dirs(&doc_dirs);
actual_rustdoc_target_data.write(cx)?
}
}
Ok(())
}
}
16 changes: 15 additions & 1 deletion src/cargo/core/compiler/context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ use super::job_queue::JobQueue;
use super::layout::Layout;
use super::lto::Lto;
use super::unit_graph::UnitDep;
use super::{BuildContext, Compilation, CompileKind, CompileMode, Executor, FileFlavor};
use super::{
BuildContext, Compilation, CompileKind, CompileMode, Executor, FileFlavor, RustDocFingerprint,
};

mod compilation_files;
use self::compilation_files::CompilationFiles;
Expand Down Expand Up @@ -133,6 +135,18 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
custom_build::build_map(&mut self)?;
self.check_collisions()?;

// 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 forcing a rebuild.
//
// This is important because the `.js`/`.html` & `.css` files that are generated by Rustc don't have
// 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
// versions of these files.
if self.bcx.build_config.mode.is_doc() {
RustDocFingerprint::check_rustdoc_fingerprint(&self)?
}

for unit in &self.bcx.roots {
// Build up a list of pending jobs, each of which represent
// compiling a particular package. No actual work is executed as
Expand Down
5 changes: 3 additions & 2 deletions src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ use lazycell::LazyCell;
use log::debug;

pub use self::build_config::{BuildConfig, CompileMode, MessageFormat};
pub use self::build_context::{BuildContext, FileFlavor, FileType, RustcTargetData, TargetInfo};
pub use self::build_context::{
BuildContext, FileFlavor, FileType, RustDocFingerprint, RustcTargetData, TargetInfo,
};
use self::build_plan::BuildPlan;
pub use self::compilation::{Compilation, Doctest, UnitOutput};
pub use self::compile_kind::{CompileKind, CompileTarget};
Expand Down Expand Up @@ -586,7 +588,6 @@ fn rustdoc(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult<Work> {
if let CompileKind::Target(target) = unit.kind {
rustdoc.arg("--target").arg(target.rustc_target());
}

let doc_dir = cx.files().out_dir(unit);

// Create the documentation directory ahead of time as rustdoc currently has
Expand Down
177 changes: 177 additions & 0 deletions tests/testsuite/doc.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//! Tests for the `cargo doc` command.

use cargo::core::compiler::RustDocFingerprint;
use cargo_test_support::paths::CargoPathExt;
use cargo_test_support::registry::Package;
use cargo_test_support::{basic_lib_manifest, basic_manifest, git, project};
Expand Down Expand Up @@ -1638,3 +1639,179 @@ fn crate_versions_flag_is_overridden() {
p.cargo("rustdoc -- --crate-version 2.0.3").run();
asserts(output_documentation());
}

#[cargo_test]
fn doc_fingerprint_is_versioning_consistent() {
// Random rustc verbose version
let old_rustc_verbose_version = format!(
"\
rustc 1.41.1 (f3e1a954d 2020-02-24)
binary: rustc
commit-hash: f3e1a954d2ead4e2fc197c7da7d71e6c61bad196
commit-date: 2020-02-24
host: {}
release: 1.41.1
LLVM version: 9.0
",
rustc_host()
);

// Create the dummy project.
let dummy_project = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "1.2.4"
authors = []
"#,
)
.file("src/lib.rs", "//! These are the docs!")
.build();

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

let fingerprint: RustDocFingerprint =
serde_json::from_str(&dummy_project.read_file("target/.rustdoc_fingerprint.json"))
.expect("JSON Serde fail");

// Check that the fingerprint contains the actual rustc version
// which has been used to compile the docs.
let output = std::process::Command::new("rustc")
.arg("-vV")
.output()
.expect("Failed to get actual rustc verbose version");
assert_eq!(
fingerprint.rustc_vv,
(String::from_utf8_lossy(&output.stdout).as_ref())
);

// As the test shows above. Now we have generated the `doc/` folder and inside
// the rustdoc fingerprint file is located with the correct rustc version.
// So we will remove it and create a new fingerprint with an old rustc version
// inside it. We will also place a bogus file inside of the `doc/` folder to ensure
// it gets removed as we expect on the next doc compilation.
dummy_project.change_file(
"target/.rustdoc_fingerprint.json",
&old_rustc_verbose_version,
);

fs::write(
CPerezz marked this conversation as resolved.
Show resolved Hide resolved
dummy_project.build_dir().join("doc/bogus_file"),
String::from("This is a bogus file and should be removed!"),
)
.expect("Error writing test bogus file");

// Now if we trigger another compilation, since the fingerprint contains an old version
// of rustc, cargo should remove the entire `/doc` folder (including the fingerprint)
// and generating another one with the actual version.
// It should also remove the bogus file we created above.
dummy_project.cargo("doc").run();

assert!(!dummy_project.build_dir().join("doc/bogus_file").exists());

let fingerprint: RustDocFingerprint =
serde_json::from_str(&dummy_project.read_file("target/.rustdoc_fingerprint.json"))
.expect("JSON Serde fail");

// Check that the fingerprint contains the actual rustc version
// which has been used to compile the docs.
assert_eq!(
fingerprint.rustc_vv,
(String::from_utf8_lossy(&output.stdout).as_ref())
);
}

#[cfg(target_os = "linux")]
#[cargo_test]
fn doc_fingerprint_respects_target_paths() {
// Random rustc verbose version
let old_rustc_verbose_version = format!(
"\
rustc 1.41.1 (f3e1a954d 2020-02-24)
binary: rustc
commit-hash: f3e1a954d2ead4e2fc197c7da7d71e6c61bad196
commit-date: 2020-02-24
host: {}
release: 1.41.1
LLVM version: 9.0
",
rustc_host()
);

// Create the dummy project.
let dummy_project = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "1.2.4"
authors = []
"#,
)
.file("src/lib.rs", "//! These are the docs!")
.build();

dummy_project
.cargo("doc --target x86_64-unknown-linux-gnu")
.run();

let fingerprint: RustDocFingerprint =
serde_json::from_str(&dummy_project.read_file("target/.rustdoc_fingerprint.json"))
.expect("JSON Serde fail");

// Check that the fingerprint contains the actual rustc version
// which has been used to compile the docs.
let output = std::process::Command::new("rustc")
CPerezz marked this conversation as resolved.
Show resolved Hide resolved
.arg("-vV")
.output()
.expect("Failed to get actual rustc verbose version");
assert_eq!(
fingerprint.rustc_vv,
(String::from_utf8_lossy(&output.stdout).as_ref())
);

// As the test shows above. Now we have generated the `doc/` folder and inside
// the rustdoc fingerprint file is located with the correct rustc version.
// So we will remove it and create a new fingerprint with an old rustc version
// inside it. We will also place a bogus file inside of the `doc/` folder to ensure
// it gets removed as we expect on the next doc compilation.
dummy_project.change_file(
"target/.rustdoc_fingerprint.json",
&old_rustc_verbose_version,
);

fs::write(
dummy_project
.build_dir()
.join("x86_64-unknown-linux-gnu/doc/bogus_file"),
String::from("This is a bogus file and should be removed!"),
)
.expect("Error writing test bogus file");

// Now if we trigger another compilation, since the fingerprint contains an old version
// of rustc, cargo should remove the entire `/doc` folder (including the fingerprint)
// and generating another one with the actual version.
// It should also remove the bogus file we created above.
dummy_project
.cargo("doc --target x86_64-unknown-linux-gnu")
.run();

assert!(!dummy_project
.build_dir()
.join("x86_64-unknown-linux-gnu/doc/bogus_file")
.exists());

let fingerprint: RustDocFingerprint =
serde_json::from_str(&dummy_project.read_file("target/.rustdoc_fingerprint.json"))
.expect("JSON Serde fail");

// Check that the fingerprint contains the actual rustc version
// which has been used to compile the docs.
assert_eq!(
fingerprint.rustc_vv,
(String::from_utf8_lossy(&output.stdout).as_ref())
);
}