-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Changes from 11 commits
5035321
c70c2cc
51d3050
e38c3ac
7a5f784
87c1f6c
65f17e1
2fea14e
31ef7af
ebd0d58
994ccec
33a5248
94519f2
27987b6
47e19f0
8ddc56f
d2572a2
7c45021
cb21e64
3c93f6c
1af0027
7bfb3d0
e78f91c
4afa585
61c2332
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,10 +3,12 @@ use crate::core::{Dependency, Target, TargetKind, Workspace}; | |
use crate::util::config::{Config, StringList, TargetConfig}; | ||
use crate::util::{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::fs::File; | ||
use std::path::{Path, PathBuf}; | ||
use std::str::{self, FromStr}; | ||
|
||
/// Information about the platform target gleaned from querying rustc. | ||
|
@@ -754,3 +756,78 @@ 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 { | ||
rustc_verbose_version: String, | ||
} | ||
|
||
impl RustDocFingerprint { | ||
/// Returns the version contained by this `RustDocFingerprint` instance. | ||
fn version(&self) -> &str { | ||
self.rustc_verbose_version.as_str() | ||
} | ||
|
||
/// Given the `doc` dir, returns the path to the rustdoc fingerprint file. | ||
fn path_to_fingerprint(doc_dir: &Path) -> PathBuf { | ||
doc_dir.to_path_buf().join(".rustdoc_fingerprint.json") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we want the fingerprint file in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added it under I'm not sure which effects can have the fact of the fingerprint file living under the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, I would prefer to keep it out of the I think it can detect whether or not it needs to check/create the fingerprint is to check the |
||
} | ||
|
||
/// Write the `RustDocFingerprint` info into the fingerprint file. | ||
pub fn write(&self, doc_dir: &Path) -> std::io::Result<()> { | ||
let rustdoc_fingerprint_file = | ||
File::create(RustDocFingerprint::path_to_fingerprint(doc_dir))?; | ||
// We write the actual `Rustc` version to it so that we just need to compile it straight | ||
// since there's no `doc/` folder to remove. | ||
serde_json::to_writer(&rustdoc_fingerprint_file, &self) | ||
.map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, format!("{:?}", e))) | ||
CPerezz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
/// 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, returning `(Self, bool)` where the `bool` says if the `doc` folder was removed. | ||
/// | ||
/// In case it's not, it takes care of removig the `doc/` folder as well as overwriting | ||
ehuss marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// the rustdoc fingerprint info in order to guarantee that we won't end up with mixed | ||
/// versions of the `js/html/css` files that `Rustc` autogenerates which do not have | ||
ehuss marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// any versioning. | ||
pub fn check_rustdoc_fingerprint( | ||
doc_dir: &Path, | ||
rustc_verbose_ver: &str, | ||
) -> anyhow::Result<(Self, bool)> { | ||
let actual_rustdoc_target_data = RustDocFingerprint { | ||
rustc_verbose_version: rustc_verbose_ver.to_string(), | ||
}; | ||
|
||
// Check wether `.rustdoc_fingerprint.json exists | ||
match std::fs::read_to_string(Self::path_to_fingerprint(doc_dir)) { | ||
CPerezz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Ok(rustdoc_data) => { | ||
let rustdoc_fingerprint: Self = match serde_json::from_str(&rustdoc_data) { | ||
Ok(res) => res, | ||
Err(_) => { | ||
crate::util::paths::remove_dir_all(doc_dir)?; | ||
return Ok((actual_rustdoc_target_data, true)); | ||
} | ||
}; | ||
// Check if rustc_version matches the one we just used. Otherways, | ||
// remove the `doc` folder to trigger a re-compilation of the docs. | ||
if rustdoc_fingerprint.version() != actual_rustdoc_target_data.version() { | ||
crate::util::paths::remove_dir_all(doc_dir)?; | ||
Ok((actual_rustdoc_target_data, true)) | ||
} else { | ||
Ok((actual_rustdoc_target_data, false)) | ||
} | ||
} | ||
// If the file does not exist, then we cannot assume that the docs were compiled | ||
// with the actual Rustc instace version. Therefore, we try to remove the | ||
ehuss marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// `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 suceeds as explained above. | ||
ehuss marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let _ = crate::util::paths::remove_dir_all(doc_dir); | ||
Ok((actual_rustdoc_target_data, true)) | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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}; | ||
pub use self::compile_kind::{CompileKind, CompileTarget}; | ||
|
@@ -589,15 +591,26 @@ 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); | ||
// 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 | ||
// compiler versions of these files. | ||
let (fingerprint, doc_dir_removed) = RustDocFingerprint::check_rustdoc_fingerprint( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This probably isn't a safe place to put this, since we don't want building the 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, it's important to highlight that inside of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
&doc_dir, | ||
cx.bcx.rustc().verbose_version.as_str(), | ||
)?; | ||
|
||
// Create the documentation directory ahead of time as rustdoc currently has | ||
// a bug where concurrent invocations will race to create this directory if | ||
// it doesn't already exist. | ||
paths::create_dir_all(&doc_dir)?; | ||
|
||
rustdoc.arg("-o").arg(doc_dir); | ||
rustdoc.arg("-o").arg(doc_dir.clone()); | ||
|
||
for feat in &unit.features { | ||
rustdoc.arg("--cfg").arg(&format!("feature=\"{}\"", feat)); | ||
|
@@ -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))?; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This still has an unrelated change here, which is causing the test failure. |
||
|
||
if doc_dir_removed { | ||
fingerprint.write(&doc_dir)? | ||
}; | ||
Ok(()) | ||
})) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ use cargo_test_support::paths::CargoPathExt; | |
use cargo_test_support::registry::Package; | ||
use cargo_test_support::{basic_lib_manifest, basic_manifest, git, project}; | ||
use cargo_test_support::{is_nightly, rustc_host}; | ||
use serde::{Deserialize, Serialize}; | ||
use std::fs; | ||
use std::str; | ||
|
||
|
@@ -1606,6 +1607,11 @@ fn crate_versions() { | |
|
||
#[cargo_test] | ||
fn crate_versions_flag_is_overridden() { | ||
#[derive(Debug, Serialize, Deserialize)] | ||
pub struct RustDocFingerprint { | ||
rustc_verbose_version: String, | ||
} | ||
|
||
let p = project() | ||
.file( | ||
"Cargo.toml", | ||
|
@@ -1639,6 +1645,148 @@ fn crate_versions_flag_is_overridden() { | |
asserts(output_documentation()); | ||
} | ||
|
||
#[cargo_test] | ||
fn doc_fingerprint_versioning_consistent() { | ||
#[derive(Debug, Serialize, Deserialize)] | ||
pub struct RustDocFingerprint { | ||
pub rustc_verbose_version: String, | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can probably just use the definition from the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you expand a bit on this. I don't understand what you mean. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of redefining the struct, you should be able to just reference |
||
|
||
// Test that using different Rustc versions forces a | ||
// doc re-compilation producing new html, css & js files. | ||
|
||
// Random rustc verbose version | ||
let stable_rustc_verbose_version = format!( | ||
CPerezz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"\ | ||
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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would suggest adding a dummy file to the doc directory, just to ensure the entire thing gets deleted. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
|
||
let fingerprint: RustDocFingerprint = serde_json::from_str( | ||
dummy_project | ||
.read_file( | ||
std::path::PathBuf::from("target") | ||
.join("doc") | ||
.join(".rustdoc_fingerprint.json") | ||
.to_str() | ||
.expect("Malformed path"), | ||
) | ||
.as_str(), | ||
) | ||
.expect("JSON Serde fail"); | ||
CPerezz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// 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") | ||
.stdout(std::process::Stdio::piped()) | ||
CPerezz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
.output() | ||
.expect("Failed to get actual rustc verbose version"); | ||
assert_eq!( | ||
fingerprint.rustc_verbose_version, | ||
(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. | ||
fs::remove_file( | ||
dummy_project | ||
.root() | ||
.join("target") | ||
.join("doc") | ||
.join(".rustdoc_fingerprint.json"), | ||
) | ||
.expect("Failed to read fingerprint on tests"); | ||
CPerezz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
fs::write( | ||
dummy_project | ||
.root() | ||
.join("target") | ||
.join("doc") | ||
.join(".rustdoc_fingerprint.json"), | ||
stable_rustc_verbose_version, | ||
) | ||
.expect("Error writing old rustc version"); | ||
CPerezz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
fs::write( | ||
CPerezz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
dummy_project | ||
.root() | ||
.join("target") | ||
.join("doc") | ||
.join("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.change_file("src/lib.rs", "//! These are the docs 2!"); | ||
CPerezz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
dummy_project.cargo("doc").run(); | ||
|
||
assert!(fs::File::open( | ||
dummy_project | ||
.root() | ||
.join("target") | ||
.join("doc") | ||
.join("bogus_file") | ||
) | ||
.is_err()); | ||
CPerezz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
let fingerprint: RustDocFingerprint = serde_json::from_str( | ||
dummy_project | ||
.read_file( | ||
std::path::PathBuf::from("target") | ||
.join("doc") | ||
.join(".rustdoc_fingerprint.json") | ||
.to_str() | ||
.expect("Malformed path"), | ||
) | ||
.as_str(), | ||
) | ||
.expect("JSON Serde fail"); | ||
CPerezz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// 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") | ||
.stdout(std::process::Stdio::piped()) | ||
.output() | ||
.expect("Failed to get actual rustc verbose version"); | ||
|
||
assert_eq!( | ||
fingerprint.rustc_verbose_version, | ||
(String::from_utf8_lossy(&output.stdout).as_ref()) | ||
); | ||
} | ||
|
||
#[cargo_test] | ||
fn doc_test_in_workspace() { | ||
let p = project() | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.