Improve handling of unknown NDK revisions in android_ndk_repository.

--
PiperOrigin-RevId: 148816635
MOS_MIGRATED_REVID=148816635

5 files changed, 137 insertions, 63 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 94b085d51f..b3fd9279d9 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
@@ -22,6 +22,7 @@ import com.google.devtools.build.lib.analysis.RuleDefinition;
 import com.google.devtools.build.lib.bazel.rules.android.ndkcrosstools.AndroidNdkCrosstools;
 import com.google.devtools.build.lib.bazel.rules.android.ndkcrosstools.AndroidNdkCrosstools.NdkCrosstoolsException;
 import com.google.devtools.build.lib.bazel.rules.android.ndkcrosstools.ApiLevel;
+import com.google.devtools.build.lib.bazel.rules.android.ndkcrosstools.NdkMajorRevision;
 import com.google.devtools.build.lib.bazel.rules.android.ndkcrosstools.NdkPaths;
 import com.google.devtools.build.lib.bazel.rules.android.ndkcrosstools.NdkRelease;
 import com.google.devtools.build.lib.bazel.rules.android.ndkcrosstools.StlImpl;
@@ -63,7 +64,7 @@ public class AndroidNdkRepositoryFunction extends RepositoryFunction {
   private static final PathFragment PLATFORMS_DIR = new PathFragment("platforms");
 
   private static final Iterable<String> PATH_ENV_VAR_AS_LIST = ImmutableList.of(PATH_ENV_VAR);
-  
+
   private static final class CrosstoolStlPair {
 
     private final CrosstoolRelease crosstoolRelease;
@@ -165,26 +166,36 @@ public class AndroidNdkRepositoryFunction extends RepositoryFunction {
       apiLevelString = apiLevels.first().toString();
     }
 
-    ApiLevel apiLevel;
-    apiLevel =
-        AndroidNdkCrosstools.KNOWN_NDK_MAJOR_REVISIONS
-            .get(ndkRelease.majorRevision)
-            .apiLevel(env.getListener(), ruleName, apiLevelString);
-
+    // NDK minor revisions should be backwards compatible within a major revision, the crosstools
+    // we generate don't care about the minor revision.
+    NdkMajorRevision ndkMajorRevision;
     if (!ndkRelease.isValid) {
-      env.getListener().handle(Event.warn(String.format(
-          "The revision of the Android NDK given in android_ndk_repository rule '%s' could not be "
-          + "determined (the revision string found is '%s'). "
-          + "Defaulting to Android NDK revision %s", ruleName, ndkRelease.rawRelease,
-          AndroidNdkCrosstools.LATEST_KNOWN_REVISION)));
+      String warningMessage =
+          String.format(
+              "The revision of the Android NDK referenced by android_ndk_repository rule '%s' "
+                  + "could not be determined (the revision string found is '%s'). Defaulting to "
+                  + "revision %s.",
+              ruleName, ndkRelease.rawRelease, AndroidNdkCrosstools.LATEST_KNOWN_REVISION.getKey());
+      env.getListener().handle(Event.warn(warningMessage));
+      ndkMajorRevision = AndroidNdkCrosstools.LATEST_KNOWN_REVISION.getValue();
+    } else if (!AndroidNdkCrosstools.isKnownNDKRevision(ndkRelease)) {
+      String warningMessage =
+          String.format(
+              "The major revision of the Android NDK referenced by android_ndk_repository rule "
+                  + "'%s' is %s. The major revisions supported by Bazel are %s. Defaulting to "
+                  + "revision %s.",
+              ruleName,
+              ndkRelease.majorRevision,
+              AndroidNdkCrosstools.KNOWN_NDK_MAJOR_REVISIONS.keySet(),
+              AndroidNdkCrosstools.LATEST_KNOWN_REVISION.getKey());
+      env.getListener().handle(Event.warn(warningMessage));
+      ndkMajorRevision = AndroidNdkCrosstools.LATEST_KNOWN_REVISION.getValue();
+    } else {
+      ndkMajorRevision =
+          AndroidNdkCrosstools.KNOWN_NDK_MAJOR_REVISIONS.get(ndkRelease.majorRevision);
     }
 
-    if (!AndroidNdkCrosstools.isKnownNDKRevision(ndkRelease)) {
-      env.getListener().handle(Event.warn(String.format(
-          "Bazel Android NDK crosstools are based on Android NDK revision %s. "
-          + "The revision of the Android NDK given in android_ndk_repository rule '%s' is '%s'",
-          AndroidNdkCrosstools.LATEST_KNOWN_REVISION, ruleName, ndkRelease.rawRelease)));
-    }
+    ApiLevel apiLevel = ndkMajorRevision.apiLevel(env.getListener(), ruleName, apiLevelString);
 
     ImmutableList.Builder<CrosstoolStlPair> crosstoolsAndStls = ImmutableList.builder();
     try {
@@ -193,13 +204,8 @@ public class AndroidNdkRepositoryFunction extends RepositoryFunction {
       NdkPaths ndkPaths = new NdkPaths(ruleName, hostPlatform, apiLevel);
 
       for (StlImpl stlImpl : StlImpls.get(ndkPaths)) {
-
-        CrosstoolRelease crosstoolRelease = AndroidNdkCrosstools.create(
-            ndkRelease,
-            ndkPaths,
-            stlImpl,
-            hostPlatform);
-
+        CrosstoolRelease crosstoolRelease =
+            ndkMajorRevision.crosstoolRelease(ndkPaths, stlImpl, hostPlatform);
         crosstoolsAndStls.add(new CrosstoolStlPair(crosstoolRelease, stlImpl));
       }
 
@@ -246,7 +252,7 @@ public class AndroidNdkRepositoryFunction extends RepositoryFunction {
       }
 
       String toolchainName = createToolchainName(crosstoolStlPair.stlImpl.getName());
-      
+
       ccToolchainSuites.append(ccToolchainSuiteTemplate
           .replace("%toolchainName%", toolchainName)
           .replace("%toolchainMap%", toolchainMap.toString().trim())
@@ -277,7 +283,7 @@ public class AndroidNdkRepositoryFunction extends RepositoryFunction {
   static String createToolchainName(String stlName) {
     return TOOLCHAIN_NAME_PREFIX + stlName;
   }
-  
+
   private static String createCcToolchainRule(String ccToolchainTemplate, CToolchain toolchain) {
 
     // TODO(bazel-team): It's unfortunate to have to extract data from a CToolchain proto like this.
@@ -345,7 +351,7 @@ public class AndroidNdkRepositoryFunction extends RepositoryFunction {
       // For NDK r10e
       releaseFilePath = directory.getRelative("RELEASE.TXT");
     }
-    
+
     SkyKey releaseFileKey = FileValue.key(RootedPath.toRootedPath(directory, releaseFilePath));
 
     String releaseFileContent;
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/ndkcrosstools/AndroidNdkCrosstools.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/ndkcrosstools/AndroidNdkCrosstools.java
index 187f005028..fab8dfbbce 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/ndkcrosstools/AndroidNdkCrosstools.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/ndkcrosstools/AndroidNdkCrosstools.java
@@ -21,7 +21,7 @@ import com.google.devtools.build.lib.bazel.rules.android.ndkcrosstools.r11.NdkMa
 import com.google.devtools.build.lib.bazel.rules.android.ndkcrosstools.r12.NdkMajorRevisionR12;
 import com.google.devtools.build.lib.bazel.rules.android.ndkcrosstools.r13.NdkMajorRevisionR13;
 import com.google.devtools.build.lib.util.OS;
-import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.CrosstoolRelease;
+import java.util.Map;
 
 /**
  * Helper methods for generating a CrosstoolRelease proto for the Android NDK based on a particular
@@ -38,8 +38,8 @@ public final class AndroidNdkCrosstools {
           "11", new NdkMajorRevisionR11(),
           "12", new NdkMajorRevisionR12(),
           "13", new NdkMajorRevisionR13());
-  public static final String LATEST_KNOWN_REVISION =
-      Iterables.getLast(KNOWN_NDK_MAJOR_REVISIONS.keySet());
+  public static final Map.Entry<String, NdkMajorRevision> LATEST_KNOWN_REVISION =
+      Iterables.getLast(KNOWN_NDK_MAJOR_REVISIONS.entrySet());
 
   /**
    * Exception thrown when there is an error creating the crosstools file.
@@ -50,23 +50,6 @@ public final class AndroidNdkCrosstools {
     }
   }
 
-  public static CrosstoolRelease create(
-      NdkRelease ndkRelease,
-      NdkPaths ndkPaths,
-      StlImpl stlImpl,
-      String hostPlatform) {
-    NdkMajorRevision ndkMajorRevision;
-    if (ndkRelease.isValid) {
-      // NDK minor revisions should be backwards compatible within a major revision, so it should be
-      // enough to check the major revision of the release.
-      ndkMajorRevision = KNOWN_NDK_MAJOR_REVISIONS.get(ndkRelease.majorRevision);
-    } else {
-      // If the NDK revision isn't valid, try using the latest one we know about.
-      ndkMajorRevision = KNOWN_NDK_MAJOR_REVISIONS.get(LATEST_KNOWN_REVISION);
-    }
-    return ndkMajorRevision.crosstoolRelease(ndkPaths, stlImpl, hostPlatform);
-  }
-
   public static String getHostPlatform(NdkRelease ndkRelease) throws NdkCrosstoolsException {
     String hostOs;
     switch (OS.getCurrent()) {
@@ -99,5 +82,5 @@ public final class AndroidNdkCrosstools {
 
   public static boolean isKnownNDKRevision(NdkRelease ndkRelease) {
     return KNOWN_NDK_MAJOR_REVISIONS.containsKey(ndkRelease.majorRevision);
-  } 
+  }
 }
\ No newline at end of file
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/ndkcrosstools/NdkRelease.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/ndkcrosstools/NdkRelease.java
index ec39fdc0da..31540b3d6d 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/ndkcrosstools/NdkRelease.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/ndkcrosstools/NdkRelease.java
@@ -51,14 +51,19 @@ public class NdkRelease {
     }
     String revision = properties.getProperty(REVISION_PROPERTY);
     String[] revisionParsed = revision.split("\\.");
-    return new NdkRelease(
-        revision, // raw revision
-        true, // isValid
-        revisionParsed[0], // major revision
-        revisionParsed[1], // minor revision
-        null, // release candidate
-        true // is64 bit. 32-bit NDKs are provided for only windows.
-    );
+    if (revisionParsed.length < 2) {
+      // Unable to parse Pkg.Revision. Return invalid NdkRelease.
+      return new NdkRelease(revision, false, null, null, null, false);
+    } else {
+      return new NdkRelease(
+          revision, // raw revision
+          true, // isValid
+          revisionParsed[0], // major revision
+          revisionParsed[1], // minor revision
+          null, // release candidate
+          true // is64 bit. 32-bit NDKs are provided for only windows.
+      );
+    }
   }
 
   /**
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/rules/android/AndroidNdkRepositoryTest.java b/src/test/java/com/google/devtools/build/lib/bazel/rules/android/AndroidNdkRepositoryTest.java
index b27efd7fc3..6fc9fbf831 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/rules/android/AndroidNdkRepositoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/rules/android/AndroidNdkRepositoryTest.java
@@ -20,6 +20,7 @@ import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
 import com.google.devtools.build.lib.analysis.FilesToRunProvider;
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
+import com.google.devtools.build.lib.testutil.MoreAsserts;
 import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import org.junit.Before;
@@ -74,4 +75,79 @@ public class AndroidNdkRepositoryTest extends BuildViewTestCase {
         .doesNotContain(
             "src external/androidndk/ndk/platforms/android-22/arch-x86/usr/lib/libandroid.so");
   }
+
+  @Test
+  public void testInvalidNdkReleaseTxt() throws Exception {
+    scratchPlatformsDirectories("arch-x86", 24);
+    FileSystemUtils.appendIsoLatin1(
+        scratch.resolve("WORKSPACE"),
+        "android_ndk_repository(",
+        "    name = 'androidndk',",
+        "    path = '/ndk',",
+        "    api_level = 24,",
+        ")");
+
+    scratch.deleteFile("/ndk/source.properties");
+    scratch.file("/ndk/RELEASE.TXT", "not a valid release string");
+
+    invalidatePackages();
+
+    assertThat(getConfiguredTarget("@androidndk//:files")).isNotNull();
+    MoreAsserts.assertContainsEvent(
+        eventCollector,
+        "The revision of the Android NDK referenced by android_ndk_repository rule 'androidndk' "
+            + "could not be determined (the revision string found is 'not a valid release string')."
+            + " Defaulting to revision 13.");
+  }
+
+  @Test
+  public void testInvalidNdkSourceProperties() throws Exception {
+    scratchPlatformsDirectories("arch-x86", 24);
+    FileSystemUtils.appendIsoLatin1(
+        scratch.resolve("WORKSPACE"),
+        "android_ndk_repository(",
+        "    name = 'androidndk',",
+        "    path = '/ndk',",
+        "    api_level = 24,",
+        ")");
+
+    scratch.overwriteFile(
+        "/ndk/source.properties",
+        "Pkg.Desc = Android NDK",
+        "Pkg.Revision = invalid package revision");
+
+    invalidatePackages();
+
+    assertThat(getConfiguredTarget("@androidndk//:files")).isNotNull();
+    MoreAsserts.assertContainsEvent(
+        eventCollector,
+        "The revision of the Android NDK referenced by android_ndk_repository rule 'androidndk' "
+            + "could not be determined (the revision string found is 'invalid package revision'). "
+            + "Defaulting to revision 13.");
+  }
+
+  @Test
+  public void testUnsupportedNdkVersion() throws Exception {
+    scratchPlatformsDirectories("arch-x86", 24);
+    FileSystemUtils.appendIsoLatin1(
+        scratch.resolve("WORKSPACE"),
+        "android_ndk_repository(",
+        "    name = 'androidndk',",
+        "    path = '/ndk',",
+        "    api_level = 24,",
+        ")");
+
+    scratch.overwriteFile(
+        "/ndk/source.properties",
+        "Pkg.Desc = Android NDK",
+        "Pkg.Revision = 14.0.3675639-beta2");
+    invalidatePackages();
+
+    assertThat(getConfiguredTarget("@androidndk//:files")).isNotNull();
+    MoreAsserts.assertContainsEvent(
+        eventCollector,
+        "The major revision of the Android NDK referenced by android_ndk_repository rule "
+            + "'androidndk' is 14. The major revisions supported by Bazel are [10, 11, 12, 13]. "
+            + "Defaulting to revision 13.");
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/rules/android/ndkcrosstools/AndroidNdkCrosstoolsTest.java b/src/test/java/com/google/devtools/build/lib/bazel/rules/android/ndkcrosstools/AndroidNdkCrosstoolsTest.java
index 09c369904d..c2d85ff144 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/rules/android/ndkcrosstools/AndroidNdkCrosstoolsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/rules/android/ndkcrosstools/AndroidNdkCrosstoolsTest.java
@@ -73,6 +73,10 @@ public class AndroidNdkCrosstoolsTest {
       this.ndkDirectoriesFilename = ndkDirectoriesFilename;
     }
 
+    NdkMajorRevision getNdkMajorRevision() {
+      return AndroidNdkCrosstools.KNOWN_NDK_MAJOR_REVISIONS.get(ndkRelease.majorRevision);
+    }
+
     ImmutableSet<String> getNdkFiles() throws IOException {
       String ndkFilesFileContent =
           ResourceFileLoader.loadResource(AndroidNdkCrosstoolsTest.class, ndkFilesFilename);
@@ -134,7 +138,7 @@ public class AndroidNdkCrosstoolsTest {
     for (StlImpl ndkStlImpl : StlImpls.get(ndkPaths)) {
       // Protos are immutable, so this can be shared between tests.
       CrosstoolRelease crosstool =
-          AndroidNdkCrosstools.create(params.ndkRelease, ndkPaths, ndkStlImpl, HOST_PLATFORM);
+          params.getNdkMajorRevision().crosstoolRelease(ndkPaths, ndkStlImpl, HOST_PLATFORM);
       crosstools.add(crosstool);
       stlFilegroupsBuilder.putAll(ndkStlImpl.getFilegroupNamesAndFilegroupFileGlobPatterns());
     }
@@ -144,18 +148,18 @@ public class AndroidNdkCrosstoolsTest {
     ndkFiles = params.getNdkFiles();
     ndkDirectories = params.getNdkDirectories();
   }
-  
+
   @Test
   public void testPathsExist() throws Exception {
 
     for (CrosstoolRelease crosstool : crosstoolReleases) {
       for (CToolchain toolchain : crosstool.getToolchainList()) {
-  
+
         // Test that all tool paths exist.
         for (ToolPath toolpath : toolchain.getToolPathList()) {
           assertThat(ndkFiles).contains(toolpath.getPath());
         }
-  
+
         // Test that all cxx_builtin_include_directory paths exist.
         for (String includeDirectory : toolchain.getCxxBuiltinIncludeDirectoryList()) {
           // Special case for builtin_sysroot.
@@ -164,13 +168,13 @@ public class AndroidNdkCrosstoolsTest {
             assertThat(ndkDirectories).contains(path);
           }
         }
-  
+
         // Test that the builtin_sysroot path exists.
         {
           String builtinSysroot = NdkPaths.stripRepositoryPrefix(toolchain.getBuiltinSysroot());
           assertThat(ndkDirectories).contains(builtinSysroot);
         }
-  
+
         // Test that all include directories added through unfiltered_cxx_flag exist.
         for (String flag : toolchain.getUnfilteredCxxFlagList()) {
           if (!flag.equals("-isystem")) {