Error on absolute src paths

These lead to non-deterministic lockfile contents because they depend on
the depth of the directory the repo is checked out into.

This may possibly introduce false positives because of hard-coded
/dev/null or similar in actual published crates - hopefully it doesn't
(ideally people aren't publishing this kind of thing to crates.io), but
if it does, we can loosen those to a warning or add a per-crate opt-out
or something.

Author: illicitonion

crate_universe/src/context.rs

@@ -52,7 +52,7 @@ impl Context {
         Ok(serde_json::from_str(&data)?)
     }
 
-    pub(crate) fn new(annotations: Annotations, sources_are_present: bool) -> Result<Self> {
+    pub(crate) fn new(annotations: Annotations, sources_are_present: bool) -> anyhow::Result<Self> {
         // Build a map of crate contexts
         let crates: BTreeMap<CrateId, CrateContext> = annotations
             .metadata
@@ -68,11 +68,11 @@ impl Context {
                     annotations.config.generate_binaries,
                     annotations.config.generate_build_scripts,
                     sources_are_present,
-                );
+                )?;
                 let id = CrateId::new(context.name.clone(), context.version.clone());
-                (id, context)
+                Ok::<_, anyhow::Error>((id, context))
             })
-            .collect();
+            .collect::<Result<_, _>>()?;
 
         // Filter for any crate that contains a binary
         let binary_crates: BTreeSet<CrateId> = crates

crate_universe/src/context/crate_context.rs

@@ -352,7 +352,7 @@ impl CrateContext {
         include_binaries: bool,
         include_build_scripts: bool,
         sources_are_present: bool,
-    ) -> Self {
+    ) -> anyhow::Result<Self> {
         let package: &Package = &packages[&annotation.node.id];
         let current_crate_id = CrateId::new(package.name.clone(), package.version.clone());
 
@@ -430,7 +430,7 @@ impl CrateContext {
             gen_binaries,
             include_build_scripts,
             sources_are_present,
-        );
+        )?;
 
         // Parse the library crate name from the set of included targets
         let library_target_name = {
@@ -511,7 +511,7 @@ impl CrateContext {
         };
 
         // Create the crate's context and apply extra settings
-        CrateContext {
+        Ok(CrateContext {
             name: package.name.clone(),
             version: package.version.clone(),
             license: package.license.clone(),
@@ -529,7 +529,7 @@ impl CrateContext {
             alias_rule: None,
             override_targets: BTreeMap::new(),
         }
-        .with_overrides(extras)
+        .with_overrides(extras))
     }
 
     fn with_overrides(mut self, extras: &BTreeMap<CrateId, PairedExtras>) -> Self {
@@ -755,7 +755,7 @@ impl CrateContext {
         gen_binaries: &GenBinaries,
         include_build_scripts: bool,
         sources_are_present: bool,
-    ) -> BTreeSet<Rule> {
+    ) -> anyhow::Result<BTreeSet<Rule>> {
         let package = &packages[&node.id];
 
         let package_root = package
@@ -774,6 +774,10 @@ impl CrateContext {
                     // content to align when rendering, the package target names are always sanitized.
                     let crate_name = sanitize_module_name(&target.name);
 
+                    if !target.src_path.starts_with(package_root) {
+                        return Some(Err(anyhow::anyhow!("Package {:?} target {:?} had an absolute source path {:?}, which is not supported", package.name, target.name, target.src_path)));
+                    }
+
                     // Locate the crate's root source file relative to the package root normalized for unix
                     let crate_root = pathdiff::diff_paths(&target.src_path, package_root).map(
                         // Normalize the path so that it always renders the same regardless of platform
@@ -782,29 +786,29 @@ impl CrateContext {
 
                     // Conditionally check to see if the dependencies is a build-script target
                     if include_build_scripts && kind == "custom-build" {
-                        return Some(Rule::BuildScript(TargetAttributes {
+                        return Some(Ok(Rule::BuildScript(TargetAttributes {
                             crate_name,
                             crate_root,
                             srcs: Glob::new_rust_srcs(!sources_are_present),
-                        }));
+                        })));
                     }
 
                     // Check to see if the dependencies is a proc-macro target
                     if kind == "proc-macro" {
-                        return Some(Rule::ProcMacro(TargetAttributes {
+                        return Some(Ok(Rule::ProcMacro(TargetAttributes {
                             crate_name,
                             crate_root,
                             srcs: Glob::new_rust_srcs(!sources_are_present),
-                        }));
+                        })));
                     }
 
                     // Check to see if the dependencies is a library target
                     if ["lib", "rlib"].contains(&kind.as_str()) {
-                        return Some(Rule::Library(TargetAttributes {
+                        return Some(Ok(Rule::Library(TargetAttributes {
                             crate_name,
                             crate_root,
                             srcs: Glob::new_rust_srcs(!sources_are_present),
-                        }));
+                        })));
                     }
 
                     // Check if the target kind is binary and is one of the ones included in gen_binaries
@@ -814,11 +818,11 @@ impl CrateContext {
                             GenBinaries::Some(set) => set.contains(&target.name),
                         }
                     {
-                        return Some(Rule::Binary(TargetAttributes {
+                        return Some(Ok(Rule::Binary(TargetAttributes {
                             crate_name: target.name.clone(),
                             crate_root,
                             srcs: Glob::new_rust_srcs(!sources_are_present),
-                        }));
+                        })));
                     }
 
                     None
@@ -866,7 +870,8 @@ mod test {
             include_binaries,
             include_build_scripts,
             are_sources_present,
-        );
+        )
+        .unwrap();
 
         assert_eq!(context.name, "common");
         assert_eq!(
@@ -914,7 +919,8 @@ mod test {
             include_binaries,
             include_build_scripts,
             are_sources_present,
-        );
+        )
+        .unwrap();
 
         assert_eq!(context.name, "common");
         assert_eq!(
@@ -979,7 +985,8 @@ mod test {
             include_binaries,
             include_build_scripts,
             are_sources_present,
-        );
+        )
+        .unwrap();
 
         assert_eq!(context.name, "openssl-sys");
         assert!(context.build_script_attrs.is_some());
@@ -1026,7 +1033,8 @@ mod test {
             include_binaries,
             include_build_scripts,
             are_sources_present,
-        );
+        )
+        .unwrap();
 
         assert_eq!(context.name, "openssl-sys");
         assert!(context.build_script_attrs.is_none());
@@ -1062,7 +1070,8 @@ mod test {
             include_binaries,
             include_build_scripts,
             are_sources_present,
-        );
+        )
+        .unwrap();
 
         assert_eq!(context.name, "sysinfo");
         assert!(context.build_script_attrs.is_none());
@@ -1104,7 +1113,8 @@ mod test {
             include_binaries,
             include_build_scripts,
             are_sources_present,
-        );
+        )
+        .unwrap();
 
         assert_eq!(context.name, "common");
         check_context(context);
@@ -1236,11 +1246,44 @@ mod test {
             include_binaries,
             include_build_scripts,
             are_sources_present,
-        );
+        )
+        .unwrap();
 
         let mut expected = Select::new();
         expected.insert("unique_feature".to_owned(), None);
 
         assert_eq!(context.common_attrs.crate_features, expected);
     }
+
+    #[test]
+    fn absolute_paths_for_srcs_are_errors() {
+        let annotations = Annotations::new(
+            crate::test::metadata::abspath(),
+            crate::test::lockfile::abspath(),
+            crate::config::Config::default(),
+        )
+        .unwrap();
+
+        let crate_annotation = &annotations.metadata.crates[&PackageId {
+            repr: "path+file://{TEMP_DIR}/common#0.1.0".to_owned(),
+        }];
+
+        let include_binaries = false;
+        let include_build_scripts = false;
+        let are_sources_present = false;
+        let err = CrateContext::new(
+            crate_annotation,
+            &annotations.metadata.packages,
+            &annotations.lockfile.crates,
+            &annotations.pairred_extras,
+            &annotations.metadata.workspace_metadata.tree_metadata,
+            include_binaries,
+            include_build_scripts,
+            are_sources_present,
+        )
+        .unwrap_err()
+        .to_string();
+
+        assert_eq!(err, "Package \"common\" target \"common\" had an absolute source path \"/dev/null\", which is not supported");
+    }
 }

