Include the transitive deps when computing the ABI key of a dep with export_deps=True. Summary: When computing the ABI key of a dependency with export_deps=True, the ABI keys for the transitive dependencies will be hashed in the final ABI key. Test Plan: Added new test: DefaultJavaLibraryRuleTest.testGetAbiKeyInThePresenceOfExportDeps().
diff --git a/src/com/facebook/buck/java/DefaultJavaLibraryRule.java b/src/com/facebook/buck/java/DefaultJavaLibraryRule.java index 5e87343..1e45507 100644 --- a/src/com/facebook/buck/java/DefaultJavaLibraryRule.java +++ b/src/com/facebook/buck/java/DefaultJavaLibraryRule.java
@@ -309,15 +309,40 @@ setAbiKey(Suppliers.memoize(new Supplier<Sha1HashCode>() { @Override public Sha1HashCode get() { - return javac.getAbiKey(); + return createTotalAbiKey(javac.getAbiKey()); } })); } else { // When there are no .java files to compile, the ABI key should be a constant. - setAbiKey(Suppliers.ofInstance(new Sha1HashCode(AbiWriterProtocol.EMPTY_ABI_KEY))); + setAbiKey(Suppliers.ofInstance(createTotalAbiKey( + new Sha1HashCode(AbiWriterProtocol.EMPTY_ABI_KEY)))); } } + /** + * Creates the total ABI key for this rule. If export_deps is true, the total key is computed by + * hashing the ABI keys of the dependencies together with the ABI key of this rule. If export_deps + * is false, the standalone ABI key for this rule is used as the total key. + * @param abiKey the standalone ABI key for this rule. + * @return total ABI key containing also the ABI keys of the dependencies. + */ + protected Sha1HashCode createTotalAbiKey(Sha1HashCode abiKey) { + if (!getExportDeps()) { + return abiKey; + } + + SortedSet<JavaLibraryRule> depsForAbiKey = getDepsForAbiKey(); + // If there are no deps to consider, just return the ABI Key for this rule. + if (depsForAbiKey.isEmpty()) { + return abiKey; + } + + // Hash the ABI keys of all dependencies together with ABI key for the current rule. + Hasher hasher = createHasherWithAbiKeyForDeps(depsForAbiKey); + hasher.putUnencodedChars(abiKey.getHash()); + return new Sha1HashCode(hasher.hash().toString()); + } + private String getPathToAbiOutputDir() { BuildTarget target = getBuildTarget(); return String.format( @@ -359,11 +384,18 @@ } /** - * Finds all deps that implement JavaLibraryRule and hash their ABI keys together. If any dep - * lacks an ABI key, then returns {@link Optional#absent()}. + * Finds all deps that implement JavaLibraryRule and hash their ABI keys together. */ @Override public Sha1HashCode getAbiKeyForDeps() { + return new Sha1HashCode(createHasherWithAbiKeyForDeps(getDepsForAbiKey()).hash().toString()); + } + + /** + * Returns a sorted set containing the dependencies which will be hashed in the final ABI key. + * @return the dependencies to be hashed in the final ABI key. + */ + private SortedSet<JavaLibraryRule> getDepsForAbiKey() { SortedSet<JavaLibraryRule> rulesWithAbiToConsider = Sets.newTreeSet(); for (BuildRule dep : getDeps()) { if (dep instanceof JavaLibraryRule) { @@ -371,7 +403,16 @@ rulesWithAbiToConsider.addAll(javaRule.getOutputClasspathEntries().keys()); } } + return rulesWithAbiToConsider; + } + /** + * Creates a Hasher containing the ABI keys of the dependencies. + * @param rulesWithAbiToConsider a sorted set containing the dependencies whose ABI key will be + * added to the hasher. + * @return a Hasher containing the ABI keys of the dependencies. + */ + private Hasher createHasherWithAbiKeyForDeps(SortedSet<JavaLibraryRule> rulesWithAbiToConsider) { Hasher hasher = Hashing.sha1().newHasher(); for (JavaLibraryRule ruleWithAbiToConsider : rulesWithAbiToConsider) { if (ruleWithAbiToConsider == this) { @@ -382,7 +423,7 @@ hasher.putUnencodedChars(abiKey.getHash()); } - return new Sha1HashCode(hasher.hash().toString()); + return hasher; } @Override
diff --git a/test/com/facebook/buck/java/DefaultJavaLibraryRuleTest.java b/test/com/facebook/buck/java/DefaultJavaLibraryRuleTest.java index e2414fe..ec2dee2 100644 --- a/test/com/facebook/buck/java/DefaultJavaLibraryRuleTest.java +++ b/test/com/facebook/buck/java/DefaultJavaLibraryRuleTest.java
@@ -683,7 +683,7 @@ consumerNoExport.getAbiKeyForDeps(), not(equalTo(consumerWithExport.getAbiKeyForDeps()))); String expectedAbiKeyNoDepsHashForConsumerWithExport = Hashing.sha1().newHasher() - .putUnencodedChars(commonWithExportAbiKeyHash) + .putUnencodedChars(commonWithExport.getAbiKey().getHash()) .putUnencodedChars(tinyLibAbiKeyHash) .hash() .toString(); @@ -695,8 +695,201 @@ observedAbiKeyNoDepsHashForConsumerWithExport); } + /** + * @see DefaultJavaLibraryRule#getAbiKey() + */ + @Test + public void testGetAbiKeyInThePresenceOfExportDeps() throws IOException { + // Create a java_library named //:commonlib with a hardcoded ABI key. + String commonLibAbiKeyHash = Strings.repeat("a", 40); + JavaLibraryRule commonLibrary = createDefaultJavaLibaryRuleWithAbiKey( + commonLibAbiKeyHash, + BuildTargetFactory.newInstance("//:commonlib"), + // Must have a source file or else its ABI will be AbiWriterProtocol.EMPTY_ABI_KEY. + /* srcs */ ImmutableSet.of("foo/Bar.java"), + /* deps */ ImmutableSet.<BuildRule>of(), + /* exportDeps */ false); + + // Create two java_library rules, each of which depends on //:commonlib, but only one of which + // exports its deps. + String libWithExportAbiKeyHash = Strings.repeat("b", 40); + DefaultJavaLibraryRule libWithExport = createDefaultJavaLibaryRuleWithAbiKey( + libWithExportAbiKeyHash, + BuildTargetFactory.newInstance("//:lib_with_export"), + /* srcs */ ImmutableSet.<String>of(), + /* deps */ ImmutableSet.<BuildRule>of(commonLibrary), + /* exportDeps */ true); + String libNoExportAbiKeyHash = Strings.repeat("c", 40); + DefaultJavaLibraryRule libNoExport = createDefaultJavaLibaryRuleWithAbiKey( + /* abiHash */ libNoExportAbiKeyHash, + BuildTargetFactory.newInstance("//:lib_no_export"), + /* srcs */ ImmutableSet.<String>of(), + /* deps */ ImmutableSet.<BuildRule>of(commonLibrary), + /* exportDeps */ false); + + // Verify getAbiKey() for the two //:lib_XXX rules. + String expectedLibWithExportAbiKeyHash = Hashing.sha1().newHasher() + .putUnencodedChars(commonLibAbiKeyHash) + .putUnencodedChars(libWithExportAbiKeyHash) + .hash() + .toString(); + assertEquals( + "getAbiKey() should include the dependencies' ABI keys for the rule with export_deps=true.", + expectedLibWithExportAbiKeyHash, + libWithExport.getAbiKey().getHash()); + + assertEquals( + "getAbiKey() should not include the dependencies' ABI keys for the rule with export_deps=false.", + libNoExportAbiKeyHash, + libNoExport.getAbiKey().getHash()); + } + + /** + * @see DefaultJavaLibraryRule#getAbiKey() + */ + @Test + public void testGetAbiKeyRecursiveExportDeps() throws IOException { + String libAAbiKeyHash = Strings.repeat("a", 40); + String libBAbiKeyHash = Strings.repeat("b", 40); + String libCAbiKeyHash = Strings.repeat("c", 40); + String libDAbiKeyHash = Strings.repeat("d", 40); + + // Test the following dependency graph: + // a(export_deps=true) -> b(export_deps=true) -> c(export_deps=true) -> d(export_deps=true) + JavaLibraryRule libD = createDefaultJavaLibaryRuleWithAbiKey( + libDAbiKeyHash, + BuildTargetFactory.newInstance("//:lib_d"), + // Must have a source file or else its ABI will be AbiWriterProtocol.EMPTY_ABI_KEY. + /* srcs */ ImmutableSet.of("foo/Bar.java"), + /* deps */ ImmutableSet.<BuildRule>of(), + /* exportDeps */ true); + JavaLibraryRule libC = createDefaultJavaLibaryRuleWithAbiKey( + libCAbiKeyHash, + BuildTargetFactory.newInstance("//:lib_c"), + // Must have a source file or else its ABI will be AbiWriterProtocol.EMPTY_ABI_KEY. + /* srcs */ ImmutableSet.of("foo/Bar.java"), + /* deps */ ImmutableSet.<BuildRule>of(libD), + /* exportDeps */ true); + JavaLibraryRule libB = createDefaultJavaLibaryRuleWithAbiKey( + libBAbiKeyHash, + BuildTargetFactory.newInstance("//:lib_b"), + // Must have a source file or else its ABI will be AbiWriterProtocol.EMPTY_ABI_KEY. + /* srcs */ ImmutableSet.of("foo/Bar.java"), + /* deps */ ImmutableSet.<BuildRule>of(libC), + /* exportDeps */ true); + JavaLibraryRule libA = createDefaultJavaLibaryRuleWithAbiKey( + libAAbiKeyHash, + BuildTargetFactory.newInstance("//:lib_a"), + // Must have a source file or else its ABI will be AbiWriterProtocol.EMPTY_ABI_KEY. + /* srcs */ ImmutableSet.of("foo/Bar.java"), + /* deps */ ImmutableSet.<BuildRule>of(libB), + /* exportDeps */ true); + + assertEquals( + "If a rule has no dependencies its final ABI key should be the rule's own ABI key.", + libDAbiKeyHash, + libD.getAbiKey().getHash()); + + String expectedLibCAbiKeyHash = Hashing.sha1().newHasher() + .putUnencodedChars(libDAbiKeyHash) + .putUnencodedChars(libCAbiKeyHash) + .hash() + .toString(); + assertEquals( + "The ABI key for lib_c should contain lib_d's ABI key.", + expectedLibCAbiKeyHash, + libC.getAbiKey().getHash()); + + String expectedLibBAbiKeyHash = Hashing.sha1().newHasher() + .putUnencodedChars(expectedLibCAbiKeyHash) + .putUnencodedChars(libDAbiKeyHash) + .putUnencodedChars(libBAbiKeyHash) + .hash() + .toString(); + assertEquals( + "The ABI key for lib_b should contain lib_c's and lib_d's ABI keys.", + expectedLibBAbiKeyHash, + libB.getAbiKey().getHash()); + + String expectedLibAAbiKeyHash = Hashing.sha1().newHasher() + .putUnencodedChars(expectedLibBAbiKeyHash) + .putUnencodedChars(expectedLibCAbiKeyHash) + .putUnencodedChars(libDAbiKeyHash) + .putUnencodedChars(libAAbiKeyHash) + .hash() + .toString(); + assertEquals( + "The ABI key for lib_a should contain lib_b's, lib_c's and lib_d's ABI keys.", + expectedLibAAbiKeyHash, + libA.getAbiKey().getHash()); + + // Test the following dependency graph: + // a(export_deps=true) -> b(export_deps=true) -> c(export_deps=false) -> d(export_deps=false) + libD = createDefaultJavaLibaryRuleWithAbiKey( + libDAbiKeyHash, + BuildTargetFactory.newInstance("//:lib_d2"), + // Must have a source file or else its ABI will be AbiWriterProtocol.EMPTY_ABI_KEY. + /* srcs */ ImmutableSet.of("foo/Bar.java"), + /* deps */ ImmutableSet.<BuildRule>of(), + /* exportDeps */ false); + libC = createDefaultJavaLibaryRuleWithAbiKey( + libCAbiKeyHash, + BuildTargetFactory.newInstance("//:lib_c2"), + // Must have a source file or else its ABI will be AbiWriterProtocol.EMPTY_ABI_KEY. + /* srcs */ ImmutableSet.of("foo/Bar.java"), + /* deps */ ImmutableSet.<BuildRule>of(libD), + /* exportDeps */ false); + libB = createDefaultJavaLibaryRuleWithAbiKey( + libBAbiKeyHash, + BuildTargetFactory.newInstance("//:lib_b2"), + // Must have a source file or else its ABI will be AbiWriterProtocol.EMPTY_ABI_KEY. + /* srcs */ ImmutableSet.of("foo/Bar.java"), + /* deps */ ImmutableSet.<BuildRule>of(libC), + /* exportDeps */ true); + libA = createDefaultJavaLibaryRuleWithAbiKey( + libAAbiKeyHash, + BuildTargetFactory.newInstance("//:lib_a2"), + // Must have a source file or else its ABI will be AbiWriterProtocol.EMPTY_ABI_KEY. + /* srcs */ ImmutableSet.of("foo/Bar.java"), + /* deps */ ImmutableSet.<BuildRule>of(libB), + /* exportDeps */ true); + + assertEquals( + "If export_deps is false, the final ABI key should be the rule's own ABI key.", + libDAbiKeyHash, + libD.getAbiKey().getHash()); + + assertEquals( + "If export_deps is false, the final ABI key should be the rule's own ABI key.", + libCAbiKeyHash, + libC.getAbiKey().getHash()); + + expectedLibBAbiKeyHash = Hashing.sha1().newHasher() + .putUnencodedChars(libCAbiKeyHash) + .putUnencodedChars(libBAbiKeyHash) + .hash() + .toString(); + assertEquals( + "The ABI key for lib_b should contain lib_c's ABI key.", + expectedLibBAbiKeyHash, + libB.getAbiKey().getHash()); + + expectedLibAAbiKeyHash = Hashing.sha1().newHasher() + .putUnencodedChars(expectedLibBAbiKeyHash) + .putUnencodedChars(libCAbiKeyHash) + .putUnencodedChars(libDAbiKeyHash) + .putUnencodedChars(libAAbiKeyHash) + .hash() + .toString(); + assertEquals( + "The ABI key for lib_a should contain lib_b's, lib_c's and lib_d's ABI keys.", + expectedLibAAbiKeyHash, + libA.getAbiKey().getHash()); + + } + private static DefaultJavaLibraryRule createDefaultJavaLibaryRuleWithAbiKey( - @Nullable final String abiHash, + @Nullable final String partialAbiHash, BuildTarget buildTarget, ImmutableSet<String> srcs, ImmutableSet<BuildRule> deps, @@ -711,10 +904,10 @@ ) { @Override public Sha1HashCode getAbiKey() { - if (abiHash == null) { + if (partialAbiHash == null) { return super.getAbiKey(); } else { - return new Sha1HashCode(abiHash); + return createTotalAbiKey(new Sha1HashCode(partialAbiHash)); } } };