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

4 files changed, 145 insertions, 23 deletions

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<Artifact> 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<Artifact> deps =
+          getCompileTimeJarsFromCollection(
+              targets,
+              javaConfig.getImportDepsCheckingLevel() == ImportDepsCheckingLevel.STRICT_ERROR);
       NestedSet<Artifact> 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<Artifact> getCompileTimeJarsFromCollection(
+      ImmutableList<TransitiveInfoCollection> deps, boolean isStrict) {
+    return JavaCompilationArgs.builder()
+        .addTransitiveCompilationArgs(
+            JavaCompilationArgsProvider.legacyFromTargets(deps),
+            /*recursive=*/ !isStrict,
+            ClasspathType.BOTH)
+        .build()
+        .getCompileTimeJars();
+  }
+
   private NestedSet<Artifact> getBootclasspath(RuleContext ruleContext) {
     if (AndroidCommon.getAndroidConfig(ruleContext).desugarJava8()) {
       return NestedSetBuilder.<Artifact>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<Artifact> 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<String> 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<Artifact> 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<String> 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<Artifact> 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<String> arguments, String suffix) {
+    assertThat(arguments).isNotEmpty();
+    Iterator<String> 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<Artifact> outputGroup =
+        outputGroupInfo.getOutputGroup(OutputGroupInfo.HIDDEN_TOP_LEVEL);
+    assertThat(outputGroup).isEmpty();
   }
 
   @Test