From a6f6fdfc839658433b19cd2ffb6fed3b81096276 Mon Sep 17 00:00:00 2001 From: Waleed Khan Date: Wed, 21 Sep 2022 17:16:22 -0700 Subject: [PATCH 1/4] new: respect `--template` for local clones I don't remember why I ignored the user's explicit `--template` setting in this case. --- focus/operations/src/clone.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/focus/operations/src/clone.rs b/focus/operations/src/clone.rs index db1cc3e7..1fb35a45 100644 --- a/focus/operations/src/clone.rs +++ b/focus/operations/src/clone.rs @@ -363,13 +363,7 @@ pub fn run( app.clone(), )?; - if template.is_some() { - warn!( - ?template, - "Repo template is ignored since this is a local clone" - ); - } - None + template } Origin::Remote(url) => { clone_remote( From 6d5f327ae8015f297e2d4b252bae2e44a610a626 Mon Sep 17 00:00:00 2001 From: Waleed Khan Date: Wed, 21 Sep 2022 17:16:53 -0700 Subject: [PATCH 2/4] Implement `Debug` for `SelectionManager` --- focus/internals/src/lib/model/selection/project.rs | 1 + focus/internals/src/lib/model/selection/selection.rs | 1 + 2 files changed, 2 insertions(+) diff --git a/focus/internals/src/lib/model/selection/project.rs b/focus/internals/src/lib/model/selection/project.rs index abb2399d..3b115746 100644 --- a/focus/internals/src/lib/model/selection/project.rs +++ b/focus/internals/src/lib/model/selection/project.rs @@ -232,6 +232,7 @@ impl TryFrom<&ProjectIndex> for TargetSet { } /// ProjectCatalog maintains indices of optional and mandatory projects defined in a repository. +#[derive(Debug)] pub struct ProjectCatalog { pub optional_projects: ProjectIndex, pub mandatory_projects: ProjectIndex, diff --git a/focus/internals/src/lib/model/selection/selection.rs b/focus/internals/src/lib/model/selection/selection.rs index 5f596435..779754a1 100644 --- a/focus/internals/src/lib/model/selection/selection.rs +++ b/focus/internals/src/lib/model/selection/selection.rs @@ -189,6 +189,7 @@ impl<'processor> SelectionOperationProcessor<'processor> { } /// SelectionManager maintains the current selection within a repository. It also provides access to projects defined in the repository via the `project_catalog()` method and associated structure. +#[derive(Debug)] pub struct SelectionManager { /// The path where the selection is stored. selection_path: PathBuf, From d21d153d18dc6b9ed43e345dffd5596b05ceb07c Mon Sep 17 00:00:00 2001 From: Waleed Khan Date: Wed, 14 Sep 2022 16:30:52 -0500 Subject: [PATCH 3/4] fix: traverse `load` dependencies in `WORKSPACE` --- focus/internals/src/lib/index/content_hash.rs | 109 ++++++++----- .../src/lib/index/dependency_graph.rs | 145 +++++++++++++++++- focus/operations/src/index.rs | 7 + ...ting__sync__sync_layer_manipulation-4.snap | 1 + ...ting__sync__sync_layer_manipulation-5.snap | 1 + ...ips_checkout_with_unchanged_profile-2.snap | 1 + ...skips_checkout_with_unchanged_profile.snap | 1 + 7 files changed, 221 insertions(+), 44 deletions(-) diff --git a/focus/internals/src/lib/index/content_hash.rs b/focus/internals/src/lib/index/content_hash.rs index 635dcbda..47283280 100644 --- a/focus/internals/src/lib/index/content_hash.rs +++ b/focus/internals/src/lib/index/content_hash.rs @@ -173,11 +173,52 @@ fn content_hash_dependency_key(ctx: &HashContext, key: DependencyKey) -> Result< } } - enum KeyOrPath<'a> { - Key(DependencyKey), - Path(&'a Path), + let (kind, maybe_label, values_to_hash) = get_dependencies(ctx, &key)?; + + let mut buf = String::new(); + write!(&mut buf, "DependencyKeyV{VERSION}::{kind}(")?; + if let Some(label) = maybe_label { + write!(&mut buf, "{label}, ")?; + } + let hashes = values_to_hash + .into_iter() + .map(|key_or_hash| match key_or_hash { + KeyOrPath::Key(dep_key) => content_hash_dependency_key(ctx, dep_key), + KeyOrPath::Path(path) => content_hash_tree_path(ctx, path), + }) + .collect::>>()?; + for hash in hashes { + write!(&mut buf, "{hash}, ")?; + } + write!(&mut buf, ")")?; + let hash = git2::Oid::hash_object(git2::ObjectType::Blob, buf.as_bytes()) + .map_err(Error::HashObject)?; + let hash = ContentHash(hash); + + if let Some(old_value) = ctx + .caches + .borrow_mut() + .dependency_key_cache + .insert(key.to_owned(), hash.clone()) + { + if old_value != hash { + error!(?key, ?old_value, new_value = ?hash, "Non-deterministic content hashing for dependency key"); + } } - let (kind, maybe_label, values_to_hash) = match &key { + Ok(hash) +} + +#[derive(Debug)] +enum KeyOrPath<'a> { + Key(DependencyKey), + Path(&'a Path), +} + +fn get_dependencies<'a>( + ctx: &HashContext, + dep_key: &'a DependencyKey, +) -> Result<(&'static str, Option<&'a Label>, Vec>)> { + match dep_key { DependencyKey::BazelPackage( label @ Label { external_repository, @@ -212,11 +253,11 @@ fn content_hash_dependency_key(ctx: &HashContext, key: DependencyKey) -> Result< target_name: TargetName::Name("WORKSPACE".to_string()), })); - ( + Ok(( "BazelPackage", Some(label), dep_keys.into_iter().map(KeyOrPath::Key).collect(), - ) + )) } DependencyKey::BazelBuildFile( @@ -259,7 +300,7 @@ fn content_hash_dependency_key(ctx: &HashContext, key: DependencyKey) -> Result< loaded_deps .into_iter() .map(DependencyKey::BazelBuildFile) - .filter(|dep_key| &key != dep_key), + .filter(|key| dep_key != key), ); dep_keys @@ -273,53 +314,37 @@ fn content_hash_dependency_key(ctx: &HashContext, key: DependencyKey) -> Result< // here. dep_keys.push(DependencyKey::Path("WORKSPACE".into())); - ( + Ok(( "BazelBuildFile", Some(label), dep_keys.into_iter().map(KeyOrPath::Key).collect(), - ) + )) } - DependencyKey::Path(path) => ("Path", None, vec![KeyOrPath::Path(path)]), + DependencyKey::Path(path) => Ok(("Path", None, vec![KeyOrPath::Path(path)])), - DependencyKey::DummyForTesting(inner_dep_key) => ( + DependencyKey::DummyForTesting(inner_dep_key) => Ok(( "DummyForTesting", None, vec![KeyOrPath::Key(inner_dep_key.as_ref().clone())], - ), - }; - - let mut buf = String::new(); - write!(&mut buf, "DependencyKeyV{VERSION}::{kind}(")?; - if let Some(label) = maybe_label { - write!(&mut buf, "{label}, ")?; + )), } - let hashes = values_to_hash +} + +pub fn get_workspace_deps(ctx: &HashContext) -> Result> { + let workspace_key = DependencyKey::BazelBuildFile(Label { + external_repository: None, + path_components: Vec::new(), + target_name: TargetName::Name("WORKSPACE".to_string()), + }); + let (_, _, deps) = get_dependencies(ctx, &workspace_key)?; + Ok(deps .into_iter() - .map(|key_or_hash| match key_or_hash { - KeyOrPath::Key(dep_key) => content_hash_dependency_key(ctx, dep_key), - KeyOrPath::Path(path) => content_hash_tree_path(ctx, path), + .filter_map(|dep_key| match dep_key { + KeyOrPath::Key(key) => Some(key), + KeyOrPath::Path(_) => None, }) - .collect::>>()?; - for hash in hashes { - write!(&mut buf, "{hash}, ")?; - } - write!(&mut buf, ")")?; - let hash = git2::Oid::hash_object(git2::ObjectType::Blob, buf.as_bytes()) - .map_err(Error::HashObject)?; - let hash = ContentHash(hash); - - if let Some(old_value) = ctx - .caches - .borrow_mut() - .dependency_key_cache - .insert(key.to_owned(), hash.clone()) - { - if old_value != hash { - error!(?key, ?old_value, new_value = ?hash, "Non-deterministic content hashing for dependency key"); - } - } - Ok(hash) + .collect()) } /// Get the dependencies induced by the special diff --git a/focus/internals/src/lib/index/dependency_graph.rs b/focus/internals/src/lib/index/dependency_graph.rs index ccaa4cda..b5dbc96a 100644 --- a/focus/internals/src/lib/index/dependency_graph.rs +++ b/focus/internals/src/lib/index/dependency_graph.rs @@ -4,7 +4,7 @@ use std::collections::{BTreeSet, HashSet}; use std::path::PathBuf; -use crate::index::content_hash::get_prelude_deps; +use crate::index::content_hash::{get_prelude_deps, get_workspace_deps}; use serde::{Deserialize, Serialize}; use tracing::{debug, warn}; @@ -15,7 +15,7 @@ use super::content_hash::HashContext; use super::{ContentHash, ObjectDatabase}; /// A key into the "Focus Build Graph" which lets us identify a corresponding -/// [`DependencyValue`] node. A [`DependencyKey`] in combination with a +/// [`DependencyValue`] node. A [`DependencyKey`] in combination with aG /// snapshot of the repository is *syntactically* content-addressable in this /// sense: the hash of the [`DependencyKey`] can be calculated only by looking /// at file contents, without having to evaluate any Bazel queries. @@ -221,6 +221,8 @@ pub fn get_files_to_materialize( let mut dep_keys = dep_keys; debug!(?dep_keys, "Initial set of dependency keys"); + dep_keys.extend(get_workspace_deps(ctx)?); + // The result of `bazel query` appears to not include dependencies that are // caused by `prelude_bazel`, so we have to manually add them as part of the // file materialization process. @@ -436,6 +438,9 @@ sh_binary( BazelPackage( Label("//package1:foo"), ), + Path( + "WORKSPACE", + ), }, } "###); @@ -515,8 +520,12 @@ sh_binary( BazelPackage( Label("//package2:bar.sh"), ), + Path( + "WORKSPACE", + ), }, paths: { + "WORKSPACE", "package1", "package2", }, @@ -608,8 +617,12 @@ New contents BazelPackage( Label("//package2:contents"), ), + Path( + "WORKSPACE", + ), }, paths: { + "WORKSPACE", "package1", "package2", }, @@ -660,6 +673,9 @@ def my_macro_inner(name): BazelPackage( Label("//package1:foo"), ), + Path( + "WORKSPACE", + ), }, } "###); @@ -685,8 +701,12 @@ def my_macro_inner(name): BazelPackage( Label("//package3:contents"), ), + Path( + "WORKSPACE", + ), }, paths: { + "WORKSPACE", "package1", "package3", }, @@ -757,8 +777,16 @@ def some_macro(): BazelPackage( Label("//package1:foo.sh"), ), + BazelBuildFile( + Label("//macro:macro.bzl"), + ), + Path( + "WORKSPACE", + ), }, paths: { + "WORKSPACE", + "macro", "package1", }, } @@ -799,6 +827,12 @@ def some_macro(): BazelPackage( Label("//package1:foo"), ), + BazelBuildFile( + Label("//macro:macro.bzl"), + ), + Path( + "WORKSPACE", + ), }, } "###); @@ -872,8 +906,12 @@ macro("foo") BazelBuildFile( Label("//tools/build_rules:prelude_bazel"), ), + Path( + "WORKSPACE", + ), }, paths: { + "WORKSPACE", "macro", "package1", "tools/build_rules", @@ -945,6 +983,9 @@ def macro2(name): BazelBuildFile( Label("//tools/build_rules:prelude_bazel"), ), + Path( + "WORKSPACE", + ), }, } "###); @@ -960,8 +1001,12 @@ def macro2(name): BazelBuildFile( Label("//tools/build_rules:prelude_bazel"), ), + Path( + "WORKSPACE", + ), }, paths: { + "WORKSPACE", "macro", "package1", "tools/build_rules", @@ -1032,8 +1077,16 @@ def some_macro(): BazelPackage( Label("//package1:foo.sh"), ), + BazelBuildFile( + Label("//macro:macro.bzl"), + ), + Path( + "WORKSPACE", + ), }, paths: { + "WORKSPACE", + "macro", "package1", }, } @@ -1147,8 +1200,12 @@ foo( BazelPackage( Label("//package1/some/sub/package:foo.sh"), ), + Path( + "WORKSPACE", + ), }, paths: { + "WORKSPACE", "package1", "package1/some/sub/package", }, @@ -1215,6 +1272,9 @@ def foo(name, srcs): BazelPackage( Label("//package1/some/sub/package:foo.sh"), ), + Path( + "WORKSPACE", + ), }, } "###); @@ -1260,4 +1320,85 @@ def foo(name, srcs): Ok(()) } + + #[test] + fn test_workspace_load_statements() -> anyhow::Result<()> { + init_logging(); + + let temp = tempfile::tempdir()?; + let fix = ScratchGitRepo::new_static_fixture(temp.path())?; + + write_files( + &fix, + r#" +file: WORKSPACE +load("//repository_rule:rule.bzl", "foo") +foo("bar") + +file: repository_rule/rule.bzl +def foo(name): + pass + +file: repository_rule/BUILD + +file: package1/foo.sh + +file: package1/BUILD +sh_binary( + name = "foo", + srcs = ["foo.sh"], +) +"#, + )?; + let head_oid = fix.commit_all("Wrote files")?; + let repo = fix.repo()?; + + let app = Arc::new(App::new_for_testing()?); + let cache_dir = tempfile::tempdir()?; + let resolver = BazelResolver::new(cache_dir.path()); + let target_set = hashset! { + "bazel://package1:foo".try_into()?, + }; + + let request = ResolutionRequest { + repo: fix.path().to_path_buf(), + targets: target_set, + }; + let cache_options = CacheOptions::default(); + let resolve_result = resolver.resolve(&request, &cache_options, app)?; + + let odb = HashMapOdb::new(); + let files_to_materialize = { + let head_commit = repo.find_commit(head_oid)?; + let head_tree = head_commit.tree()?; + let ctx = HashContext::new(&repo, &head_tree)?; + update_object_database_from_resolution(&ctx, &odb, &resolve_result)?; + get_files_to_materialize(&ctx, &odb, hashset! { parse_label("//package1:foo")? })? + }; + insta::assert_debug_snapshot!(files_to_materialize, @r###" + Ok { + seen_keys: { + BazelPackage( + Label("//package1:foo"), + ), + BazelPackage( + Label("//package1:foo.sh"), + ), + BazelBuildFile( + Label("//repository_rule:rule.bzl"), + ), + Path( + "WORKSPACE", + ), + }, + paths: { + "WORKSPACE", + "package1", + "repository_rule", + }, + } + "###); + + Ok(()) + } } diff --git a/focus/operations/src/index.rs b/focus/operations/src/index.rs index 5a5b0403..bee814cc 100644 --- a/focus/operations/src/index.rs +++ b/focus/operations/src/index.rs @@ -489,6 +489,9 @@ mod tests { BazelBuildFile( Label("//tools/build_rules:prelude_bazel"), ), + Path( + "WORKSPACE", + ), }, } "###); @@ -523,8 +526,12 @@ mod tests { BazelBuildFile( Label("//tools/build_rules:prelude_bazel"), ), + Path( + "WORKSPACE", + ), }, paths: { + "WORKSPACE", "library_a", "project_a/src/main/java/com/example/cmdline", "tools/build_rules", diff --git a/focus/operations/src/testing/snapshots/focus_operations__testing__sync__sync_layer_manipulation-4.snap b/focus/operations/src/testing/snapshots/focus_operations__testing__sync__sync_layer_manipulation-4.snap index f7186aa2..83a465fe 100644 --- a/focus/operations/src/testing/snapshots/focus_operations__testing__sync__sync_layer_manipulation-4.snap +++ b/focus/operations/src/testing/snapshots/focus_operations__testing__sync__sync_layer_manipulation-4.snap @@ -4,6 +4,7 @@ expression: "std::fs::read_to_string(&profile_path)?" --- /* !/*/ +/WORKSPACE/ /focus/ /library_b/ /mandatory_y/ diff --git a/focus/operations/src/testing/snapshots/focus_operations__testing__sync__sync_layer_manipulation-5.snap b/focus/operations/src/testing/snapshots/focus_operations__testing__sync__sync_layer_manipulation-5.snap index 72a53ba2..8ae92228 100644 --- a/focus/operations/src/testing/snapshots/focus_operations__testing__sync__sync_layer_manipulation-5.snap +++ b/focus/operations/src/testing/snapshots/focus_operations__testing__sync__sync_layer_manipulation-5.snap @@ -4,6 +4,7 @@ expression: "std::fs::read_to_string(&profile_path)?" --- /* !/*/ +/WORKSPACE/ /focus/ /mandatory_y/ !/mandatory_y/*/ diff --git a/focus/operations/src/testing/snapshots/focus_operations__testing__sync__sync_skips_checkout_with_unchanged_profile-2.snap b/focus/operations/src/testing/snapshots/focus_operations__testing__sync__sync_skips_checkout_with_unchanged_profile-2.snap index 4fefdd0e..b7065dde 100644 --- a/focus/operations/src/testing/snapshots/focus_operations__testing__sync__sync_skips_checkout_with_unchanged_profile-2.snap +++ b/focus/operations/src/testing/snapshots/focus_operations__testing__sync__sync_skips_checkout_with_unchanged_profile-2.snap @@ -4,6 +4,7 @@ expression: original_profile_contents --- /* !/*/ +/WORKSPACE/ /focus/ /mandatory_y/ !/mandatory_y/*/ diff --git a/focus/operations/src/testing/snapshots/focus_operations__testing__sync__sync_skips_checkout_with_unchanged_profile.snap b/focus/operations/src/testing/snapshots/focus_operations__testing__sync__sync_skips_checkout_with_unchanged_profile.snap index 4fefdd0e..b7065dde 100644 --- a/focus/operations/src/testing/snapshots/focus_operations__testing__sync__sync_skips_checkout_with_unchanged_profile.snap +++ b/focus/operations/src/testing/snapshots/focus_operations__testing__sync__sync_skips_checkout_with_unchanged_profile.snap @@ -4,6 +4,7 @@ expression: original_profile_contents --- /* !/*/ +/WORKSPACE/ /focus/ /mandatory_y/ !/mandatory_y/*/ From dd17d66c9308a74a0fc2420ac5d25e566ac7f270 Mon Sep 17 00:00:00 2001 From: Waleed Khan Date: Wed, 14 Sep 2022 17:19:39 -0500 Subject: [PATCH 4/4] ci: add Bazel smoke test CI job --- .github/workflows/linux.yml | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/.github/workflows/linux.yml b/.github/workflows/linux.yml index 2d407d73..b3d49d02 100644 --- a/.github/workflows/linux.yml +++ b/.github/workflows/linux.yml @@ -38,3 +38,13 @@ jobs: - name: Run Rust doc-tests timeout-minutes: 10 run: cargo test --features ci --doc --workspace + + - name: Run smoke test on `bazel` repo + run: | + REPO_DIR="../bazel-focus" + cargo run -- new --dense-repo https://github.com/bazelbuild/bazel --template bazel "$REPO_DIR" + cargo run -- -C "$REPO_DIR" add bazel://src/main/cpp:blaze_util + # FIXME: this sync shouldn't be necessary, but `focus add` seems to + # lose the changes from the repo template. + cargo run -- -C "$REPO_DIR" sync + (cd "$REPO_DIR" && bazel build //src/main/cpp:blaze_util)