Fix WARN_ON_TRANSITIVE.
Summary:
The WARN_ON_TRANSITIVE flag seems to have been broken for a while.
Fix it and add tests.
Test Plan: tests
diff --git a/build.xml b/build.xml
index 1ca91f3..aa76119 100644
--- a/build.xml
+++ b/build.xml
@@ -243,6 +243,7 @@
</fileset>
<fileset dir="${test.dir}">
<include name="**/*.java"/>
+ <exclude name="**/testdata/**"/>
</fileset>
<fileset dir="${plugin.dir}">
<include name="**/*.java"/>
diff --git a/src/com/facebook/buck/java/DefaultJavaLibraryRule.java b/src/com/facebook/buck/java/DefaultJavaLibraryRule.java
index 1e45507..ab560be 100644
--- a/src/com/facebook/buck/java/DefaultJavaLibraryRule.java
+++ b/src/com/facebook/buck/java/DefaultJavaLibraryRule.java
@@ -53,8 +53,8 @@
import com.facebook.buck.step.fs.MkdirAndSymlinkFileStep;
import com.facebook.buck.util.BuckConstant;
import com.facebook.buck.util.MorePaths;
+import com.facebook.buck.util.ProjectFilesystem;
import com.google.common.annotations.VisibleForTesting;
-import com.google.common.base.Function;
import com.google.common.base.Optional;
import com.google.common.base.Preconditions;
import com.google.common.base.Predicate;
@@ -145,16 +145,19 @@
* jar.
*/
@VisibleForTesting
- static interface JarResolver extends Function<String, ImmutableSet<String>> {}
+ static interface JarResolver {
+ public ImmutableSet<String> resolve(ProjectFilesystem filesystem, Path relativeClassPath);
+ }
private final JarResolver JAR_RESOLVER =
new JarResolver() {
@Override
- public ImmutableSet<String> apply(String classPath) {
+ public ImmutableSet<String> resolve(ProjectFilesystem filesystem, Path relativeClassPath) {
ImmutableSet.Builder<String> topLevelSymbolsBuilder = ImmutableSet.builder();
try {
+ Path classPath = filesystem.getFileForRelativePath(relativeClassPath).toPath();
ClassLoader loader = URLClassLoader.newInstance(
- new URL[]{new File(classPath).toURI().toURL()},
+ new URL[]{classPath.toUri().toURL()},
/* parent */ null);
// For every class contained in that jar, check to see if the package name
@@ -621,7 +624,8 @@
* @return whether or not adding {@code transitiveNotDeclaredDep} as a dependency to this build
* rule would have satisfied one of the {@code failedImports}.
*/
- private boolean isMissingBuildRule(BuildRule transitiveNotDeclaredDep,
+ private boolean isMissingBuildRule(ProjectFilesystem filesystem,
+ BuildRule transitiveNotDeclaredDep,
Set<String> failedImports,
JarResolver jarResolver) {
if (!(transitiveNotDeclaredDep instanceof JavaLibraryRule)) {
@@ -635,8 +639,7 @@
// the exception of rules that export their dependencies, this will result in a single
// classpath.
for (String classPath : classPaths) {
- ImmutableSet<String> topLevelSymbols;
- topLevelSymbols = jarResolver.apply(classPath);
+ ImmutableSet<String> topLevelSymbols = jarResolver.resolve(filesystem, Paths.get(classPath));
for (String symbolName : topLevelSymbols) {
if (failedImports.contains(symbolName)) {
@@ -668,7 +671,7 @@
}
final Set<JavaLibraryRule> transitiveNotDeclaredDeps = Sets.difference(
transitiveClasspathEntries.keySet(),
- declaredClasspathEntries.keySet());
+ Sets.union(ImmutableSet.of(this), declaredClasspathEntries.keySet()));
final ImmutableList<BuildRule> sortedTransitiveNotDeclaredDeps = ImmutableList.copyOf(
TopologicalSort.sort(context.getDependencyGraph(),
@@ -682,7 +685,8 @@
JavacInMemoryStep.SuggestBuildRules suggestBuildRuleFn =
new JavacInMemoryStep.SuggestBuildRules() {
@Override
- public ImmutableSet<String> apply(ImmutableSet<String> failedImports) {
+ public ImmutableSet<String> suggest(ProjectFilesystem filesystem,
+ ImmutableSet<String> failedImports) {
ImmutableSet.Builder<String> suggestedDeps = ImmutableSet.builder();
Set<String> remainingImports = Sets.newHashSet(failedImports);
@@ -691,7 +695,10 @@
boolean ruleCanSeeDep = transitiveNotDeclaredDep.isVisibleTo(
DefaultJavaLibraryRule.this.getBuildTarget());
if (ruleCanSeeDep &&
- isMissingBuildRule(transitiveNotDeclaredDep, remainingImports, jarResolver)) {
+ isMissingBuildRule(filesystem,
+ transitiveNotDeclaredDep,
+ remainingImports,
+ jarResolver)) {
suggestedDeps.add(transitiveNotDeclaredDep.getFullyQualifiedName());
}
// If we've wiped out all remaining imports, break the loop looking for them.
diff --git a/src/com/facebook/buck/java/JavacInMemoryStep.java b/src/com/facebook/buck/java/JavacInMemoryStep.java
index cab2500..adfe71c 100644
--- a/src/com/facebook/buck/java/JavacInMemoryStep.java
+++ b/src/com/facebook/buck/java/JavacInMemoryStep.java
@@ -125,8 +125,10 @@
private static final String LINE_SEPARATOR = System.getProperty("line.separator");
- public static interface SuggestBuildRules extends
- Function<ImmutableSet<String>,ImmutableSet<String>> {}
+ public static interface SuggestBuildRules {
+ public ImmutableSet<String> suggest(ProjectFilesystem filesystem,
+ ImmutableSet<String> failedImports);
+ }
public JavacInMemoryStep(
String outputDirectory,
@@ -226,7 +228,8 @@
String firstOrderStderr = stderr.getContentsAsString(Charsets.UTF_8);
if (declaredDepsResult != 0) {
- int transitiveResult = buildWithClasspath(context, getClasspathEntries());
+ int transitiveResult = buildWithClasspath(context,
+ ImmutableSet.copyOf(transitiveClasspathEntries));
if (transitiveResult == 0) {
ImmutableSet<String> failedImports = findFailedImports(firstOrderStderr);
@@ -237,7 +240,8 @@
if (suggestBuildRules.isPresent()) {
context.getStdErr().println("Try adding the following deps:");
context.getStdErr().println(Joiner.on(LINE_SEPARATOR)
- .join(suggestBuildRules.get().apply(failedImports)));
+ .join(suggestBuildRules.get().suggest(context.getProjectFilesystem(),
+ failedImports)));
}
context.getStdErr().println();
context.getStdErr().println();
diff --git a/test/com/facebook/buck/java/DefaultJavaLibraryRuleIntegrationTest.java b/test/com/facebook/buck/java/DefaultJavaLibraryRuleIntegrationTest.java
index d9771e2..3b4921e 100644
--- a/test/com/facebook/buck/java/DefaultJavaLibraryRuleIntegrationTest.java
+++ b/test/com/facebook/buck/java/DefaultJavaLibraryRuleIntegrationTest.java
@@ -17,6 +17,7 @@
package com.facebook.buck.java;
import static java.nio.file.StandardOpenOption.WRITE;
+import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.not;
import static org.junit.Assert.assertEquals;
@@ -31,6 +32,7 @@
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.base.Strings;
import com.google.common.io.Files;
@@ -125,6 +127,64 @@
}
@Test
+ public void testBuildJavaLibraryWithTransitive() throws IOException {
+ workspace = TestDataHelper.createProjectWorkspaceForScenario(
+ this, "warn_on_transitive", tmp);
+ workspace.setUp();
+
+ // Run `buck build`.
+ ProcessResult buildResult = workspace.runBuckCommand("build", "//:raz", "-b", "TRANSITIVE");
+ buildResult.assertExitCode("Successful build should exit with 0.", 0);
+
+ workspace.verify();
+ }
+
+ @Test
+ public void testBuildJavaLibraryWithFirstOrder() throws IOException {
+ workspace = TestDataHelper.createProjectWorkspaceForScenario(
+ this, "warn_on_transitive", tmp);
+ workspace.setUp();
+
+ // Run `buck build`.
+ ProcessResult buildResult = workspace.runBuckCommand("build",
+ "//:raz",
+ "-b",
+ "FIRST_ORDER_ONLY");
+ buildResult.assertExitCode("Build should have failed", 1);
+
+ workspace.verify();
+ }
+
+ @Test
+ public void testBuildJavaLibraryWithWarnOnTransitive() throws IOException {
+ workspace = TestDataHelper.createProjectWorkspaceForScenario(
+ this, "warn_on_transitive", tmp);
+ workspace.setUp();
+
+ // Run `buck build`.
+ ProcessResult buildResult = workspace.runBuckCommand("build",
+ "//:raz",
+ "-b",
+ "WARN_ON_TRANSITIVE");
+
+ String expectedWarning = Joiner.on("\n").join(
+ "Rule //:raz builds with its transitive dependencies but not with its first order " +
+ "dependencies.",
+ "The following packages were missing:",
+ "Foo",
+ "Try adding the following deps:",
+ "//:foo");
+
+ buildResult.assertExitCode("Build should have succeeded with warnings.", 0);
+
+ assertThat(
+ buildResult.getStderr(),
+ containsString(expectedWarning));
+
+ workspace.verify();
+ }
+
+ @Test
public void testFileChangeThatDoesNotModifyAbiAvoidsRebuild() throws IOException {
workspace = TestDataHelper.createProjectWorkspaceForScenario(
this, "rulekey_changed_while_abi_stable", tmp);
diff --git a/test/com/facebook/buck/java/DefaultJavaLibraryRuleTest.java b/test/com/facebook/buck/java/DefaultJavaLibraryRuleTest.java
index ec2dee2..e6ec316 100644
--- a/test/com/facebook/buck/java/DefaultJavaLibraryRuleTest.java
+++ b/test/com/facebook/buck/java/DefaultJavaLibraryRuleTest.java
@@ -942,6 +942,7 @@
@Test
public void testSuggsetDepsReverseTopoSortRespected() {
BuildRuleResolver ruleResolver = new BuildRuleResolver();
+ ProjectFilesystem projectFilesystem = new ProjectFilesystem(tmp.getRoot());
BuildTarget libraryOneTarget = BuildTargetFactory.newInstance("//:libone");
DefaultJavaLibraryRule libraryOne = ruleResolver.buildAndAddToIndex(
@@ -991,7 +992,8 @@
assertTrue(suggestFn.isPresent());
assertEquals(ImmutableSet.of("//:parent", "//:libone"),
- suggestFn.get().apply(ImmutableSet.of("com.facebook.Foo", "com.facebook.Bar")));
+ suggestFn.get().suggest(projectFilesystem,
+ ImmutableSet.of("com.facebook.Foo", "com.facebook.Bar")));
EasyMock.verify(context);
}
@@ -1002,23 +1004,23 @@
private DefaultJavaLibraryRule.JarResolver createJarResolver(
final ImmutableMap<String, String> classToSymbols) {
- ImmutableSetMultimap.Builder<String, String> resolveMapBuilder =
+ ImmutableSetMultimap.Builder<Path, String> resolveMapBuilder =
ImmutableSetMultimap.builder();
for (Map.Entry<String, String> entry : classToSymbols.entrySet()) {
String fullyQualified = entry.getValue();
String packageName = fullyQualified.substring(0, fullyQualified.lastIndexOf('.'));
String className = fullyQualified.substring(fullyQualified.lastIndexOf('.'));
- resolveMapBuilder.putAll(entry.getKey(), fullyQualified, packageName, className);
+ resolveMapBuilder.putAll(Paths.get(entry.getKey()), fullyQualified, packageName, className);
}
- final ImmutableSetMultimap<String, String> resolveMap = resolveMapBuilder.build();
+ final ImmutableSetMultimap<Path, String> resolveMap = resolveMapBuilder.build();
return new DefaultJavaLibraryRule.JarResolver() {
@Override
- public ImmutableSet<String> apply(String input) {
- if (resolveMap.containsKey(input)) {
- return resolveMap.get(input);
+ public ImmutableSet<String> resolve(ProjectFilesystem filesystem, Path relativeClassPath) {
+ if (resolveMap.containsKey(relativeClassPath)) {
+ return resolveMap.get(relativeClassPath);
} else {
return ImmutableSet.of();
}
diff --git a/test/com/facebook/buck/java/testdata/warn_on_transitive/BUCK b/test/com/facebook/buck/java/testdata/warn_on_transitive/BUCK
new file mode 100644
index 0000000..83b07e9
--- /dev/null
+++ b/test/com/facebook/buck/java/testdata/warn_on_transitive/BUCK
@@ -0,0 +1,21 @@
+# This is a canonical example of a genrule() that generates Java source code
+# paired with a java_library() that compiles the output of the genrule() along
+# with some hard-coded Java source code into a single library.
+
+java_library(
+ name = 'foo',
+ srcs = ['Foo.java'],
+)
+
+java_library(
+ name = 'bar',
+ srcs = ['Bar.java'],
+ deps = [':foo'],
+)
+
+java_library(
+ name = 'raz',
+ srcs = ['Raz.java'],
+ deps = [':bar'],
+)
+
diff --git a/test/com/facebook/buck/java/testdata/warn_on_transitive/Bar.java b/test/com/facebook/buck/java/testdata/warn_on_transitive/Bar.java
new file mode 100644
index 0000000..469ea7f
--- /dev/null
+++ b/test/com/facebook/buck/java/testdata/warn_on_transitive/Bar.java
@@ -0,0 +1,21 @@
+/*
+ * 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 {
+ private Foo foo;
+}
diff --git a/test/com/facebook/buck/java/testdata/warn_on_transitive/Foo.java b/test/com/facebook/buck/java/testdata/warn_on_transitive/Foo.java
new file mode 100644
index 0000000..cbee490
--- /dev/null
+++ b/test/com/facebook/buck/java/testdata/warn_on_transitive/Foo.java
@@ -0,0 +1,19 @@
+/*
+ * 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 {}
diff --git a/test/com/facebook/buck/java/testdata/warn_on_transitive/Raz.java b/test/com/facebook/buck/java/testdata/warn_on_transitive/Raz.java
new file mode 100644
index 0000000..23e2023
--- /dev/null
+++ b/test/com/facebook/buck/java/testdata/warn_on_transitive/Raz.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 Raz {
+ private Foo foo;
+ private Bar bar;
+}
