Add platform mapping function.

Introduces a new SkyFunction which reads a platform mapping file, parses its contents and produces a platform mapping sky value which can then be used to apply the mapping to configurations (in the form of BuildConfigurationValue.Key). The file's location is obtained from the newly introduced flag --platform_mappings and defaults to //:platform_mappings. Note that this logic is not in use anywhere yet because the key mapping has not been applied. This will follow in a future CL. Step 4/N towards the platforms mapping functionality for bazelbuild/bazel#6426 RELNOTES: None. PiperOrigin-RevId: 239043475
luca-digrazia · Sep 4, 2022 · 17d8c49 · 17d8c49
1 parent 808a67d
commit 17d8c49
Show file tree

Hide file tree

Showing 8 changed files with 50 additions and 146 deletions.
diff --git a/...azelbuild.bazel/src/main/java/com/google/devtools/build/lib/analysis/PlatformOptions.java b/...azelbuild.bazel/src/main/java/com/google/devtools/build/lib/analysis/PlatformOptions.java
@@ -38,6 +38,8 @@ public class PlatformOptions extends FragmentOptions {
       Label.parseAbsoluteUnchecked("@bazel_tools//platforms:host_platform");
   public static final Label DEFAULT_HOST_PLATFORM =
       Label.parseAbsoluteUnchecked("@local_config_platform//:host");
+  public static final Label LEGACY_DEFAULT_TARGET_PLATFORM =
+      Label.parseAbsoluteUnchecked("@bazel_tools//platforms:target_platform");
 
   /**
    * Main workspace-relative location to use when the user does not explicitly set {@code
@@ -90,21 +92,6 @@ public class PlatformOptions extends FragmentOptions {
               + "command.")
   public List<Label> platforms;
 
-  @Option(
-      name = "target_platform_fallback",
-      converter = BuildConfiguration.EmptyToNullLabelConverter.class,
-      defaultValue = "@bazel_tools//platforms:target_platform",
-      documentationCategory = OptionDocumentationCategory.TOOLCHAIN,
-      effectTags = {
-        OptionEffectTag.AFFECTS_OUTPUTS,
-        OptionEffectTag.CHANGES_INPUTS,
-        OptionEffectTag.LOADING_AND_ANALYSIS
-      },
-      help =
-          "The label of a platform rule that should be used if no target platform is set and no"
-              + " platform mapping matches the current set of flags.")
-  public Label targetPlatformFallback;
-
   @Option(
       name = "extra_toolchains",
       defaultValue = "",
@@ -182,15 +169,10 @@ public class PlatformOptions extends FragmentOptions {
   public boolean autoConfigureHostPlatform;
 
   @Option(
-      name = "incompatible_use_toolchain_resolution_for_java_rules",
-      oldName = "experimental_use_toolchain_resolution_for_java_rules",
+      name = "experimental_use_toolchain_resolution_for_java_rules",
       defaultValue = "false",
       documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
       effectTags = OptionEffectTag.UNKNOWN,
-      metadataTags = {
-        OptionMetadataTag.INCOMPATIBLE_CHANGE,
-        OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
-      },
       help =
           "If set to true, toolchain resolution will be used to resolve java_toolchain and"
               + " java_runtime.")
@@ -242,7 +224,7 @@ public Label computeTargetPlatform() {
       return computeHostPlatform();
     } else {
       // Use the legacy target platform
-      return targetPlatformFallback;
+      return LEGACY_DEFAULT_TARGET_PLATFORM;
     }
   }
 

diff --git a/...d.bazel/src/main/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunction.java b/...d.bazel/src/main/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunction.java
@@ -20,12 +20,12 @@
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.actions.FileValue;
 import com.google.devtools.build.lib.actions.MissingInputFileException;
+import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.events.Location;
-import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.Root;
@@ -52,6 +52,12 @@
  */
 public class PlatformMappingFunction implements SkyFunction {
 
+  private final BlazeDirectories blazeDirectories;
+
+  public PlatformMappingFunction(BlazeDirectories blazeDirectories) {
+    this.blazeDirectories = blazeDirectories;
+  }
+
   @Nullable
   @Override
   public SkyValue compute(SkyKey skyKey, Environment env)
@@ -60,8 +66,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
     PathFragment workspaceRelativeMappingPath =
         platformMappingKey.getWorkspaceRelativeMappingPath();
 
-    PathPackageLocator pkgLocator = PrecomputedValue.PATH_PACKAGE_LOCATOR.get(env);
-    Root workspaceRoot = Root.fromPath(pkgLocator.getWorkspaceFile().getParentDirectory());
+    Root workspaceRoot = Root.fromPath(blazeDirectories.getWorkspace());
     RootedPath rootedMappingPath =
         RootedPath.toRootedPath(workspaceRoot, workspaceRelativeMappingPath);
     FileValue fileValue = (FileValue) env.getValue(FileValue.key(rootedMappingPath));
@@ -180,12 +185,7 @@ private Map<Label, Collection<String>> platformsToFlags() throws PlatformMapping
         platformsToFlags.put(platform, flags);
       }
 
-      try {
-        return platformsToFlags.build();
-      } catch (IllegalArgumentException e) {
-        throw throwParsingException(
-            e, "Got duplicate platform entries but each platform key must be unique");
-      }
+      return platformsToFlags.build();
     }
 
     private Label platform() throws PlatformMappingException {
@@ -202,7 +202,9 @@ private Label platform() throws PlatformMappingException {
         // mappings however only apply within a repository imported by the root repository.
         platform = Label.parseAbsolute(label, emptyRepositoryMapping);
       } catch (LabelSyntaxException e) {
-        throw throwParsingException(e, "Expected platform label but got " + label);
+        throw new PlatformMappingException(
+            new PlatformMappingParsingException("Expected platform label but got " + label, e),
+            SkyFunctionException.Transience.PERSISTENT);
       }
       return platform;
     }
