Followup

1. Address review comment
2. Fix/add unit tests: bazelisk test //...
3. Fix clippy: bazelisk build //... --config=clippy
4. Fix path patching tests: bazelisk run //:vendor_edit_test_out_of_tree
   a. Recognize when patched path is from a temporary Bazel dir, and
      rebase properly back to Bazel workspace (previous logic).
   b. Don't try to read BUILD.bazel when path patching in
      extensions.bzl, since the BUILD file is writted directly to
      patched path, which does not necessarily follow the convention
      (naming and location) of other generated build files.

Change-Id: Ie3cddbafb7b524c0ea70494d76905475be1fc742

Author: jayzhuang

crate_universe/Cargo.toml

@@ -21,6 +21,7 @@ exclude = [
     "test_data/metadata/multi_kind_proc_macro_dep",
     "test_data/metadata/nested_build_dependencies",
     "test_data/metadata/no_deps",
+    "test_data/metadata/path_patching",
     "test_data/metadata/resolver_2_deps",
     "test_data/metadata/target_cfg_features",
     "test_data/metadata/target_features",

crate_universe/extensions.bzl

@@ -713,9 +713,9 @@ def _generate_hub_and_spokes(
             version = version.replace("+", "-"),
         )
 
-        build_file_content = module_ctx.read(crates_dir.get_child("BUILD.%s-%s.bazel" % (name, version)))
         if "Http" in repo:
             # Replicates functionality in repo_http.j2.
+            build_file_content = module_ctx.read(crates_dir.get_child("BUILD.%s-%s.bazel" % (name, version)))
             repo = repo["Http"]
             http_archive(
                 name = crate_repo_name,
@@ -731,6 +731,7 @@ def _generate_hub_and_spokes(
             )
         elif "Git" in repo:
             # Replicates functionality in repo_git.j2
+            build_file_content = module_ctx.read(crates_dir.get_child("BUILD.%s-%s.bazel" % (name, version)))
             repo = repo["Git"]
             kwargs = {}
             for k, v in repo["commitish"].items():

crate_universe/src/api/lockfile.rs

@@ -176,6 +176,7 @@ mod test {
                     },
                     target: String::from("anyhow"),
                     alias: None,
+                    source_annotation: None,
                 },
                 CrateDependency {
                     id: CrateId {
@@ -184,6 +185,7 @@ mod test {
                     },
                     target: String::from("reqwest"),
                     alias: None,
+                    source_annotation: None,
                 },
             ],
         );

crate_universe/src/context.rs