crate_universe/src/test.rs

@@ -170,6 +170,14 @@ pub(crate) mod metadata {
         )))
         .unwrap()
     }
+
+    pub(crate) fn abspath() -> cargo_metadata::Metadata {
+        serde_json::from_str(include_str!(concat!(
+            env!("CARGO_MANIFEST_DIR"),
+            "/test_data/metadata/abspath/metadata.json"
+        )))
+        .unwrap()
+    }
 }
 
 pub(crate) mod lockfile {
@@ -238,4 +246,12 @@ pub(crate) mod lockfile {
         )))
         .unwrap()
     }
+
+    pub(crate) fn abspath() -> cargo_lock::Lockfile {
+        cargo_lock::Lockfile::from_str(include_str!(concat!(
+            env!("CARGO_MANIFEST_DIR"),
+            "/test_data/metadata/abspath/Cargo.lock"
+        )))
+        .unwrap()
+    }
 }

crate_universe/test_data/metadata/abspath/Cargo.lock

@@ -0,0 +1,7 @@
+[package]
+name = "common"
+version = "0.1.0"
+edition = "2018"
+
+[lib]
+path = "fake.rs"

crate_universe/test_data/metadata/abspath/Cargo.toml

@@ -5,4 +5,4 @@ edition = "2021"
 publish = false
 
 [lib]
-path = "/dev/null"
+path = "fake.rs"

crate_universe/test_data/metadata/abspath/metadata.json

@@ -6,7 +6,7 @@ edition = "2021"
 publish = false
 
 [lib]
-path = "/dev/null"
+path = "fake.rs"
 
 [dependencies]
 anyhow = "1.0.79"

examples/bazel_env/rust/hello_world/Cargo.toml

@@ -6,7 +6,7 @@ edition = "2021"
 publish = false
 
 [lib]
-path = "/dev/null"
+path = "fake.rs"
 
 [dependencies]
 anyhow = "1.0.77"

examples/bzlmod/all_crate_deps/Cargo.toml

@@ -6,7 +6,7 @@ edition = "2021"
 publish = false
 
 [lib]
-path = "/dev/null"
+path = "fake.rs"
 
 [dependencies.foo]
 version = "0.0.0"