Add better exception support to PlatformMappingFunction.

Work towards platform-based flags: #19409. PiperOrigin-RevId: 598953443 Change-Id: I55cd314ee644934032a39d7d23dd9a742fe2c0f7
bazelbuild · Jan 16, 2024 · 4a6f896 · 4a6f896
1 parent 611fdbb
commit 4a6f896
Show file tree

Hide file tree

Showing 7 changed files with 184 additions and 105 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/config/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/config/BUILD
@@ -20,6 +20,7 @@ java_library(
     ],
     deps = [
         ":config",
+        ":exceptions",
         "//src/main/java/com/google/devtools/build/lib:starlark_options_parser",
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
@@ -62,6 +63,19 @@ java_library(
     ],
 )
 
+java_library(
+    name = "exceptions",
+    srcs = [
+        "PlatformMappingException.java",
+        "PlatformMappingParsingException.java",
+    ],
+    deps = [
+        "//src/main/java/com/google/devtools/build/lib/actions",
+        "//src/main/java/com/google/devtools/build/skyframe",
+        "//src/main/java/com/google/devtools/common/options",
+    ],
+)
+
 java_library(
     name = "config",
     srcs = [

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingException.java b/src/main/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingException.java
@@ -0,0 +1,32 @@
+// Copyright 2024 The Bazel Authors. All rights reserved.
+//
+// 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.google.devtools.build.lib.skyframe.config;
+
+import com.google.devtools.build.lib.actions.MissingInputFileException;
+import java.io.IOException;
+
+/** Exception class for errors while computing platform mappings. */
+public final class PlatformMappingException extends Exception {
+  PlatformMappingException(MissingInputFileException cause) {
+    super(cause);
+  }
+
+  PlatformMappingException(IOException cause) {
+    super(cause);
+  }
+
+  PlatformMappingException(PlatformMappingParsingException cause) {
+    super(cause);
+  }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingFunction.java
@@ -50,7 +50,6 @@
 import com.google.devtools.build.lib.vfs.RootedPath;
 import com.google.devtools.build.skyframe.SkyFunction;
 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.common.options.OptionsParser;
 import com.google.devtools.common.options.OptionsParsingException;
@@ -77,7 +76,7 @@ public PlatformMappingFunction(ImmutableSet<Class<? extends FragmentOptions>> op
   @Nullable
   @Override
   public PlatformMappingValue compute(SkyKey skyKey, Environment env)
-      throws PlatformMappingException, InterruptedException {
+      throws PlatformMappingFunctionException, InterruptedException {
     PlatformMappingValue.Key platformMappingKey = (PlatformMappingValue.Key) skyKey.argument();
     PathFragment workspaceRelativeMappingPath =
         platformMappingKey.getWorkspaceRelativeMappingPath();
@@ -107,7 +106,7 @@ public PlatformMappingValue compute(SkyKey skyKey, Environment env)
         continue;
       }
       if (fileValue.isDirectory()) {
-        throw new PlatformMappingException(
+        throw new PlatformMappingFunctionException(
             new MissingInputFileException(
                 createFailureDetail(
                     String.format(
@@ -122,13 +121,18 @@ public PlatformMappingValue compute(SkyKey skyKey, Environment env)
       try {
         lines = FileSystemUtils.readLines(fileValue.realRootedPath().asPath(), UTF_8);
       } catch (IOException e) {
-        throw new PlatformMappingException(e);
+        throw new PlatformMappingFunctionException(e);
       }
 
-      Mappings parsed = parse(env, lines, mainRepoContext);
+      Mappings parsed;
+      try {
+        parsed = parse(env, lines, mainRepoContext);
       if (parsed == null) {
         return null;
       }
+      } catch (PlatformMappingParsingException e) {
+        throw new PlatformMappingFunctionException(e);
+      }
       return parsed.toPlatformMappingValue(optionsClasses);
     }
 
@@ -138,7 +142,7 @@ public PlatformMappingValue compute(SkyKey skyKey, Environment env)
       return new PlatformMappingValue(
           ImmutableMap.of(), ImmutableMap.of(), ImmutableSet.of(), mainRepoContext.repoMapping());
     }
-    throw new PlatformMappingException(
+    throw new PlatformMappingFunctionException(
         new MissingInputFileException(
             createFailureDetail(
                 String.format(
@@ -156,31 +160,11 @@ private static FailureDetail createFailureDetail(String message, Code detailedCo
         .build();
   }
 
-  @VisibleForTesting
-  static final class PlatformMappingException extends SkyFunctionException {
-
-    PlatformMappingException(OptionsParsingException cause) {
-      super(cause, Transience.PERSISTENT);
-    }
-
-    PlatformMappingException(MissingInputFileException cause) {
-      super(cause, Transience.PERSISTENT);
-    }
-
-    PlatformMappingException(IOException cause) {
-      super(cause, Transience.TRANSIENT);
-    }
-
-    PlatformMappingException(PlatformMappingParsingException cause) {
-      super(cause, Transience.PERSISTENT);
-    }
-  }
-
   /** Parses the given lines, returns null if not all Skyframe deps are ready. */
   @VisibleForTesting
   @Nullable
   static Mappings parse(Environment env, List<String> lines, RepoContext mainRepoContext)
-      throws PlatformMappingException {
+      throws PlatformMappingParsingException {
     PeekingIterator<String> it =
         Iterators.peekingIterator(
             lines.stream()
@@ -267,7 +251,7 @@ public Target loadBuildSetting(String name)
   @Nullable
   private static NativeAndStarlarkFlags parseStarlarkFlags(
       ImmutableSet<String> rawFlags, Environment env, RepoContext mainRepoContext)
-      throws PlatformMappingException {
+      throws PlatformMappingParsingException {
     ImmutableSet.Builder<String> nativeFlags = ImmutableSet.builder();
     ImmutableList.Builder<String> starlarkFlags = ImmutableList.builder();
     for (String flagSetting : rawFlags) {
@@ -287,11 +271,11 @@ private static NativeAndStarlarkFlags parseStarlarkFlags(
       if (!starlarkFlagParser.parseGivenArgs(starlarkFlags.build())) {
         return null;
       }
-      return NativeAndStarlarkFlags.create(
-          nativeFlags.build(), fakeNativeParser.getStarlarkOptions());
     } catch (OptionsParsingException e) {
-      throw new PlatformMappingException(e);
+      throw new PlatformMappingParsingException(e);
     }
+      return NativeAndStarlarkFlags.create(
+          nativeFlags.build(), fakeNativeParser.getStarlarkOptions());
   }
 
   /**
@@ -300,7 +284,7 @@ private static NativeAndStarlarkFlags parseStarlarkFlags(
   @Nullable
   private static ImmutableMap<Label, NativeAndStarlarkFlags> readPlatformsToFlags(
       PeekingIterator<String> it, Environment env, RepoContext mainRepoContext)
-      throws PlatformMappingException {
+      throws PlatformMappingParsingException {
     ImmutableMap.Builder<Label, NativeAndStarlarkFlags> platformsToFlags = ImmutableMap.builder();
     boolean needSkyframeDeps = false;
     while (it.hasNext() && !it.peek().equalsIgnoreCase("flags:")) {
@@ -327,7 +311,8 @@ private static ImmutableMap<Label, NativeAndStarlarkFlags> readPlatformsToFlags(
   }
 
   private static ImmutableMap<ImmutableSet<String>, Label> readFlagsToPlatforms(
-      PeekingIterator<String> it, RepoContext mainRepoContext) throws PlatformMappingException {
+      PeekingIterator<String> it, RepoContext mainRepoContext)
+      throws PlatformMappingParsingException {
     ImmutableMap.Builder<ImmutableSet<String>, Label> flagsToPlatforms = ImmutableMap.builder();
     while (it.hasNext() && it.peek().startsWith("--")) {
       ImmutableSet<String> flags = readFlags(it);
@@ -343,7 +328,7 @@ private static ImmutableMap<ImmutableSet<String>, Label> readFlagsToPlatforms(
   }
 
   private static Label readPlatform(PeekingIterator<String> it, RepoContext mainRepoContext)
-      throws PlatformMappingException {
+      throws PlatformMappingParsingException {
     if (!it.hasNext()) {
       throw parsingException("Expected platform label but got end of file");
     }
@@ -357,7 +342,7 @@ private static Label readPlatform(PeekingIterator<String> it, RepoContext mainRe
   }
 
   private static ImmutableSet<String> readFlags(PeekingIterator<String> it)
-      throws PlatformMappingException {
+      throws PlatformMappingParsingException {
     ImmutableSet.Builder<String> flags = ImmutableSet.builder();
     // Note: Short form flags are not supported.
     while (it.hasNext() && it.peek().startsWith("--")) {
@@ -373,12 +358,12 @@ private static ImmutableSet<String> readFlags(PeekingIterator<String> it)
     return parsedFlags;
   }
 
-  private static PlatformMappingException parsingException(String message) {
+  private static PlatformMappingParsingException parsingException(String message) {
     return parsingException(message, /*cause=*/ null);
   }
 
-  private static PlatformMappingException parsingException(String message, Exception cause) {
-    return new PlatformMappingException(new PlatformMappingParsingException(message, cause));
+  private static PlatformMappingParsingException parsingException(String message, Exception cause) {
+    return new PlatformMappingParsingException(message, cause);
   }
 
   /**
@@ -406,14 +391,19 @@ PlatformMappingValue toPlatformMappingValue(
     }
   }
 
-  /**
-   * Inner wrapper exception to work around the fact that {@link SkyFunctionException} cannot carry
-   * a message of its own.
-   */
-  private static final class PlatformMappingParsingException extends Exception {
+  @VisibleForTesting
+  static final class PlatformMappingFunctionException extends SkyFunctionException {
+
+    PlatformMappingFunctionException(MissingInputFileException cause) {
+      super(new PlatformMappingException(cause), Transience.PERSISTENT);
+    }
+
+    PlatformMappingFunctionException(IOException cause) {
+      super(new PlatformMappingException(cause), Transience.TRANSIENT);
+    }
 
-    PlatformMappingParsingException(String message, Throwable cause) {
-      super(message, cause);
+    PlatformMappingFunctionException(PlatformMappingParsingException cause) {
+      super(new PlatformMappingException(cause), Transience.PERSISTENT);
     }
   }
 }
diff --git a/...n/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingParsingException.java b/...n/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingParsingException.java
@@ -0,0 +1,32 @@
+// Copyright 2024 The Bazel Authors. All rights reserved.
+//
+// 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.google.devtools.build.lib.skyframe.config;
+
+import com.google.devtools.build.skyframe.SkyFunctionException;
+import com.google.devtools.common.options.OptionsParsingException;
+
+/**
+ * Inner wrapper exception to work around the fact that {@link SkyFunctionException} cannot carry a
+ * message of its own.
+ */
+final class PlatformMappingParsingException extends Exception {
+
+  PlatformMappingParsingException(String message, Throwable cause) {
+    super(message, cause);
+  }
+
+  public PlatformMappingParsingException(OptionsParsingException cause) {
+    super(cause);
+  }
+}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/config/BUILD b/src/test/java/com/google/devtools/build/lib/skyframe/config/BUILD
@@ -27,6 +27,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value",
         "//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_cluster",
         "//src/main/java/com/google/devtools/build/lib/skyframe/config",
+        "//src/main/java/com/google/devtools/build/lib/skyframe/config:exceptions",
         "//src/main/java/com/google/devtools/build/lib/skyframe/config:sky_functions",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
         "//src/main/java/com/google/devtools/build/skyframe",