Add integration tests for android_sdk_repository() and android_ndk_repository() for testing invalid directory path attributes, and improve error descriptiveness.

Moved AndroidRepositoryUtils logic into an abstract AndroidRepositoryFunction that Android{S,N}dkRepositoryFunction extends from.

Examples of error messages:

1) Invalid NDK path

ERROR: Analysis of target '//examples/android/java/bazel:hello_world' failed; build aborted: in target '//external:android/crosstool': no such package '@androidndk//': Could not read RELEASE.TXT in Android NDK: /tmp/RELEASE.TXT (No such file or directory) Unable to read the Android NDK at /tmp, the path may be invalid. Is thepath in android_ndk_repository() or ANDROID_NDK_HOME set correctly? If the path is correct, the contents in the Android NDK directory may have been modified. Please remove the NDK and download it again with the Android SDK manager.

2) Invalid SDK path

ERROR: Analysis of target '//examples/android/java/bazel:hello_world' failed; build aborted: no such package '@androidsdk//com.android.support': Expected directory at /tmp/platforms but it is not a directory or it does not exist. Unable to read the Android SDK at /tmp, the path may be invalid. Is the path in android_sdk_repository() or ANDROID_HOME set correctly? If the path is correct, the contents in the Android SDK directory may have been modified. Please remove the SDK and download it again with the Android SDK manager.

GITHUB: #3740

RELNOTES: None.
PiperOrigin-RevId: 170068567

5 files changed, 139 insertions, 68 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidNdkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidNdkRepositoryFunction.java
index 4cd184b23c..58d51e5579 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidNdkRepositoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidNdkRepositoryFunction.java
@@ -30,7 +30,6 @@ import com.google.devtools.build.lib.bazel.rules.android.ndkcrosstools.StlImpls;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue;
-import com.google.devtools.build.lib.rules.repository.RepositoryFunction;
 import com.google.devtools.build.lib.rules.repository.WorkspaceAttributeMapper;
 import com.google.devtools.build.lib.skyframe.DirectoryListingValue;
 import com.google.devtools.build.lib.skyframe.FileSymlinkException;
@@ -55,10 +54,8 @@ import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
 
