Address latest reivew suggestions

- Instead of `fs` we use the `utils::paths` functions
to interact with the filesystem.
- The doc fingerprint is now stored under `target/` instead
of `target/doc/`.
- The code in `compile` has been reduced to a single function call.

Author: CPerezz

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

@@ -1,14 +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::fs::File;
-use std::path::{Path, PathBuf};
+use std::path::PathBuf;
 use std::str::{self, FromStr};
 
 /// Information about the platform target gleaned from querying rustc.
@@ -754,65 +755,53 @@ impl RustcTargetData {
 /// Structure used to deal with Rustdoc fingerprinting
 #[derive(Debug, Serialize, Deserialize)]
 pub struct RustDocFingerprint {
-    rustc_verbose_version: String,
+    rustc_vv: 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")
+    /// 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")
+                .with_extension("json"),
+        )?;
+        serde_json::from_str(&rustdoc_data).map_err(|e| anyhow::anyhow!("{:?}", e))
     }
 
     /// Write the `RustDocFingerprint` info into the fingerprint file.
-    pub fn write(&self, doc_dir: &Path) -> std::io::Result<()> {
-        crate::util::paths::create_dir_all(doc_dir)
-            .map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, format!("{:?}", e)))?;
-        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)))
+    fn write<'a, 'cfg>(&self, cx: &Context<'a, 'cfg>) -> CargoResult<()> {
+        paths::write(
+            &cx.files()
+                .host_root()
+                .join(".rustdoc_fingerprint.json")
+                .with_extension("json"),
+            serde_json::to_string(&self)?.as_bytes(),
+        )
     }
 
     /// 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.
+    /// call.
     ///
     /// In case it's not, it takes care of removig 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 `Rustc` autogenerates which do not have
     /// any versioning.
-    pub fn check_rustdoc_fingerprint(
-        doc_dir: &Path,
-        rustc_verbose_ver: &str,
-    ) -> anyhow::Result<(Self, bool)> {
+    pub fn check_rustdoc_fingerprint<'a, 'cfg>(cx: &Context<'a, 'cfg>) -> CargoResult<()> {
         let actual_rustdoc_target_data = RustDocFingerprint {
-            rustc_verbose_version: rustc_verbose_ver.to_string(),
+            rustc_vv: cx.bcx.rustc().verbose_version.clone(),
         };
-
-        // Check wether `.rustdoc_fingerprint.json exists
-        match std::fs::read_to_string(Self::path_to_fingerprint(doc_dir)) {
-            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));
-                    }
-                };
+        let doc_dir = cx.files().host_root().join("doc");
+        // 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 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 fingerprint.rustc_vv != actual_rustdoc_target_data.rustc_vv {
+                    paths::remove_dir_all(&doc_dir)?;
+                    actual_rustdoc_target_data.write(cx)?
                 }
             }
             // If the file does not exist, then we cannot assume that the docs were compiled
@@ -821,9 +810,10 @@ impl RustDocFingerprint {
             // exists neither, we simply do nothing and continue.
             Err(_) => {
                 // We don't care if this suceeds as explained above.
-                let _ = crate::util::paths::remove_dir_all(doc_dir);
-                Ok((actual_rustdoc_target_data, true))
+                let _ = paths::remove_dir_all(doc_dir);
+                actual_rustdoc_target_data.write(cx)?
             }
         }
+        Ok(())
     }
 }

src/cargo/core/compiler/context/mod.rs

@@ -140,15 +140,10 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
         // 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 doc_dir = self.files().host_root().join("doc");
-        let (fingerprint, doc_dir_removed) = RustDocFingerprint::check_rustdoc_fingerprint(
-            &doc_dir,
-            self.bcx.rustc().verbose_version.as_str(),
-        )?;
-        if doc_dir_removed {
-            fingerprint.write(&doc_dir)?
-        };
+        // 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

tests/testsuite/clean.rs

@@ -432,7 +432,7 @@ fn assert_all_clean(build_dir: &Path) {
     }) {
         let entry = entry.unwrap();
         let path = entry.path();
-        if let ".rustc_info.json" | ".cargo-lock" | "CACHEDIR.TAG" | ".rustdoc_fingerprint.json" =
+        if let ".rustc_info.json" | ".cargo-lock" | "CACHEDIR.TAG" =
             path.file_name().unwrap().to_str().unwrap()
         {
             continue;

tests/testsuite/doc.rs

@@ -1644,14 +1644,14 @@ fn crate_versions_flag_is_overridden() {
 fn doc_fingerprint_versioning_consistent() {
     #[derive(Debug, Serialize, Deserialize)]
     pub struct RustDocFingerprint {
-        pub rustc_verbose_version: String,
+        rustc_vv: String,
     }
 
     // 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!(
+    let old_rustc_verbose_version = format!(
         "\
 rustc 1.41.1 (f3e1a954d 2020-02-24)
 binary: rustc
@@ -1680,28 +1680,18 @@ LLVM version: 9.0
 
     dummy_project.cargo("doc").run();
 
-    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");
+    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")
-        .stdout(std::process::Stdio::piped())
         .output()
         .expect("Failed to get actual rustc verbose version");
     assert_eq!(
-        fingerprint.rustc_verbose_version,
+        fingerprint.rustc_vv,
         (String::from_utf8_lossy(&output.stdout).as_ref())
     );
 
@@ -1710,74 +1700,33 @@ LLVM version: 9.0
     // 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");
-
-    fs::write(
-        dummy_project
-            .root()
-            .join("target")
-            .join("doc")
-            .join(".rustdoc_fingerprint.json"),
-        stable_rustc_verbose_version,
-    )
-    .expect("Error writing old rustc version");
+    dummy_project.change_file(
+        "target/.rustdoc_fingerprint.json",
+        &old_rustc_verbose_version,
+    );
 
     fs::write(
-        dummy_project
-            .root()
-            .join("target")
-            .join("doc")
-            .join("bogus_file"),
+        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.change_file("src/lib.rs", "//! These are the docs 2!");
+    // It should also remove the bogus file we created above.
     dummy_project.cargo("doc").run();
 
-    assert!(fs::File::open(
-        dummy_project
-            .root()
-            .join("target")
-            .join("doc")
-            .join("bogus_file")
-    )
-    .is_err());
-
-    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");
+    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.
-    let output = std::process::Command::new("rustc")
-        .arg("-vV")
-        .stdout(std::process::Stdio::piped())
-        .output()
-        .expect("Failed to get actual rustc verbose version");
-
     assert_eq!(
-        fingerprint.rustc_verbose_version,
+        fingerprint.rustc_vv,
         (String::from_utf8_lossy(&output.stdout).as_ref())
     );
 }