From 29362fbcfaf3a6530113eb672498aed4ec28fcda Mon Sep 17 00:00:00 2001 From: Maxwell Elliott Date: Mon, 17 May 2021 14:22:28 -0700 Subject: [PATCH 1/3] Avoid rdeps when using hash all targets When using hash all targets it is overkill to use rdeps to query the impacted targets, since we know at the end of hashing what was affected. --- bazel-diff-example.sh | 17 ++----- src/main/java/com/bazel_diff/BazelClient.java | 11 +++-- .../com/bazel_diff/TargetHashingClient.java | 7 +-- src/main/java/com/bazel_diff/main.java | 5 +- .../TargetHashingClientImplTests.java | 46 +++++++++++++++++-- 5 files changed, 62 insertions(+), 24 deletions(-) diff --git a/bazel-diff-example.sh b/bazel-diff-example.sh index d8586f2..29139a5 100755 --- a/bazel-diff-example.sh +++ b/bazel-diff-example.sh @@ -9,7 +9,6 @@ previous_revision=$3 # Final Revision SHA final_revision=$4 -modified_filepaths_output="/tmp/modified_filepaths.txt" starting_hashes_json="/tmp/starting_hashes.json" final_hashes_json="/tmp/final_hashes.json" impacted_targets_path="/tmp/impacted_targets.txt" @@ -23,29 +22,21 @@ shared_flags="" $bazel_path run :bazel-diff $shared_flags --script_path="$bazel_diff" -$bazel_diff modified-filepaths $previous_revision $final_revision -w $workspace_path -b $bazel_path $modified_filepaths_output - -IFS=$'\n' read -d '' -r -a modified_filepaths < $modified_filepaths_output -formatted_filepaths=$(IFS=$'\n'; echo "${modified_filepaths[*]}") -echo "Modified Filepaths:" -echo $formatted_filepaths -echo "" - git -C $workspace_path checkout $previous_revision --quiet echo "Generating Hashes for Revision '$previous_revision'" -$bazel_diff generate-hashes -w $workspace_path -b $bazel_path $starting_hashes_json +$bazel_diff generate-hashes -w $workspace_path -b $bazel_path $starting_hashes_json -a git -C $workspace_path checkout $final_revision --quiet echo "Generating Hashes for Revision '$final_revision'" -$bazel_diff generate-hashes -w $workspace_path -b $bazel_path -m $modified_filepaths_output $final_hashes_json +$bazel_diff generate-hashes -w $workspace_path -b $bazel_path $final_hashes_json -a echo "Determining Impacted Targets" -$bazel_diff -sh $starting_hashes_json -fh $final_hashes_json -w $workspace_path -b $bazel_path -o $impacted_targets_path +$bazel_diff -sh $starting_hashes_json -fh $final_hashes_json -w $workspace_path -b $bazel_path -o $impacted_targets_path -a echo "Determining Impacted Test Targets" -$bazel_diff -sh $starting_hashes_json -fh $final_hashes_json -w $workspace_path -b $bazel_path -o $impacted_test_targets_path --avoid-query "//... except tests(//...)" +$bazel_diff -sh $starting_hashes_json -fh $final_hashes_json -w $workspace_path -b $bazel_path -o $impacted_test_targets_path -a --avoid-query "//... except tests(//...)" IFS=$'\n' read -d '' -r -a impacted_targets < $impacted_targets_path formatted_impacted_targets=$(IFS=$'\n'; echo "${impacted_targets[*]}") diff --git a/src/main/java/com/bazel_diff/BazelClient.java b/src/main/java/com/bazel_diff/BazelClient.java index 34028dc..80e658b 100644 --- a/src/main/java/com/bazel_diff/BazelClient.java +++ b/src/main/java/com/bazel_diff/BazelClient.java @@ -20,7 +20,7 @@ interface BazelClient { List queryAllTargets() throws IOException; - Set queryForImpactedTargets(Set impactedTargets, String avoidQuery) throws IOException; + Set queryForImpactedTargets(Set impactedTargets, String avoidQuery, Boolean hashAllTargets) throws IOException; Set convertFilepathsToSourceTargets(Set filepaths) throws IOException, NoSuchAlgorithmException; Set queryAllSourcefileTargets() throws IOException, NoSuchAlgorithmException; } @@ -47,12 +47,17 @@ public List queryAllTargets() throws IOException { } @Override - public Set queryForImpactedTargets(Set impactedTargets, String avoidQuery) throws IOException { + public Set queryForImpactedTargets(Set impactedTargets, String avoidQuery, Boolean hashAllTargets) throws IOException { Set impactedTargetNames = new HashSet<>(); String targetQuery = impactedTargets.stream() .map(target -> String.format("'%s'", target)) .collect(Collectors.joining(" + ")); - String query = String.format("rdeps(//... except '//external:all-targets', %s)", targetQuery); + String query = ""; + if (hashAllTargets) { + query = targetQuery; + } else { + query = String.format("rdeps(//... except '//external:all-targets', %s)", targetQuery); + } if (avoidQuery != null) { query = String.format("(%s) except (%s)", query, avoidQuery); } diff --git a/src/main/java/com/bazel_diff/TargetHashingClient.java b/src/main/java/com/bazel_diff/TargetHashingClient.java index 206c6ab..c5e7c27 100644 --- a/src/main/java/com/bazel_diff/TargetHashingClient.java +++ b/src/main/java/com/bazel_diff/TargetHashingClient.java @@ -10,7 +10,7 @@ interface TargetHashingClient { Map hashAllBazelTargets(Set modifiedFilepaths) throws IOException, NoSuchAlgorithmException; Map hashAllBazelTargetsAndSourcefiles() throws IOException, NoSuchAlgorithmException; - Set getImpactedTargets(Map startHashes, Map endHashes, String avoidQuery) throws IOException; + Set getImpactedTargets(Map startHashes, Map endHashes, String avoidQuery, Boolean hashAllTargets) throws IOException; } class TargetHashingClientImpl implements TargetHashingClient { @@ -36,7 +36,8 @@ public Map hashAllBazelTargetsAndSourcefiles() throws IOExceptio public Set getImpactedTargets( Map startHashes, Map endHashes, - String avoidQuery) + String avoidQuery, + Boolean hashAllTargets) throws IOException { Set impactedTargets = new HashSet<>(); for (Map.Entry entry : endHashes.entrySet()) { @@ -45,7 +46,7 @@ public Set getImpactedTargets( impactedTargets.add(entry.getKey()); } } - return bazelClient.queryForImpactedTargets(impactedTargets, avoidQuery); + return bazelClient.queryForImpactedTargets(impactedTargets, avoidQuery, hashAllTargets); } private byte[] createDigestForTarget( diff --git a/src/main/java/com/bazel_diff/main.java b/src/main/java/com/bazel_diff/main.java index a4258d2..bd22e90 100644 --- a/src/main/java/com/bazel_diff/main.java +++ b/src/main/java/com/bazel_diff/main.java @@ -158,6 +158,9 @@ class BazelDiff implements Callable { @Option(names = {"-o", "--output"}, scope = ScopeType.LOCAL, description = "Filepath to write the impacted Bazel targets to, newline separated") File outputPath; + @Option(names = {"-a", "--all-sourcefiles"}, description = "Experimental: Hash all sourcefile targets (instead of relying on --modifiedFilepaths), Warning: Performance may degrade from reading all source files") + Boolean hashAllSourcefiles; + @Option(names = {"-aq", "--avoid-query"}, scope = ScopeType.LOCAL, description = "A Bazel query string, any targets that pass this query will be removed from the returned set of targets") String avoidQuery; @@ -207,7 +210,7 @@ public Integer call() throws IOException { Map gsonHash = new HashMap<>(); Map startingHashes = gson.fromJson(startingFileReader, gsonHash.getClass()); Map finalHashes = gson.fromJson(finalFileReader, gsonHash.getClass()); - Set impactedTargets = hashingClient.getImpactedTargets(startingHashes, finalHashes, avoidQuery); + Set impactedTargets = hashingClient.getImpactedTargets(startingHashes, finalHashes, avoidQuery, hashAllSourcefiles); try { FileWriter myWriter = new FileWriter(outputPath); myWriter.write(impactedTargets.stream().collect(Collectors.joining(System.lineSeparator()))); diff --git a/test/java/com/bazel_diff/TargetHashingClientImplTests.java b/test/java/com/bazel_diff/TargetHashingClientImplTests.java index ca66745..cf27619 100644 --- a/test/java/com/bazel_diff/TargetHashingClientImplTests.java +++ b/test/java/com/bazel_diff/TargetHashingClientImplTests.java @@ -118,7 +118,7 @@ public void hashAllBazelTargets_sourceTargets_modifiedSources() throws IOExcepti @Test public void getImpactedTargets() throws IOException { - when(bazelClientMock.queryForImpactedTargets(anySet(), anyObject())).thenReturn( + when(bazelClientMock.queryForImpactedTargets(anySet(), anyObject(), Matchers.eq(false))).thenReturn( new HashSet<>(Arrays.asList("rule1", "rule3")) ); TargetHashingClientImpl client = new TargetHashingClientImpl(bazelClientMock); @@ -129,7 +129,7 @@ public void getImpactedTargets() throws IOException { hash2.put("rule1", "differentrule1hash"); hash2.put("rule2", "rule2hash"); hash2.put("rule3", "rule3hash"); - Set impactedTargets = client.getImpactedTargets(hash1, hash2, null); + Set impactedTargets = client.getImpactedTargets(hash1, hash2, null, false); Set expectedSet = new HashSet<>(); expectedSet.add("rule1"); expectedSet.add("rule3"); @@ -138,7 +138,7 @@ public void getImpactedTargets() throws IOException { @Test public void getImpactedTargets_withAvoidQuery() throws IOException { - when(bazelClientMock.queryForImpactedTargets(anySet(), eq("some_query"))).thenReturn( + when(bazelClientMock.queryForImpactedTargets(anySet(), eq("some_query"), Matchers.eq(false))).thenReturn( new HashSet<>(Arrays.asList("rule1")) ); TargetHashingClientImpl client = new TargetHashingClientImpl(bazelClientMock); @@ -149,7 +149,45 @@ public void getImpactedTargets_withAvoidQuery() throws IOException { hash2.put("rule1", "differentrule1hash"); hash2.put("rule2", "rule2hash"); hash2.put("rule3", "rule3hash"); - Set impactedTargets = client.getImpactedTargets(hash1, hash2, "some_query"); + Set impactedTargets = client.getImpactedTargets(hash1, hash2, "some_query", false); + Set expectedSet = new HashSet<>(); + expectedSet.add("rule1"); + assertEquals(expectedSet, impactedTargets); + } + + @Test + public void getImpactedTargets_withHashAllTargets() throws IOException { + when(bazelClientMock.queryForImpactedTargets(anySet(), anyObject(), Matchers.eq(true))).thenReturn( + new HashSet<>(Arrays.asList("rule1")) + ); + TargetHashingClientImpl client = new TargetHashingClientImpl(bazelClientMock); + Map hash1 = new HashMap<>(); + hash1.put("rule1", "rule1hash"); + hash1.put("rule2", "rule2hash"); + Map hash2 = new HashMap<>(); + hash2.put("rule1", "differentrule1hash"); + hash2.put("rule2", "rule2hash"); + hash2.put("rule3", "rule3hash"); + Set impactedTargets = client.getImpactedTargets(hash1, hash2, null, true); + Set expectedSet = new HashSet<>(); + expectedSet.add("rule1"); + assertEquals(expectedSet, impactedTargets); + } + + @Test + public void getImpactedTargets_withHashAllTargets_withAvoidQuery() throws IOException { + when(bazelClientMock.queryForImpactedTargets(anySet(), eq("some_query"), Matchers.eq(true))).thenReturn( + new HashSet<>(Arrays.asList("rule1")) + ); + TargetHashingClientImpl client = new TargetHashingClientImpl(bazelClientMock); + Map hash1 = new HashMap<>(); + hash1.put("rule1", "rule1hash"); + hash1.put("rule2", "rule2hash"); + Map hash2 = new HashMap<>(); + hash2.put("rule1", "differentrule1hash"); + hash2.put("rule2", "rule2hash"); + hash2.put("rule3", "rule3hash"); + Set impactedTargets = client.getImpactedTargets(hash1, hash2, "some_query", true); Set expectedSet = new HashSet<>(); expectedSet.add("rule1"); assertEquals(expectedSet, impactedTargets); From bf6d2b20b1f468250ed3c742622c38db5cc63bbb Mon Sep 17 00:00:00 2001 From: Maxwell Elliott Date: Tue, 18 May 2021 17:26:47 -0700 Subject: [PATCH 2/3] fix for potential null pointer --- src/main/java/com/bazel_diff/BazelClient.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/bazel_diff/BazelClient.java b/src/main/java/com/bazel_diff/BazelClient.java index 80e658b..1f713c4 100644 --- a/src/main/java/com/bazel_diff/BazelClient.java +++ b/src/main/java/com/bazel_diff/BazelClient.java @@ -53,7 +53,7 @@ public Set queryForImpactedTargets(Set impactedTargets, String a .map(target -> String.format("'%s'", target)) .collect(Collectors.joining(" + ")); String query = ""; - if (hashAllTargets) { + if (hashAllTargets != null && hashAllTargets) { query = targetQuery; } else { query = String.format("rdeps(//... except '//external:all-targets', %s)", targetQuery); From 4e12513a4f85a51692a805bd14f66765a249f42a Mon Sep 17 00:00:00 2001 From: Maxwell Elliott Date: Wed, 19 May 2021 08:45:55 -0700 Subject: [PATCH 3/3] address feedback --- src/main/java/com/bazel_diff/BazelClient.java | 11 +++-------- src/main/java/com/bazel_diff/TargetHashingClient.java | 5 ++++- .../com/bazel_diff/TargetHashingClientImplTests.java | 9 +++++---- 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/src/main/java/com/bazel_diff/BazelClient.java b/src/main/java/com/bazel_diff/BazelClient.java index 1f713c4..5f62893 100644 --- a/src/main/java/com/bazel_diff/BazelClient.java +++ b/src/main/java/com/bazel_diff/BazelClient.java @@ -20,7 +20,7 @@ interface BazelClient { List queryAllTargets() throws IOException; - Set queryForImpactedTargets(Set impactedTargets, String avoidQuery, Boolean hashAllTargets) throws IOException; + Set queryForImpactedTargets(Set impactedTargets, String avoidQuery) throws IOException; Set convertFilepathsToSourceTargets(Set filepaths) throws IOException, NoSuchAlgorithmException; Set queryAllSourcefileTargets() throws IOException, NoSuchAlgorithmException; } @@ -47,17 +47,12 @@ public List queryAllTargets() throws IOException { } @Override - public Set queryForImpactedTargets(Set impactedTargets, String avoidQuery, Boolean hashAllTargets) throws IOException { + public Set queryForImpactedTargets(Set impactedTargets, String avoidQuery) throws IOException { Set impactedTargetNames = new HashSet<>(); String targetQuery = impactedTargets.stream() .map(target -> String.format("'%s'", target)) .collect(Collectors.joining(" + ")); - String query = ""; - if (hashAllTargets != null && hashAllTargets) { - query = targetQuery; - } else { - query = String.format("rdeps(//... except '//external:all-targets', %s)", targetQuery); - } + String query = query = String.format("rdeps(//... except '//external:all-targets', %s)", targetQuery); if (avoidQuery != null) { query = String.format("(%s) except (%s)", query, avoidQuery); } diff --git a/src/main/java/com/bazel_diff/TargetHashingClient.java b/src/main/java/com/bazel_diff/TargetHashingClient.java index c5e7c27..33d708f 100644 --- a/src/main/java/com/bazel_diff/TargetHashingClient.java +++ b/src/main/java/com/bazel_diff/TargetHashingClient.java @@ -46,7 +46,10 @@ public Set getImpactedTargets( impactedTargets.add(entry.getKey()); } } - return bazelClient.queryForImpactedTargets(impactedTargets, avoidQuery, hashAllTargets); + if (hashAllTargets != null && hashAllTargets && avoidQuery == null) { + return impactedTargets; + } + return bazelClient.queryForImpactedTargets(impactedTargets, avoidQuery); } private byte[] createDigestForTarget( diff --git a/test/java/com/bazel_diff/TargetHashingClientImplTests.java b/test/java/com/bazel_diff/TargetHashingClientImplTests.java index cf27619..19ad28c 100644 --- a/test/java/com/bazel_diff/TargetHashingClientImplTests.java +++ b/test/java/com/bazel_diff/TargetHashingClientImplTests.java @@ -118,7 +118,7 @@ public void hashAllBazelTargets_sourceTargets_modifiedSources() throws IOExcepti @Test public void getImpactedTargets() throws IOException { - when(bazelClientMock.queryForImpactedTargets(anySet(), anyObject(), Matchers.eq(false))).thenReturn( + when(bazelClientMock.queryForImpactedTargets(anySet(), anyObject())).thenReturn( new HashSet<>(Arrays.asList("rule1", "rule3")) ); TargetHashingClientImpl client = new TargetHashingClientImpl(bazelClientMock); @@ -138,7 +138,7 @@ public void getImpactedTargets() throws IOException { @Test public void getImpactedTargets_withAvoidQuery() throws IOException { - when(bazelClientMock.queryForImpactedTargets(anySet(), eq("some_query"), Matchers.eq(false))).thenReturn( + when(bazelClientMock.queryForImpactedTargets(anySet(), eq("some_query"))).thenReturn( new HashSet<>(Arrays.asList("rule1")) ); TargetHashingClientImpl client = new TargetHashingClientImpl(bazelClientMock); @@ -157,7 +157,7 @@ public void getImpactedTargets_withAvoidQuery() throws IOException { @Test public void getImpactedTargets_withHashAllTargets() throws IOException { - when(bazelClientMock.queryForImpactedTargets(anySet(), anyObject(), Matchers.eq(true))).thenReturn( + when(bazelClientMock.queryForImpactedTargets(anySet(), anyObject())).thenReturn( new HashSet<>(Arrays.asList("rule1")) ); TargetHashingClientImpl client = new TargetHashingClientImpl(bazelClientMock); @@ -171,12 +171,13 @@ public void getImpactedTargets_withHashAllTargets() throws IOException { Set impactedTargets = client.getImpactedTargets(hash1, hash2, null, true); Set expectedSet = new HashSet<>(); expectedSet.add("rule1"); + expectedSet.add("rule3"); assertEquals(expectedSet, impactedTargets); } @Test public void getImpactedTargets_withHashAllTargets_withAvoidQuery() throws IOException { - when(bazelClientMock.queryForImpactedTargets(anySet(), eq("some_query"), Matchers.eq(true))).thenReturn( + when(bazelClientMock.queryForImpactedTargets(anySet(), eq("some_query"))).thenReturn( new HashSet<>(Arrays.asList("rule1")) ); TargetHashingClientImpl client = new TargetHashingClientImpl(bazelClientMock);