-/**
- * Implementation of the {@code android_ndk_repository} rule.
- */
-public class AndroidNdkRepositoryFunction extends RepositoryFunction {
+/** Implementation of the {@code android_ndk_repository} rule. */
+public class AndroidNdkRepositoryFunction extends AndroidRepositoryFunction {
 
   private static final String TOOLCHAIN_NAME_PREFIX = "toolchain-";
   private static final String PATH_ENV_VAR = "ANDROID_NDK_HOME";
@@ -82,7 +79,6 @@ public class AndroidNdkRepositoryFunction extends RepositoryFunction {
     return true;
   }
 
-
   @Override
   public boolean verifyMarkerData(Rule rule, Map<String, String> markerData, Environment env)
       throws InterruptedException {
@@ -94,8 +90,12 @@ public class AndroidNdkRepositoryFunction extends RepositoryFunction {
   }
 
   @Override
-  public RepositoryDirectoryValue.Builder fetch(Rule rule, Path outputDirectory,
-      BlazeDirectories directories, Environment env, Map<String, String> markerData)
+  public RepositoryDirectoryValue.Builder fetch(
+      Rule rule,
+      Path outputDirectory,
+      BlazeDirectories directories,
+      Environment env,
+      Map<String, String> markerData)
       throws InterruptedException, RepositoryFunctionException {
     Map<String, String> environ =
         declareEnvironmentDependencies(markerData, env, PATH_ENV_VAR_AS_LIST);
@@ -149,24 +149,22 @@ public class AndroidNdkRepositoryFunction extends RepositoryFunction {
       }
     } else {
       DirectoryListingValue platformsDirectoryValue =
-          AndroidRepositoryUtils.getDirectoryListing(ndkHome, PLATFORMS_DIR, env);
+          getDirectoryListing(ndkHome, PLATFORMS_DIR, env);
       if (platformsDirectoryValue == null) {
         return null;
       }
 
-      ImmutableSortedSet<Integer> apiLevels =
-          AndroidRepositoryUtils.getApiLevels(platformsDirectoryValue.getDirents());
+      ImmutableSortedSet<Integer> apiLevels = getApiLevels(platformsDirectoryValue.getDirents());
       if (apiLevels.isEmpty()) {
         // Every Android NDK to date ships with multiple api levels, so the only reason that this
         // would be empty is if the user is not pointing to a standard NDK or has tinkered with it
         // themselves.
-        throw new RepositoryFunctionException(
+        throwInvalidPathException(
+            ndkHome,
             new EvalException(
                 rule.getLocation(),
                 "android_ndk_repository requires that at least one Android platform is present in "
-                    + "the Android NDK platforms directory. Please ensure that the path attribute "
-                    + "or the ANDROID_NDK_HOME environment variable points to a valid NDK."),
-            Transience.PERSISTENT);
+                    + "the Android NDK platforms directory."));
       }
       apiLevelString = apiLevels.first().toString();
     }
@@ -251,18 +249,21 @@ public class AndroidNdkRepositoryFunction extends RepositoryFunction {
 
       StringBuilder toolchainMap = new StringBuilder();
       for (CToolchain toolchain : crosstool.getToolchainList()) {
-        toolchainMap.append(String.format("      \"%s|%s\": \":%s\",\n",
-            toolchain.getTargetCpu(),
-            toolchain.getCompiler(),
-            toolchain.getToolchainIdentifier()));
+        toolchainMap.append(
+            String.format(
+                "      \"%s|%s\": \":%s\",\n",
+                toolchain.getTargetCpu(),
+                toolchain.getCompiler(),
+                toolchain.getToolchainIdentifier()));
       }
 
       String toolchainName = createToolchainName(crosstoolStlPair.stlImpl.getName());
 
-      ccToolchainSuites.append(ccToolchainSuiteTemplate
-          .replace("%toolchainName%", toolchainName)
-          .replace("%toolchainMap%", toolchainMap.toString().trim())
-          .replace("%crosstoolReleaseProto%", crosstool.toString()));
+      ccToolchainSuites.append(
+          ccToolchainSuiteTemplate
+              .replace("%toolchainName%", toolchainName)
+              .replace("%toolchainMap%", toolchainMap.toString().trim())
+              .replace("%crosstoolReleaseProto%", crosstool.toString()));
 
       // Create the cc_toolchain rules
       for (CToolchain toolchain : crosstool.getToolchainList()) {
@@ -271,11 +272,12 @@ public class AndroidNdkRepositoryFunction extends RepositoryFunction {
 
       // Create the STL file group rules
       for (Map.Entry<String, String> entry :
-        crosstoolStlPair.stlImpl.getFilegroupNamesAndFilegroupFileGlobPatterns().entrySet()) {
+          crosstoolStlPair.stlImpl.getFilegroupNamesAndFilegroupFileGlobPatterns().entrySet()) {
 
-        stlFilegroups.append(stlFilegroupTemplate
-            .replace("%name%", entry.getKey())
-            .replace("%fileGlobPattern%", entry.getValue()));
+        stlFilegroups.append(
+            stlFilegroupTemplate
+                .replace("%name%", entry.getKey())
+                .replace("%fileGlobPattern%", entry.getValue()));
       }
     }
 
@@ -336,8 +338,7 @@ public class AndroidNdkRepositoryFunction extends RepositoryFunction {
 
     StringBuilder toolchainFileGlobs = new StringBuilder();
     for (String toolchainFileGlobPattern : toolchainFileGlobPatterns) {
-      toolchainFileGlobs.append(String.format(
-          "        \"%s\",\n", toolchainFileGlobPattern));
+      toolchainFileGlobs.append(String.format("        \"%s\",\n", toolchainFileGlobPattern));
     }
 
     return ccToolchainTemplate
@@ -349,7 +350,7 @@ public class AndroidNdkRepositoryFunction extends RepositoryFunction {
         .replace("%toolchainFileGlobs%", toolchainFileGlobs.toString().trim());
   }
 
-  private static NdkRelease getNdkRelease(Path directory, Environment env)
+  private NdkRelease getNdkRelease(Path directory, Environment env)
       throws RepositoryFunctionException, InterruptedException {
 
     // For NDK r11+
@@ -361,17 +362,24 @@ public class AndroidNdkRepositoryFunction extends RepositoryFunction {
 
     SkyKey releaseFileKey = FileValue.key(RootedPath.toRootedPath(directory, releaseFilePath));
 
-    String releaseFileContent;
+    String releaseFileContent = "";
     try {
-      env.getValueOrThrow(releaseFileKey, IOException.class, FileSymlinkException.class,
+      env.getValueOrThrow(
+          releaseFileKey,
+          IOException.class,
+          FileSymlinkException.class,
           InconsistentFilesystemException.class);
 
-      releaseFileContent = new String(FileSystemUtils.readContent(releaseFilePath),
-          StandardCharsets.UTF_8);
+      releaseFileContent =
+          new String(FileSystemUtils.readContent(releaseFilePath), StandardCharsets.UTF_8);
     } catch (IOException | FileSymlinkException | InconsistentFilesystemException e) {
-      throw new RepositoryFunctionException(
-          new IOException("Could not read " + releaseFilePath.getBaseName() + " in Android NDK: "
-              + e.getMessage()), Transience.PERSISTENT);
+      throwInvalidPathException(
+          directory,
+          new IOException(
+              "Could not read "
+                  + releaseFilePath.getBaseName()
+                  + " in Android NDK: "
+                  + e.getMessage()));
     }
 
     return NdkRelease.create(releaseFileContent.trim());
@@ -384,4 +392,18 @@ public class AndroidNdkRepositoryFunction extends RepositoryFunction {
       throw new IllegalStateException(e);
     }
   }
+
+  @Override
+  protected void throwInvalidPathException(Path path, Exception e)
+      throws RepositoryFunctionException {
+    throw new RepositoryFunctionException(
+        new IOException(
+            String.format(
+                "%s Unable to read the Android NDK at %s, the path may be invalid. Is "
+                    + "the path in android_ndk_repository() or %s set correctly? If the path is "
+                    + "correct, the contents in the Android NDK directory may have been modified.",
+                e.getMessage(), path, PATH_ENV_VAR),
+            e),
+        Transience.PERSISTENT);
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidRepositoryUtils.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidRepositoryFunction.java
index 4e5f0d0df4..4d232a9cc6 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidRepositoryUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidRepositoryFunction.java
@@ -14,7 +14,7 @@
 package com.google.devtools.build.lib.bazel.rules.android;
 
 import com.google.common.collect.ImmutableSortedSet;
-import com.google.devtools.build.lib.rules.repository.RepositoryFunction.RepositoryFunctionException;
+import com.google.devtools.build.lib.rules.repository.RepositoryFunction;
 import com.google.devtools.build.lib.skyframe.DirectoryListingValue;
 import com.google.devtools.build.lib.skyframe.Dirents;
 import com.google.devtools.build.lib.skyframe.FileSymlinkException;
@@ -30,19 +30,25 @@ import java.io.IOException;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
-/**
- * Helper methods for {@code AndroidSdkRepositoryFunction} and {@code AndroidNdkRepositoryFunction}.
- */
-final class AndroidRepositoryUtils {
+/** This class contains the common logic between Android NDK and SDK repository functions. */
+abstract class AndroidRepositoryFunction extends RepositoryFunction {
   private static final Pattern PLATFORMS_API_LEVEL_PATTERN = Pattern.compile("android-(\\d+)");
 
   /**
+   * Android rules depend on the contents in the SDK and NDK existing in the correct locations. This
+   * error is thrown when required files or directories are not found or believed to be tampered
+   * with.
+   */
+  abstract void throwInvalidPathException(Path path, Exception e)
+      throws RepositoryFunctionException;
+
+  /**
    * Gets a {@link DirectoryListingValue} for {@code dirPath} or returns null.
    *
-   * <p>First, we get a {@link FileValue} to check the the {@code dirPath} exists and is a
-   * directory. If not, we throw an exception.
+   * <p>First, we get a {@link FileValue} to check the {@code dirPath} exists and is a directory. If
+   * not, we throw an exception.
    */
-  static DirectoryListingValue getDirectoryListing(Path root, PathFragment dirPath, Environment env)
+  final DirectoryListingValue getDirectoryListing(Path root, PathFragment dirPath, Environment env)
       throws RepositoryFunctionException, InterruptedException {
     RootedPath rootedPath = RootedPath.toRootedPath(root, dirPath);
     try {
@@ -57,12 +63,12 @@ final class AndroidRepositoryUtils {
         return null;
       }
       if (!dirFileValue.exists() || !dirFileValue.isDirectory()) {
-        throw new RepositoryFunctionException(
+        throwInvalidPathException(
+            root,
             new IOException(
                 String.format(
                     "Expected directory at %s but it is not a directory or it does not exist.",
-                    rootedPath.asPath().getPathString())),
-            Transience.PERSISTENT);
+                    rootedPath.asPath().getPathString())));
       }
       return (DirectoryListingValue)
           env.getValueOrThrow(
@@ -78,10 +84,10 @@ final class AndroidRepositoryUtils {
   /**
    * Gets the numeric api levels from the contents of the platforms directory in descending order.
    *
-   * Note that the directory entries are assumed to match {@code android-[0-9]+}. Any directory
+   * <p>Note that the directory entries are assumed to match {@code android-[0-9]+}. Any directory
    * entries that are not directories or do not match that pattern are ignored.
    */
-  static ImmutableSortedSet<Integer> getApiLevels(Dirents platformsDirectories) {
+  static final ImmutableSortedSet<Integer> getApiLevels(Dirents platformsDirectories) {
     ImmutableSortedSet.Builder<Integer> apiLevels = ImmutableSortedSet.reverseOrder();
     for (Dirent platformDirectory : platformsDirectories) {
       if (platformDirectory.getType() != Dirent.Type.DIRECTORY) {
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryFunction.java
index aa5f7619b5..de58dbf653 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryFunction.java
@@ -26,7 +26,6 @@ import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.RuleDefinition;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue;
-import com.google.devtools.build.lib.rules.repository.RepositoryFunction;
 import com.google.devtools.build.lib.rules.repository.WorkspaceAttributeMapper;
 import com.google.devtools.build.lib.skyframe.DirectoryListingValue;
 import com.google.devtools.build.lib.skyframe.Dirents;
@@ -42,7 +41,6 @@ import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.RootedPath;
 import com.google.devtools.build.skyframe.SkyFunction.Environment;
-import com.google.devtools.build.skyframe.SkyFunctionException;
 import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
@@ -51,10 +49,8 @@ import java.io.IOException;
 import java.util.Map;
 import java.util.Properties;
 
-/**
- * Implementation of the {@code android_sdk_repository} rule.
- */
-public class AndroidSdkRepositoryFunction extends RepositoryFunction {
+/** Implementation of the {@code android_sdk_repository} rule. */
+public class AndroidSdkRepositoryFunction extends AndroidRepositoryFunction {
   private static final PathFragment BUILD_TOOLS_DIR = PathFragment.create("build-tools");
   private static final PathFragment PLATFORMS_DIR = PathFragment.create("platforms");
   private static final PathFragment SYSTEM_IMAGES_DIR = PathFragment.create("system-images");
@@ -83,9 +79,13 @@ public class AndroidSdkRepositoryFunction extends RepositoryFunction {
   }
 
   @Override
-  public RepositoryDirectoryValue.Builder fetch(Rule rule, final Path outputDirectory,
-      BlazeDirectories directories, Environment env, Map<String, String> markerData)
-      throws SkyFunctionException, InterruptedException {
+  public RepositoryDirectoryValue.Builder fetch(
+      Rule rule,
+      final Path outputDirectory,
+      BlazeDirectories directories,
+      Environment env,
+      Map<String, String> markerData)
+      throws RepositoryFunctionException, InterruptedException {
     Map<String, String> environ =
         declareEnvironmentDependencies(markerData, env, PATH_ENV_VAR_AS_LIST);
     if (environ == null) {
@@ -114,13 +114,12 @@ public class AndroidSdkRepositoryFunction extends RepositoryFunction {
     }
 
     DirectoryListingValue platformsDirectoryValue =
-        AndroidRepositoryUtils.getDirectoryListing(androidSdkPath, PLATFORMS_DIR, env);
+        getDirectoryListing(androidSdkPath, PLATFORMS_DIR, env);
     if (platformsDirectoryValue == null) {
       return null;
     }
 
-    ImmutableSortedSet<Integer> apiLevels =
-        AndroidRepositoryUtils.getApiLevels(platformsDirectoryValue.getDirents());
+    ImmutableSortedSet<Integer> apiLevels = getApiLevels(platformsDirectoryValue.getDirents());
     if (apiLevels.isEmpty()) {
       throw new RepositoryFunctionException(
           new EvalException(
@@ -169,7 +168,7 @@ public class AndroidSdkRepositoryFunction extends RepositoryFunction {
       // If the build_tools_version attribute is not explicitly set, we select the highest version
       // installed in the SDK.
       DirectoryListingValue directoryValue =
-          AndroidRepositoryUtils.getDirectoryListing(androidSdkPath, BUILD_TOOLS_DIR, env);
+          getDirectoryListing(androidSdkPath, BUILD_TOOLS_DIR, env);
       if (directoryValue == null) {
         return null;
       }
@@ -348,19 +347,19 @@ public class AndroidSdkRepositoryFunction extends RepositoryFunction {
   }
 
   /**
-   * Gets PathFragments for /sdk/system-images/*&#47;*&#47;*, which are the directories in the
-   * SDK that contain system images needed for android_device.
+   * Gets PathFragments for /sdk/system-images/*&#47;*&#47;*, which are the directories in the SDK
+   * that contain system images needed for android_device.
    *
-   * If the sdk/system-images directory does not exist, an empty set is returned.
+   * <p>If the sdk/system-images directory does not exist, an empty set is returned.
    */
-  private static ImmutableSortedSet<PathFragment> getAndroidDeviceSystemImageDirs(
+  private ImmutableSortedSet<PathFragment> getAndroidDeviceSystemImageDirs(
       Path androidSdkPath, Environment env)
       throws RepositoryFunctionException, InterruptedException {
     if (!androidSdkPath.getRelative(SYSTEM_IMAGES_DIR).exists()) {
       return ImmutableSortedSet.of();
     }
     DirectoryListingValue systemImagesDirectoryValue =
-        AndroidRepositoryUtils.getDirectoryListing(androidSdkPath, SYSTEM_IMAGES_DIR, env);
+        getDirectoryListing(androidSdkPath, SYSTEM_IMAGES_DIR, env);
     if (systemImagesDirectoryValue == null) {
       return null;
     }
@@ -427,4 +426,18 @@ public class AndroidSdkRepositoryFunction extends RepositoryFunction {
     }
     return directoryListingValues.build();
   }
+
+  @Override
+  protected void throwInvalidPathException(Path path, Exception e)
+      throws RepositoryFunctionException {
+    throw new RepositoryFunctionException(
+        new IOException(
+            String.format(
+                "%s Unable to read the Android SDK at %s, the path may be invalid. Is "
+                    + "the path in android_sdk_repository() or %s set correctly? If the path is "
+                    + "correct, the contents in the Android SDK directory may have been modified.",
+                e.getMessage(), path, PATH_ENV_VAR),
+            e),
+        Transience.PERSISTENT);
+  }
 }
diff --git a/src/test/shell/bazel/android/android_integration_test.sh b/src/test/shell/bazel/android/android_integration_test.sh
index dc35d87eda..6048f638af 100755
--- a/src/test/shell/bazel/android/android_integration_test.sh
+++ b/src/test/shell/bazel/android/android_integration_test.sh
@@ -160,6 +160,21 @@ EOF
   expect_log "Either the path attribute of android_sdk_repository"
 }
 
+function test_android_sdk_repository_wrong_path() {
+  create_new_workspace
+  mkdir "$TEST_SRCDIR/some_dir"
+  cat > WORKSPACE <<EOF
+android_sdk_repository(
+    name = "androidsdk",
+    api_level = 25,
+    path = "$TEST_SRCDIR/some_dir",
+)
+EOF
+  bazel build @androidsdk//:files >& $TEST_log && fail "Should have failed"
+  expect_log "Unable to read the Android SDK at $TEST_SRCDIR/some_dir, the path may be invalid." \
+    " Is the path in android_sdk_repository() or \$ANDROID_SDK_HOME set correctly?"
+}
+
 # Check that the build succeeds if an android_sdk is specified with --android_sdk
 function test_specifying_android_sdk_flag() {
   create_new_workspace
diff --git a/src/test/shell/bazel/android/android_ndk_integration_test.sh b/src/test/shell/bazel/android/android_ndk_integration_test.sh
index 155514040b..9c4fd56073 100755
--- a/src/test/shell/bazel/android/android_ndk_integration_test.sh
+++ b/src/test/shell/bazel/android/android_ndk_integration_test.sh
@@ -284,6 +284,21 @@ EOF
   expect_log "Either the path attribute of android_ndk_repository"
 }
 
+function test_android_ndk_repository_wrong_path() {
+  create_new_workspace
+  mkdir "$TEST_SRCDIR/some_dir"
+  cat > WORKSPACE <<EOF
+android_ndk_repository(
+    name = "androidndk",
+    api_level = 25,
+    path = "$TEST_SRCDIR/some_dir",
+)
+EOF
+  bazel build @androidndk//:files >& $TEST_log && fail "Should have failed"
+  expect_log "Unable to read the Android NDK at $TEST_SRCDIR/some_dir, the path may be invalid." \
+    " Is the path in android_ndk_repository() or \$ANDROID_NDK_HOME set correctly?"
+}
+
 # ndk r10 and earlier
 if [[ ! -r "${TEST_SRCDIR}/androidndk/ndk/RELEASE.TXT" ]]; then
   # ndk r11 and later