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; +}