Fix a cache corruption bug caused by including a rule's output on its classpath.
Summary:
Apparently, a `java_library` was including its output JAR on the classpath when
building itself. This means that removing `srcs` from a `java_library` and then rebuilding
the rule would still succeed if the output JAR was still around from the previous build.
This would explain some rebuild issues that we have received anecdotal reports of,
but have not been able to reproduce.
This diff solves the bug by fixing `DefaultJavaLibraryRule.getDeclaredClasspathEntries()`
so that its return value matches that which is required by its Javadoc:
```
/**
* @return The set of entries to pass to {@code javac}'s {@code -classpath} flag in order to
* compile the {@code srcs} associated with this rule. This set only contains the classpath
* entries for those rules that are declared as direct dependencies of this rule.
*/
public ImmutableSetMultimap<JavaLibraryRule, String> getDeclaredClasspathEntries();
```
Test Plan:
Added an integration test to make sure changing the set of files in a java_library's srcs
causes a rebuild.
Added a unit test to make sure the `-classpath` argument passed to `javac` does not include
the output JAR of the `java_library` rule being built.
diff --git a/src/com/facebook/buck/java/DefaultJavaLibraryRule.java b/src/com/facebook/buck/java/DefaultJavaLibraryRule.java
index cdd64ab..5e87343 100644
--- a/src/com/facebook/buck/java/DefaultJavaLibraryRule.java
+++ b/src/com/facebook/buck/java/DefaultJavaLibraryRule.java
@@ -260,8 +260,7 @@
final ImmutableSetMultimap.Builder<JavaLibraryRule, String> classpathEntries =
ImmutableSetMultimap.builder();
- Iterable<JavaLibraryRule> javaLibraryDeps = Iterables.filter(
- Sets.union(getDeps(), ImmutableSet.of(DefaultJavaLibraryRule.this)),
+ Iterable<JavaLibraryRule> javaLibraryDeps = Iterables.filter(getDeps(),
JavaLibraryRule.class);
for (JavaLibraryRule rule : javaLibraryDeps) {
diff --git a/src/com/facebook/buck/java/PrebuiltJarRule.java b/src/com/facebook/buck/java/PrebuiltJarRule.java
index 385179b..616ed6a 100644
--- a/src/com/facebook/buck/java/PrebuiltJarRule.java
+++ b/src/com/facebook/buck/java/PrebuiltJarRule.java
@@ -38,6 +38,7 @@
import com.facebook.buck.step.AbstractExecutionStep;
import com.facebook.buck.step.ExecutionContext;
import com.facebook.buck.step.Step;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Optional;
import com.google.common.base.Preconditions;
import com.google.common.base.Supplier;
@@ -150,6 +151,11 @@
return abiKey;
}
+ @VisibleForTesting
+ void setAbiKey(Sha1HashCode abiKey) {
+ this.abiKey = Preconditions.checkNotNull(abiKey);
+ }
+
@Override
public void initializeFromDisk(OnDiskBuildInfo onDiskBuildInfo) {
Optional<Sha1HashCode> abiKeyHash = onDiskBuildInfo.getHash(AbiRule.ABI_KEY_ON_DISK_METADATA);
diff --git a/test/com/facebook/buck/java/CleanClasspathIntegrationTest.java b/test/com/facebook/buck/java/CleanClasspathIntegrationTest.java
new file mode 100644
index 0000000..88f11fa
--- /dev/null
+++ b/test/com/facebook/buck/java/CleanClasspathIntegrationTest.java
@@ -0,0 +1,72 @@
+/*
+ * Copyright 2013-present Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License. You may obtain
+ * a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.facebook.buck.java;
+
+import static org.junit.Assert.assertTrue;
+
+import com.facebook.buck.testutil.integration.DebuggableTemporaryFolder;
+import com.facebook.buck.testutil.integration.ProjectWorkspace;
+import com.facebook.buck.testutil.integration.ProjectWorkspace.ProcessResult;
+import com.facebook.buck.testutil.integration.TestDataHelper;
+import com.google.common.base.Charsets;
+import com.google.common.base.Joiner;
+import com.google.common.io.Files;
+
+import org.junit.Rule;
+import org.junit.Test;
+
+import java.io.File;
+import java.io.IOException;
+
+/**
+ * Integration test to verify that when a {@code java_library} rule is built, the classpath that is
+ * used to build it does not contain any leftover artifacts from the previous build.
+ */
+public class CleanClasspathIntegrationTest {
+
+ @Rule
+ public DebuggableTemporaryFolder tmp = new DebuggableTemporaryFolder();
+
+ @Test
+ public void testJavaLibraryRuleDoesNotIncludeItsOwnOldOutputOnTheClasspath() throws IOException {
+ ProjectWorkspace workspace = TestDataHelper.createProjectWorkspaceForScenario(
+ this, "classpath_corruption_regression", tmp);
+ workspace.setUp();
+
+ // Build //:example so that content is written to buck-out/gen/.
+ ProcessResult processResult1 = workspace.runBuckCommand("build", "//:example");
+ processResult1.assertExitCode(0);
+ assertTrue(
+ "example.jar should be written. This should not be on the classpath on the next build.",
+ workspace.getFile("buck-out/gen/lib__example__output/example.jar").isFile());
+
+ // Overwrite the existing BUCK file, redefining the java_library rule to exclude Bar.java from
+ // its srcs.
+ File buildFile = workspace.getFile("BUCK");
+ String newBuildFileContents = Joiner.on('\n').join(
+ "java_library(",
+ " name = 'example',",
+ " srcs = [ 'Foo.java' ], ",
+ ")");
+ Files.write(newBuildFileContents, buildFile, Charsets.UTF_8);
+
+ // Rebuilding //:example should fail even though Bar.class is in
+ // buck-out/gen/lib__example__output/example.jar.
+ ProcessResult processResult2 = workspace.runBuckCommand("build", "//:example");
+ processResult2.assertExitCode("Build should fail because Foo.java depends on Bar.java.", 1);
+ }
+}
diff --git a/test/com/facebook/buck/java/DefaultJavaLibraryRuleTest.java b/test/com/facebook/buck/java/DefaultJavaLibraryRuleTest.java
index 8baffea..e2414fe 100644
--- a/test/com/facebook/buck/java/DefaultJavaLibraryRuleTest.java
+++ b/test/com/facebook/buck/java/DefaultJavaLibraryRuleTest.java
@@ -18,6 +18,7 @@
import static com.facebook.buck.util.BuckConstant.BIN_DIR;
import static org.easymock.EasyMock.createNiceMock;
+import static org.easymock.EasyMock.expect;
import static org.easymock.EasyMock.replay;
import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.not;
@@ -72,6 +73,7 @@
import com.google.common.base.Predicate;
import com.google.common.base.Splitter;
import com.google.common.base.Strings;
+import com.google.common.collect.FluentIterable;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
@@ -442,6 +444,57 @@
parent.getTransitiveClasspathEntries());
}
+ @Test
+ public void testClasspathForJavacCommand() throws IOException {
+ BuildRuleResolver ruleResolver = new BuildRuleResolver();
+
+ BuildTarget libraryOneTarget = BuildTargetFactory.newInstance("//:libone");
+ PrebuiltJarRule libraryOne = ruleResolver.buildAndAddToIndex(
+ PrebuiltJarRule.newPrebuiltJarRuleBuilder(new FakeAbstractBuildRuleBuilderParams())
+ .setBuildTarget(libraryOneTarget)
+ .setBinaryJar("java/src/com/libone/bar.jar"));
+
+ BuildTarget libraryTwoTarget = BuildTargetFactory.newInstance("//:libtwo");
+ JavaLibraryRule libraryTwo = ruleResolver.buildAndAddToIndex(
+ DefaultJavaLibraryRule.newJavaLibraryRuleBuilder(new FakeAbstractBuildRuleBuilderParams())
+ .setBuildTarget(libraryTwoTarget)
+ .addSrc("java/src/com/libtwo/Foo.java")
+ .addDep(BuildTargetFactory.newInstance("//:libone")));
+
+ DependencyGraph graph = RuleMap.createGraphFromBuildRules(ruleResolver);
+
+ BuildContext buildContext = EasyMock.createMock(BuildContext.class);
+ expect(buildContext.getDependencyGraph()).andReturn(graph);
+ expect(buildContext.getBuildDependencies()).andReturn(BuildDependencies.FIRST_ORDER_ONLY)
+ .times(2);
+ JavaPackageFinder javaPackageFinder = EasyMock.createMock(JavaPackageFinder.class);
+ expect(buildContext.getJavaPackageFinder()).andReturn(javaPackageFinder);
+
+ replay(buildContext, javaPackageFinder);
+
+ libraryOne.build(buildContext);
+ libraryOne.setAbiKey(new Sha1HashCode(Strings.repeat("cafebabe", 5)));
+ List<Step> steps = libraryTwo.getBuildSteps(buildContext, new FakeBuildableContext());
+
+ EasyMock.verify(buildContext, javaPackageFinder);
+
+ ImmutableList<JavacInMemoryStep> javacSteps = FluentIterable
+ .from(steps)
+ .filter(JavacInMemoryStep.class)
+ .toList();
+ assertEquals("There should be only one javac step.", 1, javacSteps.size());
+ JavacInMemoryStep javacStep = javacSteps.get(0);
+ assertEquals(
+ "The classpath to use when compiling //:libtwo according to getDeclaredClasspathEntries()" +
+ "should contain only bar.jar.",
+ ImmutableSet.of("java/src/com/libone/bar.jar"),
+ ImmutableSet.copyOf(libraryTwo.getDeclaredClasspathEntries().values()));
+ assertEquals(
+ "The classpath for the javac step to compile //:libtwo should contain only bar.jar.",
+ ImmutableSet.of("java/src/com/libone/bar.jar"),
+ javacStep.getClasspathEntries());
+ }
+
/**
* Verify adding an annotation processor java binary with options.
*/
@@ -515,12 +568,16 @@
.addDep(BuildTargetFactory.newInstance("//:libtwo")));
assertEquals(
+ "A java_library that depends on //:libone should include only libone.jar in its " +
+ "classpath when compiling itself.",
ImmutableSetMultimap.builder()
.put(libraryOne, "buck-out/gen/lib__libone__output/libone.jar")
.build(),
libraryOne.getOutputClasspathEntries());
assertEquals(
+ "//:libtwo exports its deps, so a java_library that depends on //:libtwo should include " +
+ "both libone.jar and libtwo.jar in its classpath when compiling itself.",
ImmutableSetMultimap.builder()
.put(libraryOne, "buck-out/gen/lib__libone__output/libone.jar")
.put(libraryTwo, "buck-out/gen/lib__libone__output/libone.jar")
@@ -528,13 +585,33 @@
.build(),
libraryTwo.getOutputClasspathEntries());
- ImmutableSetMultimap.Builder<BuildRule, String> expected = ImmutableSetMultimap.builder();
- expected.put(parent, "buck-out/gen/lib__parent__output/parent.jar");
- expected.putAll(libraryTwo,
- "buck-out/gen/lib__libone__output/libone.jar",
- "buck-out/gen/lib__libtwo__output/libtwo.jar");
+ assertEquals(
+ "//:libtwo exports its deps, so both libone.jar and libtwo.jar should be on the classpath" +
+ " when compiling //:parent.",
+ ImmutableSetMultimap.builder()
+ .put(libraryTwo, "buck-out/gen/lib__libone__output/libone.jar")
+ .put(libraryTwo, "buck-out/gen/lib__libtwo__output/libtwo.jar")
+ .build(),
+ parent.getDeclaredClasspathEntries());
- assertEquals(expected.build(), parent.getDeclaredClasspathEntries());
+ assertEquals(
+ "A java_binary that depends on //:parent should include libone.jar, libtwo.jar and " +
+ "parent.jar.",
+ ImmutableSetMultimap.builder()
+ .put(libraryOne, "buck-out/gen/lib__libone__output/libone.jar")
+ .put(libraryTwo, "buck-out/gen/lib__libone__output/libone.jar")
+ .put(libraryTwo, "buck-out/gen/lib__libtwo__output/libtwo.jar")
+ .put(parent, "buck-out/gen/lib__parent__output/parent.jar")
+ .build(),
+ parent.getTransitiveClasspathEntries());
+
+ assertEquals(
+ "A java_library that depends on //:parent should include only parent.jar in its " +
+ "-classpath when compiling itself.",
+ ImmutableSetMultimap.builder()
+ .put(parent, "buck-out/gen/lib__parent__output/parent.jar")
+ .build(),
+ parent.getOutputClasspathEntries());
}
/**
@@ -766,9 +843,9 @@
DependencyGraph graph = RuleMap.createGraphFromBuildRules(ruleResolver);
BuildContext context = EasyMock.createMock(BuildContext.class);
- EasyMock.expect(context.getDependencyGraph()).andReturn(graph).anyTimes();
+ expect(context.getDependencyGraph()).andReturn(graph).anyTimes();
- EasyMock.expect(context.getBuildDependencies()).andReturn(buildDependencies).anyTimes();
+ expect(context.getBuildDependencies()).andReturn(buildDependencies).anyTimes();
replay(context);
@@ -783,7 +860,7 @@
ImmutableList<Path> bootclasspathEntries = (bootclasspath == null)
? ImmutableList.<Path>of(Paths.get("I am not used"))
: ImmutableList.of(Paths.get(bootclasspath));
- EasyMock.expect(platformTarget.getBootclasspathEntries()).andReturn(bootclasspathEntries)
+ expect(platformTarget.getBootclasspathEntries()).andReturn(bootclasspathEntries)
.anyTimes();
replay(platformTarget);
diff --git a/test/com/facebook/buck/java/testdata/classpath_corruption_regression/BUCK b/test/com/facebook/buck/java/testdata/classpath_corruption_regression/BUCK
new file mode 100644
index 0000000..2f71c07
--- /dev/null
+++ b/test/com/facebook/buck/java/testdata/classpath_corruption_regression/BUCK
@@ -0,0 +1,7 @@
+java_library(
+ name = 'example',
+ srcs = [
+ 'Bar.java',
+ 'Foo.java',
+ ],
+)
diff --git a/test/com/facebook/buck/java/testdata/classpath_corruption_regression/Bar.java b/test/com/facebook/buck/java/testdata/classpath_corruption_regression/Bar.java
new file mode 100644
index 0000000..51142c5
--- /dev/null
+++ b/test/com/facebook/buck/java/testdata/classpath_corruption_regression/Bar.java
@@ -0,0 +1,22 @@
+/*
+ * Copyright 2013-present Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License. You may obtain
+ * a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.example;
+
+public class Bar {
+ @SuppressWarnings("unused")
+ private static final Foo foo = new Foo();
+}
diff --git a/test/com/facebook/buck/java/testdata/classpath_corruption_regression/Foo.java b/test/com/facebook/buck/java/testdata/classpath_corruption_regression/Foo.java
new file mode 100644
index 0000000..2bf22b8
--- /dev/null
+++ b/test/com/facebook/buck/java/testdata/classpath_corruption_regression/Foo.java
@@ -0,0 +1,22 @@
+/*
+ * Copyright 2013-present Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License. You may obtain
+ * a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.example;
+
+public class Foo {
+ @SuppressWarnings("unused")
+ private static final Bar bar = new Bar();
+}
