From a4852b5fbf09c40bfebfec6ed231ebeb11950194 Mon Sep 17 00:00:00 2001 From: cnsun Date: Mon, 23 Apr 2018 11:49:44 -0700 Subject: Add a new "STRICT_ERROR" for the flag "--experimental_import_deps_checking" to check the direct dependencies for aar_import targets. Currently the default value of this flag is not changed. And it will be enabled in a separate cl. RELNOTES: None PiperOrigin-RevId: 193959866 --- .../build/lib/rules/android/AarImport.java | 29 ++++-- .../rules/java/ImportDepsCheckActionBuilder.java | 22 ++-- .../build/lib/rules/java/JavaConfiguration.java | 4 +- .../build/lib/rules/android/AarImportTest.java | 113 ++++++++++++++++++++- 4 files changed, 145 insertions(+), 23 deletions(-) (limited to 'src') diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AarImport.java b/src/main/java/com/google/devtools/build/lib/rules/android/AarImport.java index c44cc3fd22..c4197b8367 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AarImport.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AarImport.java @@ -33,6 +33,8 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.rules.java.ImportDepsCheckActionBuilder; import com.google.devtools.build.lib.rules.java.JavaCommon; +import com.google.devtools.build.lib.rules.java.JavaCompilationArgs; +import com.google.devtools.build.lib.rules.java.JavaCompilationArgs.ClasspathType; import com.google.devtools.build.lib.rules.java.JavaCompilationArgsProvider; import com.google.devtools.build.lib.rules.java.JavaCompilationArtifacts; import com.google.devtools.build.lib.rules.java.JavaConfiguration; @@ -148,14 +150,7 @@ public class AarImport implements RuleConfiguredTargetFactory { /* compileDeps = */ targets, /* runtimeDeps = */ targets, /* bothDeps = */ targets); - // Need to compute the "deps" here, as mergedJar is put on the classpath later too. - NestedSet deps = - common - .collectJavaCompilationArgs( - /* recursive = */ true, - JavaCommon.isNeverLink(ruleContext), - /* srcLessDepsExport = */ false) - .getCompileTimeJars(); + common.setJavaCompilationArtifacts( new JavaCompilationArtifacts.Builder() .addRuntimeJar(mergedJar) @@ -173,10 +168,13 @@ public class AarImport implements RuleConfiguredTargetFactory { JavaCommon.isNeverLink(ruleContext), /* srcLessDepsExport = */ false)); - JavaConfiguration javaConfig = ruleContext.getFragment(JavaConfiguration.class); - Artifact depsCheckerResult = null; + JavaConfiguration javaConfig = ruleContext.getFragment(JavaConfiguration.class); if (javaConfig.getImportDepsCheckingLevel() != ImportDepsCheckingLevel.OFF) { + NestedSet deps = + getCompileTimeJarsFromCollection( + targets, + javaConfig.getImportDepsCheckingLevel() == ImportDepsCheckingLevel.STRICT_ERROR); NestedSet bootclasspath = getBootclasspath(ruleContext); depsCheckerResult = createAarArtifact(ruleContext, "aar_import_deps_checker_result.txt"); ImportDepsCheckActionBuilder.newBuilder() @@ -223,6 +221,17 @@ public class AarImport implements RuleConfiguredTargetFactory { return ruleBuilder.build(); } + private NestedSet getCompileTimeJarsFromCollection( + ImmutableList deps, boolean isStrict) { + return JavaCompilationArgs.builder() + .addTransitiveCompilationArgs( + JavaCompilationArgsProvider.legacyFromTargets(deps), + /*recursive=*/ !isStrict, + ClasspathType.BOTH) + .build() + .getCompileTimeJars(); + } + private NestedSet getBootclasspath(RuleContext ruleContext) { if (AndroidCommon.getAndroidConfig(ruleContext).desugarJava8()) { return NestedSetBuilder.stableOrder() diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/ImportDepsCheckActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/java/ImportDepsCheckActionBuilder.java index e6d9f00ff1..405e81530f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/ImportDepsCheckActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/ImportDepsCheckActionBuilder.java @@ -80,10 +80,7 @@ public final class ImportDepsCheckActionBuilder { checkNotNull(bootclasspath); checkNotNull(declaredDeps); checkState( - importDepsCheckingLevel == ImportDepsCheckingLevel.ERROR - || importDepsCheckingLevel == ImportDepsCheckingLevel.WARNING, - "%s", - importDepsCheckingLevel); + importDepsCheckingLevel != ImportDepsCheckingLevel.OFF, "%s", importDepsCheckingLevel); CustomCommandLine args = CustomCommandLine.builder() @@ -91,10 +88,7 @@ public final class ImportDepsCheckActionBuilder { .addExecPaths(VectorArg.addBefore("--input").each(jarsToCheck)) .addExecPaths(VectorArg.addBefore("--classpath_entry").each(declaredDeps)) .addExecPaths(VectorArg.addBefore("--bootclasspath_entry").each(bootclasspath)) - .add( - importDepsCheckingLevel == ImportDepsCheckingLevel.ERROR - ? "--fail_on_errors" - : "--nofail_on_errors") + .addDynamicString(convertErrorFlag(importDepsCheckingLevel)) .build(); ruleContext.registerAction( new SpawnAction.Builder() @@ -115,4 +109,16 @@ public final class ImportDepsCheckActionBuilder { .addCommandLine(args) .build(ruleContext)); } + + private static String convertErrorFlag(ImportDepsCheckingLevel level) { + switch (level) { + case ERROR: + case STRICT_ERROR: + return "--fail_on_errors"; + case WARNING: + return "--nofail_on_errors"; + default: + throw new RuntimeException("Unhandled deps checking level: " + level); + } + } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java index 071bce4b7e..294e4054f5 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java @@ -76,7 +76,9 @@ public final class JavaConfiguration extends Fragment { /** Emit warnings when the dependencies of java_import/aar_import are not complete. */ WARNING, /** Emit errors when the dependencies of java_import/aar_import are not complete. */ - ERROR + ERROR, + /** Emit errors when the DIRECT dependencies of java_import/aar_import are not complete. */ + STRICT_ERROR, } /** diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AarImportTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/AarImportTest.java index 0707611329..7c150b1d72 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/android/AarImportTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/android/AarImportTest.java @@ -32,8 +32,10 @@ import com.google.devtools.build.lib.rules.java.JavaCompilationArgsProvider; import com.google.devtools.build.lib.rules.java.JavaInfo; import com.google.devtools.build.lib.rules.java.JavaRuleOutputJarsProvider; import com.google.devtools.build.lib.rules.java.JavaRuleOutputJarsProvider.OutputJar; +import java.util.Iterator; import java.util.List; import java.util.Set; +import org.junit.Assert; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -44,8 +46,8 @@ import org.junit.runners.JUnit4; public class AarImportTest extends BuildViewTestCase { @Before public void setup() throws Exception { - useConfiguration("--experimental_import_deps_checking=ERROR"); - scratch.file("a/BUILD", + scratch.file( + "a/BUILD", "aar_import(", " name = 'foo',", " aar = 'foo.aar',", @@ -59,6 +61,16 @@ public class AarImportTest extends BuildViewTestCase { " aar = 'bar.aar',", " deps = [':baz'],", " exports = [':foo', '//java:baz'],", + ")", + "aar_import(", + " name = 'intermediate',", + " aar = 'intermediate.aar',", + " deps = [':bar']", + ")", + "aar_import(", + " name = 'last',", + " aar = 'last.aar',", + " deps = [':intermediate'],", ")"); scratch.file("java/BUILD", "android_binary(", @@ -129,7 +141,66 @@ public class AarImportTest extends BuildViewTestCase { } @Test - public void testDepsCheckerActionExists() throws Exception { + public void testDepsCheckerActionExistsForLevelError() throws Exception { + useConfiguration("--experimental_import_deps_checking=ERROR"); + ConfiguredTarget aarImportTarget = getConfiguredTarget("//a:last"); + OutputGroupInfo outputGroupInfo = aarImportTarget.get(OutputGroupInfo.SKYLARK_CONSTRUCTOR); + NestedSet outputGroup = + outputGroupInfo.getOutputGroup(OutputGroupInfo.HIDDEN_TOP_LEVEL); + Artifact artifact = Iterables.getOnlyElement(outputGroup); + assertThat(artifact.isTreeArtifact()).isFalse(); + assertThat(artifact.getExecPathString()) + .endsWith("_aar/last/aar_import_deps_checker_result.txt"); + + SpawnAction checkerAction = getGeneratingSpawnAction(artifact); + List arguments = checkerAction.getArguments(); + assertThat(arguments) + .containsAllOf( + "--bootclasspath_entry", + "--classpath_entry", + "--input", + "--output", + "--fail_on_errors"); + ensureArgumentsHaveClassEntryOptionWithSuffix(arguments, "/bar/classes_and_libs_merged.jar"); + ensureArgumentsHaveClassEntryOptionWithSuffix(arguments, "/baz/java/baz-ijar.jar"); + ensureArgumentsHaveClassEntryOptionWithSuffix(arguments, "/baz/classes_and_libs_merged.jar"); + ensureArgumentsHaveClassEntryOptionWithSuffix(arguments, "/foo/classes_and_libs_merged.jar"); + ensureArgumentsHaveClassEntryOptionWithSuffix( + arguments, "/intermediate/classes_and_libs_merged.jar"); + assertThat(arguments.stream().filter(arg -> "--classpath_entry".equals(arg)).count()) + .isEqualTo(5); + } + + @Test + public void testDepsCheckerActionExistsForLevelStrictError() throws Exception { + useConfiguration("--experimental_import_deps_checking=STRICT_ERROR"); + ConfiguredTarget aarImportTarget = getConfiguredTarget("//a:last"); + OutputGroupInfo outputGroupInfo = aarImportTarget.get(OutputGroupInfo.SKYLARK_CONSTRUCTOR); + NestedSet outputGroup = + outputGroupInfo.getOutputGroup(OutputGroupInfo.HIDDEN_TOP_LEVEL); + Artifact artifact = Iterables.getOnlyElement(outputGroup); + assertThat(artifact.isTreeArtifact()).isFalse(); + assertThat(artifact.getExecPathString()) + .endsWith("_aar/last/aar_import_deps_checker_result.txt"); + + SpawnAction checkerAction = getGeneratingSpawnAction(artifact); + List arguments = checkerAction.getArguments(); + assertThat(arguments) + .containsAllOf( + "--bootclasspath_entry", + "--classpath_entry", + "--input", + "--output", + "--fail_on_errors"); + ensureArgumentsHaveClassEntryOptionWithSuffix( + arguments, "/intermediate/classes_and_libs_merged.jar"); + assertThat(arguments.stream().filter(arg -> "--classpath_entry".equals(arg)).count()) + .isEqualTo(1); + } + + @Test + public void testDepsCheckerActionExistsForLevelWarning() throws Exception { + useConfiguration("--experimental_import_deps_checking=WARNING"); ConfiguredTarget aarImportTarget = getConfiguredTarget("//a:bar"); OutputGroupInfo outputGroupInfo = aarImportTarget.get(OutputGroupInfo.SKYLARK_CONSTRUCTOR); NestedSet outputGroup = @@ -147,7 +218,41 @@ public class AarImportTest extends BuildViewTestCase { "--classpath_entry", "--input", "--output", - "--fail_on_errors"); + "--nofail_on_errors"); + } + + /** + * Tests whether the given argument list contains an argument nameds "--classpath_entry" with a + * value that ends with the given suffix. + */ + private static void ensureArgumentsHaveClassEntryOptionWithSuffix( + List arguments, String suffix) { + assertThat(arguments).isNotEmpty(); + Iterator iterator = arguments.iterator(); + assertThat(iterator.hasNext()).isTrue(); + String prev = iterator.next(); + while (iterator.hasNext()) { + String current = iterator.next(); + if ("--classpath_entry".equals(prev) && current.endsWith(suffix)) { + return; // Success. + } + prev = current; + } + Assert.fail( + "The arguments does not have the expected --classpath_entry: The arguments are " + + arguments + + ", and the expected class entry suffix is " + + suffix); + } + + @Test + public void testDepsCheckerActionNotExistsForLevelOff() throws Exception { + useConfiguration("--experimental_import_deps_checking=OFF"); + ConfiguredTarget aarImportTarget = getConfiguredTarget("//a:bar"); + OutputGroupInfo outputGroupInfo = aarImportTarget.get(OutputGroupInfo.SKYLARK_CONSTRUCTOR); + NestedSet outputGroup = + outputGroupInfo.getOutputGroup(OutputGroupInfo.HIDDEN_TOP_LEVEL); + assertThat(outputGroup).isEmpty(); } @Test -- cgit v1.2.3