Make PlatformMappingFunction properly package-path aware and do a couple of other clean-ups: * Make exceptions thrown because of I/O errors transient, since this SkyFunction is the one responsible for them. * Add #equals and #hashCode for PlatformMappingsValue (and #toString). This way, a change in the file that doesn't affect the value of the PlatformMappingValue will be change-pruned. Fixes #8288. PiperOrigin-RevId: 247971816
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunction.java index b701661..953f48e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunction.java
@@ -16,6 +16,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.actions.FileValue; @@ -33,7 +34,6 @@ import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunctionException; import com.google.devtools.build.skyframe.SkyKey; -import com.google.devtools.build.skyframe.SkyValue; import java.io.IOException; import java.nio.charset.StandardCharsets; import java.util.Collection; @@ -54,56 +54,63 @@ @Nullable @Override - public SkyValue compute(SkyKey skyKey, Environment env) + public PlatformMappingValue compute(SkyKey skyKey, Environment env) throws PlatformMappingException, InterruptedException { PlatformMappingValue.Key platformMappingKey = (PlatformMappingValue.Key) skyKey.argument(); PathFragment workspaceRelativeMappingPath = platformMappingKey.getWorkspaceRelativeMappingPath(); PathPackageLocator pkgLocator = PrecomputedValue.PATH_PACKAGE_LOCATOR.get(env); - Root workspaceRoot = Root.fromPath(pkgLocator.getWorkspaceFile().getParentDirectory()); - RootedPath rootedMappingPath = - RootedPath.toRootedPath(workspaceRoot, workspaceRelativeMappingPath); - FileValue fileValue = (FileValue) env.getValue(FileValue.key(rootedMappingPath)); - if (fileValue == null) { + if (pkgLocator == null) { return null; } - if (!fileValue.exists()) { - if (!platformMappingKey.wasExplicitlySetByUser()) { - // If no flag was passed and the default mapping file does not exist treat this as if the - // mapping file was empty rather than an error. - return PlatformMappingValue.EMPTY; + ImmutableList<Root> pathEntries = pkgLocator.getPathEntries(); + for (Root root : pathEntries) { + RootedPath rootedMappingPath = RootedPath.toRootedPath(root, workspaceRelativeMappingPath); + FileValue fileValue = (FileValue) env.getValue(FileValue.key(rootedMappingPath)); + if (fileValue == null) { + return null; } - throw new PlatformMappingException( - new MissingInputFileException( - String.format( - "--platform_mappings was set to '%s' but no such file exists relative to the " - + "top-level workspace, '%s'", - workspaceRelativeMappingPath, workspaceRoot), - Location.BUILTIN), - SkyFunctionException.Transience.PERSISTENT); - } - if (fileValue.isDirectory()) { - throw new PlatformMappingException( - new MissingInputFileException( - String.format( - "--platform_mappings was set to '%s' relative to the top-level workspace '%s' but" - + "that path refers to a directory, not a file", - workspaceRelativeMappingPath, workspaceRoot), - Location.BUILTIN), - SkyFunctionException.Transience.PERSISTENT); + + if (!fileValue.exists()) { + continue; + } + if (fileValue.isDirectory()) { + throw new PlatformMappingException( + new MissingInputFileException( + String.format( + "--platform_mappings was set to '%s' relative to the top-level workspace '%s'" + + " but that path refers to a directory, not a file", + workspaceRelativeMappingPath, root), + Location.BUILTIN), + SkyFunctionException.Transience.PERSISTENT); + } + + Iterable<String> lines; + try { + lines = + FileSystemUtils.readLines(fileValue.realRootedPath().asPath(), StandardCharsets.UTF_8); + } catch (IOException e) { + throw new PlatformMappingException(e, SkyFunctionException.Transience.TRANSIENT); + } + + return new Parser(lines.iterator()).parse().toPlatformMappingValue(); } - Iterable<String> lines; - try { - lines = - FileSystemUtils.readLines(fileValue.realRootedPath().asPath(), StandardCharsets.UTF_8); - } catch (IOException e) { - throw new PlatformMappingException(e, SkyFunctionException.Transience.PERSISTENT); + if (!platformMappingKey.wasExplicitlySetByUser()) { + // If no flag was passed and the default mapping file does not exist treat this as if the + // mapping file was empty rather than an error. + return PlatformMappingValue.EMPTY; } - - return new Parser(lines.iterator()).parse().toPlatformMappingValue(); + throw new PlatformMappingException( + new MissingInputFileException( + String.format( + "--platform_mappings was set to '%s' but no such file exists relative to the " + + "package path roots, '%s'", + workspaceRelativeMappingPath, pathEntries), + Location.BUILTIN), + SkyFunctionException.Transience.PERSISTENT); } @Nullable
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PlatformMappingValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/PlatformMappingValue.java index 9f37b18..92b2acb 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PlatformMappingValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PlatformMappingValue.java
@@ -14,6 +14,7 @@ package com.google.devtools.build.lib.skyframe; +import com.google.common.base.MoreObjects; import com.google.common.base.Preconditions; import com.google.common.cache.Cache; import com.google.common.cache.CacheBuilder; @@ -149,8 +150,8 @@ PlatformMappingValue( Map<Label, Collection<String>> platformsToFlags, Map<Collection<String>, Label> flagsToPlatforms) { - this.platformsToFlags = platformsToFlags; - this.flagsToPlatforms = flagsToPlatforms; + this.platformsToFlags = Preconditions.checkNotNull(platformsToFlags); + this.flagsToPlatforms = Preconditions.checkNotNull(flagsToPlatforms); this.parserCache = CacheBuilder.newBuilder() .initialCapacity(platformsToFlags.size() + flagsToPlatforms.size()) @@ -249,4 +250,30 @@ // TODO(schmitt): Parse starlark options as well. return parser; } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + if (!(obj instanceof PlatformMappingValue)) { + return false; + } + PlatformMappingValue that = (PlatformMappingValue) obj; + return this.flagsToPlatforms.equals(that.flagsToPlatforms) + && this.platformsToFlags.equals(that.platformsToFlags); + } + + @Override + public int hashCode() { + return 37 * flagsToPlatforms.hashCode() + platformsToFlags.hashCode(); + } + + @Override + public String toString() { + return MoreObjects.toStringHelper(this) + .add("flagsToPlatforms", flagsToPlatforms) + .add("platformsToFlags", platformsToFlags) + .toString(); + } }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionTest.java index 5f0df27..6a41424 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionTest.java
@@ -147,6 +147,79 @@ assertThat(toMappedOptions(mapped).get(CoreOptions.class).cpu).isEqualTo("one"); } + @Test + public void handlesNoWorkspaceFile() throws Exception { + scratch.setWorkingDir("/other/package/path"); + scratch.file( + "my_mapping_file", + "platforms:", // Force line break + " //platforms:one", // Force line break + " --cpu=one"); + setPackageCacheOptions("--package_path=/other/package/path"); + + PlatformMappingValue platformMappingValue = + executeFunction(PlatformMappingValue.Key.create(PathFragment.create("my_mapping_file"))); + BuildOptions modifiedOptions = DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS.clone(); + modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM1); + + BuildConfigurationValue.Key mapped = + platformMappingValue.map( + keyForOptions(modifiedOptions), DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); + + assertThat(toMappedOptions(mapped).get(CoreOptions.class).cpu).isEqualTo("one"); + } + + @Test + public void multiplePackagePaths() throws Exception { + scratch.setWorkingDir("/other/package/path"); + scratch.file( + "my_mapping_file", + "platforms:", // Force line break + " //platforms:one", // Force line break + " --cpu=one"); + setPackageCacheOptions("--package_path=%workspace%:/other/package/path"); + + PlatformMappingValue platformMappingValue = + executeFunction(PlatformMappingValue.Key.create(PathFragment.create("my_mapping_file"))); + + BuildOptions modifiedOptions = DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS.clone(); + modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM1); + + BuildConfigurationValue.Key mapped = + platformMappingValue.map( + keyForOptions(modifiedOptions), DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); + + assertThat(toMappedOptions(mapped).get(CoreOptions.class).cpu).isEqualTo("one"); + } + + @Test + public void multiplePackagePathsFirstWins() throws Exception { + scratch.file( + "my_mapping_file", + "platforms:", // Force line break + " //platforms:one", // Force line break + " --cpu=one"); + scratch.setWorkingDir("/other/package/path"); + scratch.file( + "my_mapping_file", + "platforms:", // Force line break + " //platforms:one", // Force line break + " --cpu=two"); + setPackageCacheOptions("--package_path=%workspace%:/other/package/path"); + + PlatformMappingValue platformMappingValue = + executeFunction(PlatformMappingValue.Key.create(PathFragment.create("my_mapping_file"))); + + BuildOptions modifiedOptions = DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS.clone(); + modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM1); + + BuildConfigurationValue.Key mapped = + platformMappingValue.map( + keyForOptions(modifiedOptions), DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); + + assertThat(toMappedOptions(mapped).get(CoreOptions.class).cpu).isEqualTo("one"); + } + private PlatformMappingValue executeFunction(PlatformMappingValue.Key key) throws Exception { SkyframeExecutor skyframeExecutor = getSkyframeExecutor(); skyframeExecutor.injectExtraPrecomputedValues(