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 index ffdf5b6f0f83d7..4e4250d1f9f240 100644 --- 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 new file mode 100644 index 00000000000000..4d9821b60d5a25 --- /dev/null +++ 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 index 43798f1441d3bc..eb9111f33e56de 100644 --- 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> 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 lines, RepoContext mainRepoContext) - throws PlatformMappingException { + throws PlatformMappingParsingException { PeekingIterator it = Iterators.peekingIterator( lines.stream() @@ -267,7 +251,7 @@ public Target loadBuildSetting(String name) @Nullable private static NativeAndStarlarkFlags parseStarlarkFlags( ImmutableSet rawFlags, Environment env, RepoContext mainRepoContext) - throws PlatformMappingException { + throws PlatformMappingParsingException { ImmutableSet.Builder nativeFlags = ImmutableSet.builder(); ImmutableList.Builder 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 readPlatformsToFlags( PeekingIterator it, Environment env, RepoContext mainRepoContext) - throws PlatformMappingException { + throws PlatformMappingParsingException { ImmutableMap.Builder platformsToFlags = ImmutableMap.builder(); boolean needSkyframeDeps = false; while (it.hasNext() && !it.peek().equalsIgnoreCase("flags:")) { @@ -327,7 +311,8 @@ private static ImmutableMap readPlatformsToFlags( } private static ImmutableMap, Label> readFlagsToPlatforms( - PeekingIterator it, RepoContext mainRepoContext) throws PlatformMappingException { + PeekingIterator it, RepoContext mainRepoContext) + throws PlatformMappingParsingException { ImmutableMap.Builder, Label> flagsToPlatforms = ImmutableMap.builder(); while (it.hasNext() && it.peek().startsWith("--")) { ImmutableSet flags = readFlags(it); @@ -343,7 +328,7 @@ private static ImmutableMap, Label> readFlagsToPlatforms( } private static Label readPlatform(PeekingIterator 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 it, RepoContext mainRe } private static ImmutableSet readFlags(PeekingIterator it) - throws PlatformMappingException { + throws PlatformMappingParsingException { ImmutableSet.Builder flags = ImmutableSet.builder(); // Note: Short form flags are not supported. while (it.hasNext() && it.peek().startsWith("--")) { @@ -373,12 +358,12 @@ private static ImmutableSet readFlags(PeekingIterator 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/src/main/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingParsingException.java b/src/main/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingParsingException.java new file mode 100644 index 00000000000000..25114251097af2 --- /dev/null +++ b/src/main/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 index 1842ecd84526c6..d3ed700889aefe 100644 --- 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", diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingFunctionParserTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingFunctionParserTest.java index 54fc70b9789f6a..bf0e02edd83078 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingFunctionParserTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingFunctionParserTest.java @@ -22,7 +22,6 @@ import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.Label.RepoContext; -import com.google.devtools.build.lib.cmdline.LabelSyntaxException; import com.google.devtools.build.lib.cmdline.RepositoryMapping; import com.google.devtools.build.lib.cmdline.RepositoryName; import org.junit.Test; @@ -242,9 +241,9 @@ public void testParseCommentOnly() throws Exception { @Test public void testParseExtraPlatformInFlags() throws Exception { - PlatformMappingFunction.PlatformMappingException exception = + PlatformMappingParsingException exception = assertThrows( - PlatformMappingFunction.PlatformMappingException.class, + PlatformMappingParsingException.class, () -> parse( "flags:", // Force line break @@ -258,9 +257,9 @@ public void testParseExtraPlatformInFlags() throws Exception { @Test public void testParsePlatformWithoutFlags() throws Exception { - PlatformMappingFunction.PlatformMappingException exception = + PlatformMappingParsingException exception = assertThrows( - PlatformMappingFunction.PlatformMappingException.class, + PlatformMappingParsingException.class, () -> parse( "platforms:", // Force line break @@ -272,9 +271,9 @@ public void testParsePlatformWithoutFlags() throws Exception { @Test public void testParseFlagsWithoutPlatform() throws Exception { - PlatformMappingFunction.PlatformMappingException exception = + PlatformMappingParsingException exception = assertThrows( - PlatformMappingFunction.PlatformMappingException.class, + PlatformMappingParsingException.class, () -> parse( "flags:", // Force line break @@ -299,9 +298,9 @@ public void testParseCommentEndOfFile() throws Exception { @Test public void testParseUnknownSection() throws Exception { - PlatformMappingFunction.PlatformMappingException exception = + PlatformMappingParsingException exception = assertThrows( - PlatformMappingFunction.PlatformMappingException.class, + PlatformMappingParsingException.class, () -> parse( "platform:", // Force line break @@ -311,25 +310,26 @@ 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")); + exception = + assertThrows( + PlatformMappingParsingException.class, + () -> + parse( + "platforms:", + " //platforms:one", + " --cpu=one", + "flag:", + " --cpu=one", + " //platforms:one")); assertThat(exception).hasMessageThat().contains("platform"); } @Test public void testParsePlatformsInvalidPlatformLabel() throws Exception { - PlatformMappingFunction.PlatformMappingException exception = + PlatformMappingParsingException exception = assertThrows( - PlatformMappingFunction.PlatformMappingException.class, + PlatformMappingParsingException.class, () -> parse( "platforms:", // Force line break @@ -337,14 +337,13 @@ public void testParsePlatformsInvalidPlatformLabel() throws Exception { " --cpu=one")); assertThat(exception).hasMessageThat().contains("@@@"); - assertThat(exception).hasCauseThat().hasCauseThat().isInstanceOf(LabelSyntaxException.class); } @Test public void testParseFlagsInvalidPlatformLabel() throws Exception { - PlatformMappingFunction.PlatformMappingException exception = + PlatformMappingParsingException exception = assertThrows( - PlatformMappingFunction.PlatformMappingException.class, + PlatformMappingParsingException.class, () -> parse( "flags:", // Force line break @@ -352,14 +351,13 @@ public void testParseFlagsInvalidPlatformLabel() throws Exception { " @@@")); assertThat(exception).hasMessageThat().contains("@@@"); - assertThat(exception).hasCauseThat().hasCauseThat().isInstanceOf(LabelSyntaxException.class); } @Test public void testParsePlatformsInvalidFlag() throws Exception { - PlatformMappingFunction.PlatformMappingException exception = + PlatformMappingParsingException exception = assertThrows( - PlatformMappingFunction.PlatformMappingException.class, + PlatformMappingParsingException.class, () -> parse( "platforms:", // Force line break @@ -371,13 +369,13 @@ public void testParsePlatformsInvalidFlag() throws Exception { @Test public void testParseFlagsInvalidFlag() throws Exception { - PlatformMappingFunction.PlatformMappingException exception = + PlatformMappingParsingException exception = assertThrows( - PlatformMappingFunction.PlatformMappingException.class, + PlatformMappingParsingException.class, () -> parse( "flags:", // Force line break - " -cpu=one", // Force line break + " -cpu=one", // Force line breakPlatformMappingFunction " //platforms:one")); assertThat(exception).hasMessageThat().contains("-cpu"); @@ -385,9 +383,9 @@ public void testParseFlagsInvalidFlag() throws Exception { @Test public void testParsePlatformsDuplicatePlatform() throws Exception { - PlatformMappingFunction.PlatformMappingException exception = + PlatformMappingParsingException exception = assertThrows( - PlatformMappingFunction.PlatformMappingException.class, + PlatformMappingParsingException.class, () -> parse( "platforms:", // Force line break @@ -397,45 +395,33 @@ public void testParsePlatformsDuplicatePlatform() throws Exception { " --cpu=two")); assertThat(exception).hasMessageThat().contains("duplicate"); - assertThat(exception) - .hasCauseThat() - .hasCauseThat() - .hasMessageThat() - .contains("//platforms:one"); } @Test public void testParseFlagsDuplicateFlags() throws Exception { - PlatformMappingFunction.PlatformMappingException exception = + PlatformMappingParsingException exception = assertThrows( - PlatformMappingFunction.PlatformMappingException.class, + PlatformMappingParsingException.class, () -> parse( "flags:", // Force line break " --compilation_mode=dbg", // Force line break - " --cpu=one", // Force line break + " --cpu=one", // Force line break:242 " //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) - throws PlatformMappingFunction.PlatformMappingException { + throws PlatformMappingParsingException { return parse(RepositoryMapping.ALWAYS_FALLBACK, lines); } private static PlatformMappingFunction.Mappings parse( - RepositoryMapping mainRepoMapping, String... lines) - throws PlatformMappingFunction.PlatformMappingException { + RepositoryMapping mainRepoMapping, String... lines) throws PlatformMappingParsingException { return PlatformMappingFunction.parse( /* env= */ null, ImmutableList.copyOf(lines), diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingFunctionTest.java index f9e8e0bdcdb082..c121077222491f 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingFunctionTest.java @@ -35,7 +35,6 @@ import com.google.devtools.build.lib.testutil.TestRuleClassProvider; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.skyframe.EvaluationResult; -import com.google.devtools.common.options.OptionsParsingException; import java.util.Optional; import org.junit.Before; import org.junit.Test; @@ -74,12 +73,13 @@ public void setDefaultBuildOptions() { @Test public void testMappingFileDoesNotExist() { - MissingInputFileException exception = + PlatformMappingException exception = assertThrows( - MissingInputFileException.class, + PlatformMappingException.class, () -> executeFunction( PlatformMappingValue.Key.create(PathFragment.create("random_location")))); + assertThat(exception).hasCauseThat().isInstanceOf(MissingInputFileException.class); assertThat(exception).hasMessageThat().contains("random_location"); } @@ -98,10 +98,11 @@ public void testMappingFileDoesNotExistDefaultLocation() throws Exception { public void testMappingFileIsDirectory() throws Exception { scratch.dir("somedir"); - MissingInputFileException exception = + PlatformMappingException exception = assertThrows( - MissingInputFileException.class, + PlatformMappingException.class, () -> executeFunction(PlatformMappingValue.Key.create(PathFragment.create("somedir")))); + assertThat(exception).hasCauseThat().isInstanceOf(MissingInputFileException.class); assertThat(exception).hasMessageThat().contains("somedir"); } @@ -282,12 +283,14 @@ public void badStarlarkFlag() throws Exception { " //platforms:one", // Force line break " --//test:this_flag_doesnt_exist=mapped_value"); - assertThrows( - "Failed to load //test:this_flag_doesnt_exist", - OptionsParsingException.class, - () -> - executeFunction( - PlatformMappingValue.Key.create(PathFragment.create("my_mapping_file")))); + PlatformMappingException exception = + assertThrows( + PlatformMappingException.class, + () -> + executeFunction( + PlatformMappingValue.Key.create(PathFragment.create("my_mapping_file")))); + assertThat(exception).hasCauseThat().isInstanceOf(PlatformMappingParsingException.class); + assertThat(exception).hasMessageThat().contains("Failed to load //test:this_flag_doesnt_exist"); } @Test @@ -370,6 +373,27 @@ public void platformTransitionWithStarlarkFlagMapping() throws Exception { .isEqualTo("platform-mapped value"); } + @Test + public void mappingSyntaxError() throws Exception { + scratch.file("test/BUILD"); + scratch.file( + "my_mapping_file", + "platforms:", // Force line break + " //platforms:one", // Force line break + " --cpu=k8", + " //platforms:one", // Duplicate platform label + " --cpu=arm"); + + PlatformMappingException exception = + assertThrows( + PlatformMappingException.class, + () -> + executeFunction( + PlatformMappingValue.Key.create(PathFragment.create("my_mapping_file")))); + assertThat(exception).hasCauseThat().isInstanceOf(PlatformMappingParsingException.class); + assertThat(exception).hasMessageThat().contains("Got duplicate platform entries"); + } + private PlatformMappingValue executeFunction(PlatformMappingValue.Key key) throws Exception { SkyframeExecutor skyframeExecutor = getSkyframeExecutor(); skyframeExecutor.injectExtraPrecomputedValues(