@@ -269,6 +269,7 @@ mod test {
     fn mock_context_common() -> Context {
         let annotations = Annotations::new(
             crate::test::metadata::common(),
+            &None,
             crate::test::lockfile::common(),
             Config::default(),
             Utf8Path::new("/tmp/bazelworkspace"),
@@ -281,6 +282,7 @@ mod test {
     fn mock_context_aliases() -> Context {
         let annotations = Annotations::new(
             crate::test::metadata::alias(),
+            &None,
             crate::test::lockfile::alias(),
             Config::default(),
             Utf8Path::new("/tmp/bazelworkspace"),
@@ -293,6 +295,7 @@ mod test {
     fn mock_context_workspace_build_scripts_deps() -> Context {
         let annotations = Annotations::new(
             crate::test::metadata::workspace_build_scripts_deps(),
+            &None,
             crate::test::lockfile::workspace_build_scripts_deps(),
             Config {
                 generate_build_scripts: true,

crate_universe/src/context/crate_context.rs

@@ -26,6 +26,7 @@ pub struct CrateDependency {
     #[serde(default, skip_serializing_if = "Option::is_none")]
     pub alias: Option<String>,
 
+    /// Where to acquire the source of this dependency.
     #[serde(default, skip_serializing_if = "Option::is_none")]
     pub(crate) source_annotation: Option<SourceAnnotation>,
 }
@@ -479,10 +480,7 @@ impl CrateContext {
                     id: current_crate_id,
                     target: target.crate_name.clone(),
                     alias: None,
-                    source_annotation: match source_annotations.get(&annotation.node.id) {
-                        Some(source_annotation) => Some(source_annotation.clone()),
-                        None => None,
-                    },
+                    source_annotation: source_annotations.get(&annotation.node.id).cloned(),
                 },
                 None,
             );
@@ -882,6 +880,7 @@ mod test {
     fn common_annotations() -> Annotations {
         Annotations::new(
             crate::test::metadata::common(),
+            &None,
             crate::test::lockfile::common(),
             crate::config::Config::default(),
             Utf8Path::new("/tmp/bazelworkspace"),
@@ -986,6 +985,7 @@ mod test {
     fn build_script_annotations() -> Annotations {
         Annotations::new(
             crate::test::metadata::build_scripts(),
+            &None,
             crate::test::lockfile::build_scripts(),
             crate::config::Config::default(),
             Utf8Path::new("/tmp/bazelworkspace"),
@@ -996,6 +996,7 @@ mod test {
     fn crate_type_annotations() -> Annotations {
         Annotations::new(
             crate::test::metadata::crate_types(),
+            &None,
             crate::test::lockfile::crate_types(),
             crate::config::Config::default(),
             Utf8Path::new("/tmp/bazelworkspace"),
@@ -1300,6 +1301,7 @@ mod test {
     fn absolute_paths_for_srcs_are_errors() {
         let annotations = Annotations::new(
             crate::test::metadata::abspath(),
+            &None,
             crate::test::lockfile::abspath(),
             crate::config::Config::default(),
             Utf8Path::new("/tmp/bazelworkspace"),

crate_universe/src/context/platforms.rs

@@ -135,6 +135,7 @@ mod test {
                 id: CrateId::new("mock_crate_b".to_owned(), VERSION_ZERO_ONE_ZERO),
                 target: "mock_crate_b".to_owned(),
                 alias: None,
+                source_annotation: None,
             },
             None,
         );
@@ -193,6 +194,7 @@ mod test {
                 id: CrateId::new("mock_crate_b".to_owned(), VERSION_ZERO_ONE_ZERO),
                 target: "mock_crate_b".to_owned(),
                 alias: None,
+                source_annotation: None,
             },
             Some(configuration),
         );
@@ -279,6 +281,7 @@ mod test {
                 id: CrateId::new("mock_crate_b".to_owned(), VERSION_ZERO_ONE_ZERO),
                 target: "mock_crate_b".to_owned(),
                 alias: None,
+                source_annotation: None,
             },
             Some(configuration.clone()),
         );
@@ -345,6 +348,7 @@ mod test {
                 id: CrateId::new("mock_crate_b".to_owned(), VERSION_ZERO_ONE_ZERO),
                 target: "mock_crate_b".to_owned(),
                 alias: None,
+                source_annotation: None,
             },
             Some(configuration.clone()),
         );

crate_universe/src/metadata.rs

@@ -361,7 +361,7 @@ pub(crate) fn load_metadata(
     let metadata =
         serde_json::from_str(&content).context("Unable to deserialize Cargo metadata")?;
 
-    let lockfile = cargo_lock::Lockfile::load(&lockfile_path)
+    let lockfile = cargo_lock::Lockfile::load(lockfile_path)
         .with_context(|| format!("Failed to load lockfile: {}", lockfile_path.display()))?;
 
     Ok((metadata, lockfile))

crate_universe/src/metadata/metadata_annotation.rs

@@ -3,7 +3,7 @@
 use std::collections::{BTreeMap, BTreeSet};
 use std::path::PathBuf;
 
-use anyhow::{bail, Result};
+use anyhow::{bail, Context, Result};
 use camino::{Utf8Path, Utf8PathBuf};
 use cargo_metadata::{Node, Package, PackageId};
 use hex::ToHex;
@@ -277,21 +277,28 @@ impl LockfileAnnotation {
                                         // its parent directory (relative to Bazel workspace root),
                                         // since `suffix` is relative to it in the actual Bazel
                                         // workspace.
-                                        let relative_lockfile_path = Utf8Path::from_path(
-                                            path.parent().expect("unexpected empty lockfile path"),
+                                        let p = Utf8Path::from_path(
+                                            path.parent()
+                                                .context("unexpected empty lockfile path")?,
                                         )
-                                        .expect("unxpected non-Unicode lockfile path")
-                                        .strip_prefix(nonhermetic_root_bazel_workspace_dir)
-                                        .expect(
-                                            "unexpected lockfile path no under root Bazel workspace",
-                                        );
-                                        new_path.push(relative_lockfile_path);
-                                    } else if let Some(prefix) =
-                                        // Lockfile path includes workspace prefix, so avoid
-                                        // duplicating it here.
-                                        workspace_metadata.workspace_prefix.as_ref()
-                                    {
-                                        new_path.push(prefix);
+                                        .context("unxpected non-Unicode lockfile path")?;
+                                        if p.starts_with(nonhermetic_root_bazel_workspace_dir) {
+                                            // If path in lockfile is under Bazel root, strip Bazel
+                                            // root to get the actual path.
+                                            let relative_lockfile_path = p.strip_prefix(nonhermetic_root_bazel_workspace_dir)
+                                                .context("unexpected lockfile path not under root Bazel workspace")?;
+                                            new_path.push(relative_lockfile_path);
+                                        } else {
+                                            // If path in lockfile is not under Bazel root, we are
+                                            // likely in a temporary directory, so rebase to Bazel
+                                            // root.
+                                            new_path.push(nonhermetic_root_bazel_workspace_dir);
+                                            if let Some(prefix) =
+                                                workspace_metadata.workspace_prefix.as_ref()
+                                            {
+                                                new_path.push(prefix);
+                                            }
+                                        }
                                     }
                                     new_path.push(suffix);
                                     new_path
@@ -581,6 +588,7 @@ mod test {
     #[test]
     fn annotate_lockfile_with_aliases() {
         LockfileAnnotation::new(
+            &None,
             test::lockfile::alias(),
             &test::metadata::alias(),
             Utf8Path::new("/tmp/bazelworkspace"),
@@ -596,6 +604,7 @@ mod test {
     #[test]
     fn annotate_lockfile_with_build_scripts() {
         LockfileAnnotation::new(
+            &None,
             test::lockfile::build_scripts(),
             &test::metadata::build_scripts(),
             Utf8Path::new("/tmp/bazelworkspace"),
@@ -606,6 +615,7 @@ mod test {
     #[test]
     fn annotate_lockfile_with_no_deps() {
         LockfileAnnotation::new(
+            &None,
             test::lockfile::no_deps(),
             &test::metadata::no_deps(),
             Utf8Path::new("/tmp/bazelworkspace"),
@@ -616,6 +626,7 @@ mod test {
     #[test]
     fn detects_strip_prefix_for_git_repo() {
         let crates = LockfileAnnotation::new(
+            &None,
             test::lockfile::git_repos(),
             &test::metadata::git_repos(),
             Utf8Path::new("/tmp/bazelworkspace"),
@@ -643,6 +654,7 @@ mod test {
     #[test]
     fn resolves_commit_from_branches_and_tags() {
         let crates = LockfileAnnotation::new(
+            &None,
             test::lockfile::git_repos(),
             &test::metadata::git_repos(),
             Utf8Path::new("/tmp/bazelworkspace"),
@@ -666,6 +678,27 @@ mod test {
         );
     }
 
+    #[test]
+    fn detects_local_path_patching() {
+        let crates = LockfileAnnotation::new(
+            &None,
+            test::lockfile::path_patching(),
+            &test::metadata::path_patching(),
+            Utf8Path::new("/tmp/bazelworkspace"),
+        )
+        .unwrap()
+        .crates;
+
+        // We can't reliably construct the PackageId because it'll contain local absolute paths,
+        // so compare the entire container, which should only have one element anyways.
+        assert_eq!(
+            crates.into_values().collect::<Vec<_>>(),
+            [SourceAnnotation::Path {
+                path: "child_a".into()
+            }]
+        );
+    }
+
     #[test]
     fn detect_unused_annotation() {
         // Create a config with some random annotation
@@ -677,6 +710,7 @@ mod test {
 
         let result = Annotations::new(
             test::metadata::no_deps(),
+            &None,
             test::lockfile::no_deps(),
             config,
             Utf8Path::new("/tmp/bazelworkspace"),
@@ -715,6 +749,7 @@ mod test {
         // crate author in package metadata.
         let combined_annotations = Annotations::new(
             test::metadata::has_package_metadata(),
+            &None,
             test::lockfile::has_package_metadata(),
             config,
             Utf8Path::new("/tmp/bazelworkspace"),

crate_universe/src/rendering.rs

@@ -1387,6 +1387,7 @@ mod test {
         };
         let annotations = Annotations::new(
             test::metadata::alias(),
+            &None,
             test::lockfile::alias(),
             config,
             Utf8Path::new("/tmp/bazelworkspace"),
@@ -1599,6 +1600,7 @@ mod test {
 
         let annotations = Annotations::new(
             metadata,
+            &None,
             lockfile,
             config.clone(),
             Utf8Path::new("/tmp/bazelworkspace"),
@@ -1987,6 +1989,7 @@ mod test {
                         // this is identical to what we have in the `name` attribute
                         // which creates conflict in `render_module_build_file`
                         alias: Some("mock_crate".into()),
+                        source_annotation: None,
                     }])),
                     ..Default::default()
                 },

crate_universe/src/test.rs

@@ -109,6 +109,14 @@ pub(crate) mod metadata {
         .unwrap()
     }
 
+    pub(crate) fn path_patching() -> cargo_metadata::Metadata {
+        serde_json::from_str(include_str!(concat!(
+            env!("CARGO_MANIFEST_DIR"),
+            "/test_data/metadata/path_patching/metadata.json"
+        )))
+        .unwrap()
+    }
+
     pub(crate) fn optional_deps_disabled() -> cargo_metadata::Metadata {
         serde_json::from_str(include_str!(concat!(
             env!("CARGO_MANIFEST_DIR"),
@@ -249,6 +257,14 @@ pub(crate) mod lockfile {
         .unwrap()
     }
 
+    pub(crate) fn path_patching() -> cargo_lock::Lockfile {
+        cargo_lock::Lockfile::from_str(include_str!(concat!(
+            env!("CARGO_MANIFEST_DIR"),
+            "/test_data/metadata/path_patching/Cargo.lock"
+        )))
+        .unwrap()
+    }
+
     pub(crate) fn common() -> cargo_lock::Lockfile {
         cargo_lock::Lockfile::from_str(include_str!(concat!(
             env!("CARGO_MANIFEST_DIR"),

crate_universe/test_data/metadata/path_patching/Cargo.lock

@@ -0,0 +1,15 @@
+[package]
+name = "path_patching"
+version = "0.1.0"
+edition = "2018"
+
+# Required to satisfy cargo but no `lib.rs` is expected to
+# exist within test data.
+[lib]
+path = "lib.rs"
+
+[dependencies]
+child_a = "1.4.2"
+
+[patch.crates-io]
+child_a = { path = "child_a" }

crate_universe/test_data/metadata/path_patching/Cargo.toml

@@ -0,0 +1,11 @@
+[package]
+name = "child_a"
+version = "1.4.2"
+edition = "2018"
+
+# Required to satisfy cargo but no `lib.rs` is expected to
+# exist within test data.
+[lib]
+path = "lib.rs"
+
+[dependencies]