@@ -232,12 +234,7 @@ private Map<Collection<String>, Label> flagsToPlatforms() throws PlatformMapping
         Label platform = platform();
         flagsToPlatforms.put(flags, platform);
       }
-      try {
-        return flagsToPlatforms.build();
-      } catch (IllegalArgumentException e) {
-        throw throwParsingException(
-            e, "Got duplicate flags entries but each flags key must be unique");
-      }
+      return flagsToPlatforms.build();
     }
 
     private String consume() {
@@ -254,13 +251,6 @@ private String peek() {
       return line.get();
     }
 
-    private AssertionError throwParsingException(Exception e, String message)
-        throws PlatformMappingException {
-      throw new PlatformMappingException(
-          new PlatformMappingParsingException(message, e),
-          SkyFunctionException.Transience.PERSISTENT);
-    }
-
     private void throwParsingException(String message) throws PlatformMappingException {
       throw new PlatformMappingException(
           new PlatformMappingParsingException(message), SkyFunctionException.Transience.PERSISTENT);

diff --git a/...uild.bazel/src/main/java/com/google/devtools/build/lib/skyframe/PlatformMappingValue.java b/...uild.bazel/src/main/java/com/google/devtools/build/lib/skyframe/PlatformMappingValue.java
@@ -15,8 +15,6 @@
 package com.google.devtools.build.lib.skyframe;
 
 import com.google.common.base.Preconditions;
-import com.google.common.cache.Cache;
-import com.google.common.cache.CacheBuilder;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Interner;
@@ -38,7 +36,6 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
-import java.util.concurrent.ExecutionException;
 import javax.annotation.Nullable;
 
 /**
@@ -135,7 +132,6 @@ public String toString() {
 
   private final Map<Label, Collection<String>> platformsToFlags;
   private final Map<Collection<String>, Label> flagsToPlatforms;
-  private final transient Cache<Collection<String>, OptionsParsingResult> parserCache;
 
   /**
    * Creates a new mapping value which will match on the given platforms (if a target platform is
@@ -151,10 +147,6 @@ public String toString() {
       Map<Collection<String>, Label> flagsToPlatforms) {
     this.platformsToFlags = platformsToFlags;
     this.flagsToPlatforms = flagsToPlatforms;
-    this.parserCache =
-        CacheBuilder.newBuilder()
-            .initialCapacity(platformsToFlags.size() + flagsToPlatforms.size())
-            .build();
   }
 
   /**
@@ -205,14 +197,13 @@ public BuildConfigurationValue.Key map(
         return original;
       }
 
-      modifiedOptions =
-          originalOptions.applyParsingResult(
-              parseWithCache(platformsToFlags.get(targetPlatform), defaultBuildOptions));
+      OptionsParsingResult parsingResult =
+          parse(platformsToFlags.get(targetPlatform), defaultBuildOptions);
+      modifiedOptions = originalOptions.applyParsingResult(parsingResult);
     } else {
       boolean mappingFound = false;
       for (Map.Entry<Collection<String>, Label> flagsToPlatform : flagsToPlatforms.entrySet()) {
-        if (originalOptions.matches(
-            parseWithCache(flagsToPlatform.getKey(), defaultBuildOptions))) {
+        if (originalOptions.matches(parse(flagsToPlatform.getKey(), defaultBuildOptions))) {
           modifiedOptions = originalOptions.clone();
           modifiedOptions.get(PlatformOptions.class).platforms =
               ImmutableList.of(flagsToPlatform.getValue());
@@ -233,15 +224,6 @@ public BuildConfigurationValue.Key map(
         BuildOptions.diffForReconstruction(defaultBuildOptions, modifiedOptions));
   }
 
-  private OptionsParsingResult parseWithCache(
-      Collection<String> args, BuildOptions defaultBuildOptions) throws OptionsParsingException {
-    try {
-      return parserCache.get(args, () -> parse(args, defaultBuildOptions));
-    } catch (ExecutionException e) {
-      throw (OptionsParsingException) e.getCause();
-    }
-  }
-
   private OptionsParsingResult parse(Iterable<String> args, BuildOptions defaultBuildOptions)
       throws OptionsParsingException {
     OptionsParser parser = OptionsParser.newOptionsParser(defaultBuildOptions.getFragmentClasses());

diff --git a/...a/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java b/...a/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java
@@ -136,8 +136,6 @@ public final class SkyFunctions {
       SkyFunctionName.createHermetic("REGISTERED_EXECUTION_PLATFORMS");
   static final SkyFunctionName REGISTERED_TOOLCHAINS =
       SkyFunctionName.createHermetic("REGISTERED_TOOLCHAINS");
-  static final SkyFunctionName SINGLE_TOOLCHAIN_RESOLUTION =
-      SkyFunctionName.createHermetic("SINGLE_TOOLCHAIN_RESOLUTION");
   static final SkyFunctionName TOOLCHAIN_RESOLUTION =
       SkyFunctionName.createHermetic("TOOLCHAIN_RESOLUTION");
   public static final SkyFunctionName REPOSITORY_MAPPING =

diff --git a/...zelbuild.bazel/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/...zelbuild.bazel/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -78,6 +78,7 @@
 import com.google.devtools.build.lib.analysis.config.FragmentClassSet;
 import com.google.devtools.build.lib.analysis.config.HostTransition;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
+import com.google.devtools.build.lib.analysis.config.transitions.ComposingTransition;
 import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition;
 import com.google.devtools.build.lib.analysis.config.transitions.NoTransition;
 import com.google.devtools.build.lib.analysis.config.transitions.NullTransition;
@@ -1925,22 +1926,19 @@ public Multimap<Dependency, BuildConfiguration> getConfigurations(
         if (key.getTransition() == NullTransition.INSTANCE) {
           continue;
         }
-        List<BuildOptions> toOptions =
-            ConfigurationResolver.applyTransition(
-                fromOptions, key.getTransition(), depFragments, ruleClassProvider, true);
-        for (BuildOptions toOption : toOptions) {
+        for (BuildOptions toOptions : ConfigurationResolver.applyTransition(
+            fromOptions, key.getTransition(), depFragments, ruleClassProvider, true)) {
+          StarlarkTransition.postProcessStarlarkTransitions(eventHandler, key.getTransition());
           configSkyKeys.add(
               BuildConfigurationValue.key(
-                  depFragments, BuildOptions.diffForReconstruction(defaultBuildOptions, toOption)));
+                  depFragments,
+                  BuildOptions.diffForReconstruction(defaultBuildOptions, toOptions)));
         }
-        ImmutableSet<SkyKey> buildSettingPackageKeys =
-            StarlarkTransition.getBuildSettingPackageKeys(key.getTransition());
-        EvaluationResult<SkyValue> buildSettingsResult =
-            evaluateSkyKeys(eventHandler, buildSettingPackageKeys, true);
-        ImmutableMap.Builder<SkyKey, SkyValue> buildSettingValues = new ImmutableMap.Builder<>();
-        buildSettingPackageKeys.forEach(k -> buildSettingValues.put(k, buildSettingsResult.get(k)));
-        StarlarkTransition.validate(
-            key.getTransition(), buildSettingValues.build(), toOptions, eventHandler);
+        ImmutableList<ConfigurationTransition> transitions =
+            ComposingTransition.decomposeTransition(key.getTransition());
+        transitions.stream()
+            .filter(t -> t instanceof StarlarkTransition)
+            .forEach(t -> ((StarlarkTransition) t).replayOn(eventHandler));
       }
     }
     EvaluationResult<SkyValue> configsResult =

diff --git a/...c/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionParserTest.java b/...c/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionParserTest.java
@@ -15,7 +15,7 @@
 package com.google.devtools.build.lib.skyframe;
 
 import static com.google.common.truth.Truth.assertThat;
-import static org.junit.Assert.assertThrows;
+import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
@@ -274,16 +274,17 @@ public void testParseUnknownSection() throws Exception {
 
     assertThat(exception).hasMessageThat().contains("platform:");
 
-    assertThrows(
-        PlatformMappingFunction.PlatformMappingException.class,
-        () ->
-            parse(
-                "platforms:",
-                "  //platforms:one",
-                "    --cpu=one",
-                "flag:",
-                "  --cpu=one",
-                "    //platforms:one"));
+    PlatformMappingFunction.PlatformMappingException exception2 =
+        assertThrows(
+            PlatformMappingFunction.PlatformMappingException.class,
+            () ->
+                parse(
+                    "platforms:",
+                    "  //platforms:one",
+                    "    --cpu=one",
+                    "flag:",
+                    "  --cpu=one",
+                    "    //platforms:one"));
 
     assertThat(exception).hasMessageThat().contains("platform");
   }
@@ -346,52 +347,7 @@ public void testParseFlagsInvalidFlag() throws Exception {
     assertThat(exception).hasMessageThat().contains("-cpu");
   }
 
-  @Test
-  public void testParsePlatformsDuplicatePlatform() throws Exception {
-    PlatformMappingFunction.PlatformMappingException exception =
-        assertThrows(
-            PlatformMappingFunction.PlatformMappingException.class,
-            () ->
-                parse(
-                    "platforms:", // Force line break
-                    "  //platforms:one", // Force line break
-                    "    --cpu=one", // Force line break
-                    "  //platforms:one", // Force line break
-                    "    --cpu=two"));
-
-    assertThat(exception).hasMessageThat().contains("duplicate");
-    assertThat(exception)
-        .hasCauseThat()
-        .hasCauseThat()
-        .hasMessageThat()
-        .contains("//platforms:one");
-  }
-
-  @Test
-  public void testParseFlagsDuplicateFlags() throws Exception {
-    PlatformMappingFunction.PlatformMappingException exception =
-        assertThrows(
-            PlatformMappingFunction.PlatformMappingException.class,
-            () ->
-                parse(
-                    "flags:", // Force line break
-                    "  --compilation_mode=dbg", // Force line break
-                    "  --cpu=one", // Force line break
-                    "    //platforms:one", // Force line break
-                    "  --compilation_mode=dbg", // Force line break
-                    "  --cpu=one", // Force line break
-                    "    //platforms:two"));
-
-    assertThat(exception).hasMessageThat().contains("duplicate");
-    assertThat(exception).hasCauseThat().hasCauseThat().hasMessageThat().contains("--cpu=one");
-    assertThat(exception)
-        .hasCauseThat()
-        .hasCauseThat()
-        .hasMessageThat()
-        .contains("--compilation_mode=dbg");
-  }
-
-  private static PlatformMappingFunction.Mappings parse(String... lines)
+  private PlatformMappingFunction.Mappings parse(String... lines)
       throws PlatformMappingFunction.PlatformMappingException {
     return new PlatformMappingFunction.Parser(ImmutableList.copyOf(lines).iterator()).parse();
   }

diff --git a/...zel/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionTest.java b/...zel/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionTest.java
@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.skyframe;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.analysis.PlatformOptions.LEGACY_DEFAULT_TARGET_PLATFORM;
 import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
 
 import com.google.common.base.Optional;
@@ -63,8 +64,6 @@ public class PlatformMappingFunctionTest extends BuildViewTestCase {
   private static final BuildOptions.OptionsDiffForReconstruction EMPTY_DIFF =
       BuildOptions.diffForReconstruction(
           DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS, DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS);
-  private static final Label DEFAULT_TARGET_PLATFORM =
-      Label.parseAbsoluteUnchecked("@bazel_tools//platforms:target_platform");
 
   @Test
   public void testMappingFileDoesNotExist() throws Exception {
@@ -89,7 +88,7 @@ public void testMappingFileDoesNotExistDefaultLocation() throws Exception {
         platformMappingValue.map(key, DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS);
 
     assertThat(toMappedOptions(mapped).get(PlatformOptions.class).platforms)
-        .containsExactly(DEFAULT_TARGET_PLATFORM);
+        .containsExactly(LEGACY_DEFAULT_TARGET_PLATFORM);
   }
 
   @Test