Add test / documentation for scrape-examples cycle-avoidance, add map for doc unit -> metadata to Context

Author: willcrichton

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

@@ -81,6 +81,10 @@ pub struct Context<'a, 'cfg> {
     /// compilation is happening (only object, only bitcode, both, etc), and is
     /// precalculated early on.
     pub lto: HashMap<Unit, Lto>,
+
+    /// Map of Doc/Docscrape units to metadata for their -Cmetadata flag.
+    /// See Context::find_metadata_units for more details.
+    pub metadata_for_doc_units: HashMap<Unit, Metadata>,
 }
 
 impl<'a, 'cfg> Context<'a, 'cfg> {
@@ -121,6 +125,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
             rustc_clients: HashMap::new(),
             pipelining,
             lto: HashMap::new(),
+            metadata_for_doc_units: HashMap::new(),
         })
     }
 
@@ -135,6 +140,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
         self.prepare()?;
         custom_build::build_map(&mut self)?;
         self.check_collisions()?;
+        self.compute_metadata_for_doc_units();
 
         // 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
@@ -614,4 +620,40 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
 
         Ok(client)
     }
+
+    /// Finds metadata for Doc/Docscrape units.
+    ///
+    /// rustdoc needs a -Cmetadata flag in order to recognize StableCrateIds that refer to
+    /// items in the crate being documented. The -Cmetadata flag used by reverse-dependencies
+    /// will be the metadata of the Cargo unit that generated the current library's rmeta file,
+    /// which should be a Check unit.
+    ///
+    /// If the current crate has reverse-dependencies, such a Check unit should exist, and so
+    /// we use that crate's metadata. If not, we use the crate's Doc unit so at least examples
+    /// scraped from the current crate can be used when documenting the current crate.
+    pub fn compute_metadata_for_doc_units(&mut self) {
+        for unit in self.bcx.unit_graph.keys() {
+            if !unit.mode.is_doc() && !unit.mode.is_doc_scrape() {
+                continue;
+            }
+
+            let matching_units = self
+                .bcx
+                .unit_graph
+                .keys()
+                .filter(|other| {
+                    unit.pkg == other.pkg
+                        && unit.target == other.target
+                        && !other.mode.is_doc_scrape()
+                })
+                .collect::<Vec<_>>();
+            let metadata_unit = matching_units
+                .iter()
+                .find(|other| other.mode.is_check())
+                .or_else(|| matching_units.iter().find(|other| other.mode.is_doc()))
+                .unwrap_or(&unit);
+            self.metadata_for_doc_units
+                .insert(unit.clone(), self.files().metadata(metadata_unit));
+        }
+    }
 }

src/cargo/core/compiler/mod.rs

@@ -648,28 +648,7 @@ fn rustdoc(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult<Work> {
         rustdoc.args(args);
     }
 
-    // rustdoc needs a -Cmetadata flag in order to recognize StableCrateIds that refer to
-    // items in the crate being documented. The -Cmetadata flag used by reverse-dependencies
-    // will be the metadata of the Cargo unit that generated the current library's rmeta file,
-    // which should be a Check unit.
-    //
-    // If the current crate has reverse-dependencies, such a Check unit should exist, and so
-    // we use that crate's metadata. If not, we use the crate's Doc unit so at least examples
-    // scraped from the current crate can be used when documenting the current crate.
-    let matching_units = cx
-        .bcx
-        .unit_graph
-        .keys()
-        .filter(|other| {
-            unit.pkg == other.pkg && unit.target == other.target && !other.mode.is_doc_scrape()
-        })
-        .collect::<Vec<_>>();
-    let metadata_unit = matching_units
-        .iter()
-        .find(|other| other.mode.is_check())
-        .or_else(|| matching_units.iter().find(|other| other.mode.is_doc()))
-        .unwrap_or(&unit);
-    let metadata = cx.files().metadata(metadata_unit);
+    let metadata = cx.metadata_for_doc_units[&unit];
     rustdoc.arg("-C").arg(format!("metadata={}", metadata));
 
     let scrape_output_path = |unit: &Unit| -> CargoResult<PathBuf> {

src/cargo/core/compiler/unit_dependencies.rs

@@ -714,9 +714,14 @@ fn connect_run_custom_build_deps(state: &mut State<'_, '_>) {
                         && other.unit.target.is_linkable()
                         && other.unit.pkg.manifest().links().is_some()
                 })
-                // Avoid cycles when using the --scrape-examples feature
-                // FIXME(wcrichto): unclear why this exact filter is the fix
-                .filter(|(_, other)| !other.unit.mode.is_doc_scrape())
+                // Avoid cycles when using the doc --scrape-examples feature:
+                // Say a workspace has crates A and B where A has a build-dependency on B.
+                // The Doc units for A and B will have a dependency on the Docscrape for both A and B.
+                // So this would add a dependency from B-build to A-build, causing a cycle:
+                //   B (build) -> A (build) -> B(build)
+                // See the test scrape_examples_avoid_build_script_cycle for a concrete example.
+                // To avoid this cycle, we filter out the B -> A (docscrape) dependency.
+                .filter(|(_parent, other)| !other.unit.mode.is_doc_scrape())
                 // Skip dependencies induced via dev-dependencies since
                 // connections between `links` and build scripts only happens
                 // via normal dependencies. Otherwise since dev-dependencies can

tests/testsuite/doc.rs

@@ -2150,7 +2150,7 @@ fn doc_fingerprint_unusual_behavior() {
 }
 
 #[cargo_test]
-fn scrape_examples() {
+fn scrape_examples_basic() {
     if !is_nightly() {
         // --scrape-examples is unstable
         return;
@@ -2185,3 +2185,50 @@ fn scrape_examples() {
     assert!(doc_html.contains("Examples found in repository"));
     assert!(doc_html.contains("More examples"));
 }
+
+#[cargo_test]
+fn scrape_examples_avoid_build_script_cycle() {
+    if !is_nightly() {
+        // --scrape-examples is unstable
+        return;
+    }
+
+    let p = project()
+        // package with build dependency
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "0.0.1"
+                authors = []
+                links = "foo"
+
+                [workspace]
+                members = ["bar"]
+
+                [build-dependencies]
+                bar = {path = "bar"}
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .file("build.rs", "fn main(){}")
+        // dependency
+        .file(
+            "bar/Cargo.toml",
+            r#"
+                [package]
+                name = "bar"
+                version = "0.0.1"
+                authors = []
+                links = "bar"
+            "#,
+        )
+        .file("bar/src/lib.rs", "")
+        .file("bar/build.rs", "fn main(){}")
+        .build();
+
+    p.cargo("doc --all -Zunstable-options --scrape-examples all")
+        .masquerade_as_nightly_cargo()
+        .run();
+}