From 4a63e854744315264b935f9c3d7af66cf03a31dc Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 2 May 2024 05:19:28 -0700 Subject: [PATCH] More platform mapping test cleanups. Work towards platform-based flags: #19409. PiperOrigin-RevId: 630030455 Change-Id: I91933097a31ad5fe1ff5955e58261bde357362d0 --- .../config/PlatformMappingFunctionTest.java | 62 ++--- .../config/PlatformMappingValueTest.java | 221 ++++++++++-------- 2 files changed, 141 insertions(+), 142 deletions(-) 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 ca604442c5dc9e..4f0eee9decc4dc 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 @@ -40,7 +40,6 @@ import com.google.devtools.common.options.OptionDocumentationCategory; import com.google.devtools.common.options.OptionEffectTag; import com.google.devtools.common.options.OptionMetadataTag; -import com.google.devtools.common.options.OptionsParser; import java.util.List; import java.util.Optional; import org.junit.Test; @@ -57,9 +56,6 @@ public final class PlatformMappingFunctionTest extends BuildViewTestCase { private static final Label PLATFORM1 = Label.parseCanonicalUnchecked("//platforms:one"); - private static final Label DEFAULT_TARGET_PLATFORM = - Label.parseCanonicalUnchecked("@bazel_tools//tools:host_platform"); - /** Extra options for this test. */ public static class DummyTestOptions extends FragmentOptions { public DummyTestOptions() {} @@ -88,15 +84,6 @@ public DummyTestOptions() {} public List list; } - private static final ImmutableList> BUILD_CONFIG_OPTIONS = - ImmutableList.of( - // Needed for --platforms - PlatformOptions.class, - // All other native flags - DummyTestOptions.class); - - // Make sure PlatformMappingFunction can use the new options class. - /** Test fragment. */ @RequiresOptions(options = {DummyTestOptions.class}) public static final class DummyTestOptionsFragment extends Fragment { @@ -121,13 +108,8 @@ protected ConfiguredRuleClassProvider createRuleClassProvider() { return builder.build(); } - private BuildOptions createBuildOptions() { - return BuildOptions.of( - BUILD_CONFIG_OPTIONS, OptionsParser.builder().optionsClasses(BUILD_CONFIG_OPTIONS).build()); - } - @Test - public void testMappingFileDoesNotExist() { + public void invalidMappingFile_doesNotExist_customLocation() { PlatformMappingException exception = assertThrows( PlatformMappingException.class, @@ -139,18 +121,18 @@ public void testMappingFileDoesNotExist() { } @Test - public void testMappingFileDoesNotExistDefaultLocation() throws Exception { + public void invalidMappingFile_doesNotExist_defaultLocation() throws Exception { PlatformMappingValue platformMappingValue = executeFunction(PlatformMappingValue.Key.create(null)); BuildOptions mapped = platformMappingValue.map(createBuildOptions()); assertThat(mapped.get(PlatformOptions.class).platforms) - .containsExactly(DEFAULT_TARGET_PLATFORM); + .containsExactly(Label.parseCanonicalUnchecked("@bazel_tools//tools:host_platform")); } @Test - public void testMappingFileIsDirectory() throws Exception { + public void invalidMappingFile_isDirectory() throws Exception { scratch.dir("somedir"); PlatformMappingException exception = @@ -174,8 +156,7 @@ public void mapFromPlatform() throws Exception { PlatformMappingValue platformMappingValue = executeFunction(PlatformMappingValue.Key.create(PathFragment.create("my_mapping_file"))); - BuildOptions modifiedOptions = createBuildOptions(); - modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM1); + BuildOptions modifiedOptions = createBuildOptions("--platforms=//platforms:one"); BuildOptions mapped = platformMappingValue.map(modifiedOptions); @@ -198,8 +179,7 @@ public void mapFromPlatform_fromAlternatePackagePath() throws Exception { PlatformMappingValue platformMappingValue = executeFunction(PlatformMappingValue.Key.create(PathFragment.create("my_mapping_file"))); - BuildOptions modifiedOptions = createBuildOptions(); - modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM1); + BuildOptions modifiedOptions = createBuildOptions("--platforms=//platforms:one"); BuildOptions mapped = platformMappingValue.map(modifiedOptions); @@ -226,8 +206,7 @@ public void mapFromPlatform_noWorkspace() throws Exception { PlatformMappingValue platformMappingValue = executeFunction(PlatformMappingValue.Key.create(PathFragment.create("my_mapping_file"))); - BuildOptions modifiedOptions = createBuildOptions(); - modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM1); + BuildOptions modifiedOptions = createBuildOptions("--platforms=//platforms:one"); BuildOptions mapped = platformMappingValue.map(modifiedOptions); @@ -249,8 +228,7 @@ public void multiplePackagePaths() throws Exception { PlatformMappingValue platformMappingValue = executeFunction(PlatformMappingValue.Key.create(PathFragment.create("my_mapping_file"))); - BuildOptions modifiedOptions = createBuildOptions(); - modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM1); + BuildOptions modifiedOptions = createBuildOptions("--platforms=//platforms:one"); BuildOptions mapped = platformMappingValue.map(modifiedOptions); @@ -279,8 +257,7 @@ public void multiplePackagePathsFirstWins() throws Exception { PlatformMappingValue platformMappingValue = executeFunction(PlatformMappingValue.Key.create(PathFragment.create("my_mapping_file"))); - BuildOptions modifiedOptions = createBuildOptions(); - modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM1); + BuildOptions modifiedOptions = createBuildOptions("--platforms=//platforms:one"); BuildOptions mapped = platformMappingValue.map(modifiedOptions); @@ -302,8 +279,7 @@ public void mapFromPlatform_internalOption() throws Exception { PlatformMappingValue platformMappingValue = executeFunction(PlatformMappingValue.Key.create(PathFragment.create("my_mapping_file"))); - BuildOptions modifiedOptions = createBuildOptions(); - modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM1); + BuildOptions modifiedOptions = createBuildOptions("--platforms=//platforms:one"); BuildOptions mapped = platformMappingValue.map(modifiedOptions); @@ -324,8 +300,8 @@ public void mapFromPlatform_starlarkFlag() throws Exception { PlatformMappingValue platformMappingValue = executeFunction(PlatformMappingValue.Key.create(PathFragment.create("my_mapping_file"))); - BuildOptions modifiedOptions = createBuildOptions(); - modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM1); + BuildOptions modifiedOptions = createBuildOptions("--platforms=//platforms:one"); + BuildOptions mapped = platformMappingValue.map(modifiedOptions); assertThat(mapped.getStarlarkOptions()) @@ -345,9 +321,8 @@ public void mapFromPlatform_listFlag_overridesConfig() throws Exception { PlatformMappingValue platformMappingValue = executeFunction(PlatformMappingValue.Key.create(PathFragment.create("my_mapping_file"))); - BuildOptions modifiedOptions = createBuildOptions(); - modifiedOptions.get(DummyTestOptions.class).list = ImmutableList.of("from_config"); - modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM1); + BuildOptions modifiedOptions = + createBuildOptions("--platforms=//platforms:one", "--list=from_config"); BuildOptions mapped = platformMappingValue.map(modifiedOptions); @@ -467,8 +442,7 @@ public void mapFromFlag() throws Exception { PlatformMappingValue platformMappingValue = executeFunction(PlatformMappingValue.Key.create(PathFragment.create("my_mapping_file"))); - BuildOptions modifiedOptions = createBuildOptions(); - modifiedOptions.get(DummyTestOptions.class).strOption = "one"; + BuildOptions modifiedOptions = createBuildOptions("--str_option=one"); BuildOptions mapped = platformMappingValue.map(modifiedOptions); @@ -489,11 +463,7 @@ public void mapFromFlag_starlarkFlag() throws Exception { PlatformMappingValue platformMappingValue = executeFunction(PlatformMappingValue.Key.create(PathFragment.create("my_mapping_file"))); - BuildOptions modifiedOptions = - createBuildOptions().toBuilder() - .addStarlarkOption( - Label.parseCanonicalUnchecked("//flag:my_string_flag"), "mapped_value") - .build(); + BuildOptions modifiedOptions = createBuildOptions("--//flag:my_string_flag=mapped_value"); BuildOptions mapped = platformMappingValue.map(modifiedOptions); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingValueTest.java index 43bdad47ff5281..0899bcdf49019e 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingValueTest.java @@ -21,14 +21,18 @@ import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.analysis.PlatformOptions; import com.google.devtools.build.lib.analysis.config.BuildOptions; -import com.google.devtools.build.lib.analysis.config.CompilationMode; -import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.analysis.config.FragmentOptions; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.RepositoryMapping; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.common.options.Option; +import com.google.devtools.common.options.OptionDocumentationCategory; +import com.google.devtools.common.options.OptionEffectTag; import com.google.devtools.common.options.OptionsParsingException; +import com.google.errorprone.annotations.CanIgnoreReturnValue; +import java.util.HashMap; +import java.util.Map; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -37,102 +41,144 @@ @RunWith(JUnit4.class) public final class PlatformMappingValueTest { - private static final ImmutableSet> - BUILD_CONFIG_PLATFORM_OPTIONS = ImmutableSet.of(CoreOptions.class, PlatformOptions.class); - - private static final Label PLATFORM1 = Label.parseCanonicalUnchecked("//platforms:one"); - private static final Label PLATFORM2 = Label.parseCanonicalUnchecked("@dep~1.0//platforms:two"); + private static final Label PLATFORM_ONE = Label.parseCanonicalUnchecked("//platforms:one"); + private static final Label PLATFORM_TWO = + Label.parseCanonicalUnchecked("@dep~1.0//platforms:two"); private static final RepositoryMapping REPO_MAPPING = RepositoryMapping.create( ImmutableMap.of( "", RepositoryName.MAIN, "dep", RepositoryName.createUnvalidated("dep~1.0")), RepositoryName.MAIN); - - private static final BuildOptions DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS = - BuildOptions.getDefaultBuildOptionsForFragments(BUILD_CONFIG_PLATFORM_OPTIONS); private static final Label DEFAULT_TARGET_PLATFORM = Label.parseCanonicalUnchecked("@bazel_tools//tools:host_platform"); + /** Extra options for this test. */ + public static class DummyTestOptions extends FragmentOptions { + public DummyTestOptions() {} + + @Option( + name = "str_option", + documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + effectTags = {OptionEffectTag.NO_OP}, + defaultValue = "defVal") + public String strOption; + + @Option( + name = "other_str_option", + documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + effectTags = {OptionEffectTag.NO_OP}, + defaultValue = "defVal") + public String otherStrOption; + } + + private static final ImmutableSet> BUILD_CONFIG_OPTIONS = + // PlatformOptions is required for mapping. + ImmutableSet.of(PlatformOptions.class, DummyTestOptions.class); + + private BuildOptions createBuildOptions(String... args) throws OptionsParsingException { + return BuildOptions.of(BUILD_CONFIG_OPTIONS, args); + } + + private static PlatformMappingBuilder builder() { + return new PlatformMappingBuilder(); + } + + private static class PlatformMappingBuilder { + private final Map platformsToFlags = new HashMap<>(); + private final Map flagsToPlatforms = new HashMap<>(); + + @CanIgnoreReturnValue + public PlatformMappingBuilder addPlatform(Label platform, NativeAndStarlarkFlags flags) { + this.platformsToFlags.put(platform, flags); + return this; + } + + @CanIgnoreReturnValue + public PlatformMappingBuilder addPlatform(Label platform, String... nativeFlags) { + return this.addPlatform(platform, createFlags(nativeFlags)); + } + + @CanIgnoreReturnValue + public PlatformMappingBuilder addFlags(NativeAndStarlarkFlags flags, Label platform) { + this.flagsToPlatforms.put(flags, platform); + return this; + } + + @CanIgnoreReturnValue + public PlatformMappingBuilder addFlags(Label platform, String... nativeFlags) { + return this.addFlags(createFlags(nativeFlags), platform); + } + + public PlatformMappingValue build() { + return new PlatformMappingValue( + ImmutableMap.copyOf(platformsToFlags), + ImmutableMap.copyOf(flagsToPlatforms), + BUILD_CONFIG_OPTIONS); + } + + private NativeAndStarlarkFlags createFlags(String... nativeFlags) { + return NativeAndStarlarkFlags.builder() + .nativeFlags(ImmutableList.copyOf(nativeFlags)) + .optionsClasses(BUILD_CONFIG_OPTIONS) + .repoMapping(REPO_MAPPING) + .build(); + } + } + @Test - public void testMapNoMappings() throws OptionsParsingException { - ImmutableMap platformsToFlags = ImmutableMap.of(); - ImmutableMap flagsToPlatforms = ImmutableMap.of(); - PlatformMappingValue mappingValue = - new PlatformMappingValue(platformsToFlags, flagsToPlatforms, BUILD_CONFIG_PLATFORM_OPTIONS); + public void map_noMappings() throws OptionsParsingException { + PlatformMappingValue mappingValue = builder().build(); - BuildOptions mapped = mappingValue.map(DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); + BuildOptions mapped = mappingValue.map(createBuildOptions()); assertThat(mapped.get(PlatformOptions.class).platforms) .containsExactly(DEFAULT_TARGET_PLATFORM); } @Test - public void testMapPlatformToFlags() throws Exception { - ImmutableMap platformsToFlags = - ImmutableMap.of(PLATFORM1, createFlags("--cpu=one", "--compilation_mode=dbg")); - - ImmutableMap flagsToPlatforms = ImmutableMap.of(); + public void map_platformToFlags() throws Exception { PlatformMappingValue mappingValue = - new PlatformMappingValue(platformsToFlags, flagsToPlatforms, BUILD_CONFIG_PLATFORM_OPTIONS); + builder().addPlatform(PLATFORM_ONE, "--str_option=one", "--other_str_option=dbg").build(); - BuildOptions modifiedOptions = DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS.clone(); - modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM1); + BuildOptions modifiedOptions = createBuildOptions("--platforms=//platforms:one"); BuildOptions mapped = mappingValue.map(modifiedOptions); - assertThat(mapped.get(CoreOptions.class).cpu).isEqualTo("one"); + assertThat(mapped.get(DummyTestOptions.class).strOption).isEqualTo("one"); } @Test - public void testMapFlagsToPlatform() throws Exception { - ImmutableMap flagsToPlatforms = - ImmutableMap.of(createFlags("--cpu=one", "--compilation_mode=dbg"), PLATFORM1); - - ImmutableMap platformsToFlags = ImmutableMap.of(); + public void map_flagsToPlatform() throws Exception { PlatformMappingValue mappingValue = - new PlatformMappingValue(platformsToFlags, flagsToPlatforms, BUILD_CONFIG_PLATFORM_OPTIONS); - - BuildOptions modifiedOptions = DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS.clone(); - modifiedOptions.get(CoreOptions.class).cpu = "one"; - modifiedOptions.get(CoreOptions.class).compilationMode = CompilationMode.DBG; + builder().addFlags(PLATFORM_ONE, "--str_option=one", "--other_str_option=dbg").build(); + BuildOptions modifiedOptions = createBuildOptions("--str_option=one", "--other_str_option=dbg"); BuildOptions mapped = mappingValue.map(modifiedOptions); - assertThat(mapped.get(PlatformOptions.class).platforms).containsExactly(PLATFORM1); + assertThat(mapped.get(PlatformOptions.class).platforms).containsExactly(PLATFORM_ONE); } @Test - public void testMapFlagsToPlatformPriority() throws Exception { - ImmutableMap flagsToPlatforms = - ImmutableMap.of( - createFlags("--cpu=one", "--compilation_mode=dbg"), - PLATFORM1, - createFlags("--cpu=foo"), - PLATFORM2); - - ImmutableMap platformsToFlags = ImmutableMap.of(); + public void map_flagsToPlatform_checkPriority() throws Exception { PlatformMappingValue mappingValue = - new PlatformMappingValue(platformsToFlags, flagsToPlatforms, BUILD_CONFIG_PLATFORM_OPTIONS); + builder() + .addFlags(PLATFORM_ONE, "--str_option=one", "--other_str_option=dbg") + .addFlags(PLATFORM_TWO, "--str_option=two") + .build(); - BuildOptions modifiedOptions = DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS.clone(); - modifiedOptions.get(CoreOptions.class).cpu = "foo"; + BuildOptions modifiedOptions = createBuildOptions("--str_option=two"); BuildOptions mapped = mappingValue.map(modifiedOptions); - assertThat(mapped.get(PlatformOptions.class).platforms).containsExactly(PLATFORM2); + assertThat(mapped.get(PlatformOptions.class).platforms).containsExactly(PLATFORM_TWO); } @Test - public void testMapFlagsToPlatformNoneMatching() throws Exception { - ImmutableMap flagsToPlatforms = - ImmutableMap.of(createFlags("--cpu=foo", "--compilation_mode=dbg"), PLATFORM1); - - ImmutableMap platformsToFlags = ImmutableMap.of(); + public void map_flagsToPlatform_noneMatching() throws Exception { PlatformMappingValue mappingValue = - new PlatformMappingValue(platformsToFlags, flagsToPlatforms, BUILD_CONFIG_PLATFORM_OPTIONS); + builder().addFlags(PLATFORM_ONE, "--str_option=foo", "--other_str_option=dbg").build(); - BuildOptions modifiedOptions = DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS.clone(); - modifiedOptions.get(CoreOptions.class).cpu = "bar"; + BuildOptions modifiedOptions = createBuildOptions("--str_option=bar"); BuildOptions mapped = mappingValue.map(modifiedOptions); @@ -141,58 +187,49 @@ public void testMapFlagsToPlatformNoneMatching() throws Exception { } @Test - public void testMapNoPlatformOptions() throws Exception { - ImmutableMap flagsToPlatforms = - ImmutableMap.of(createFlags("--cpu=one"), PLATFORM1); - - ImmutableMap platformsToFlags = ImmutableMap.of(); - PlatformMappingValue mappingValue = - new PlatformMappingValue(platformsToFlags, flagsToPlatforms, BUILD_CONFIG_PLATFORM_OPTIONS); + public void map_noPlatformOptions() throws Exception { + PlatformMappingValue mappingValue = builder().build(); + // Does not contain PlatformOptions. BuildOptions options = BuildOptions.of(ImmutableList.of()); - assertThrows(IllegalArgumentException.class, () -> mappingValue.map(options)); } @Test - public void testMapNoMappingIfPlatformIsSetButNotMatching() throws Exception { - ImmutableMap platformsToFlags = - ImmutableMap.of(PLATFORM1, createFlags("--cpu=one", "--compilation_mode=dbg")); - ImmutableMap flagsToPlatforms = - ImmutableMap.of(createFlags("--cpu=one"), PLATFORM1); - - BuildOptions modifiedOptions = DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS.clone(); - modifiedOptions.get(CoreOptions.class).cpu = "one"; - modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM2); - + public void map_noMappingIfPlatformIsSetButNotMatching() throws Exception { PlatformMappingValue mappingValue = - new PlatformMappingValue(platformsToFlags, flagsToPlatforms, BUILD_CONFIG_PLATFORM_OPTIONS); + builder() + // Add a mapping for a different platform. + .addPlatform(PLATFORM_ONE, "--str_option=one", "--other_str_option=dbg") + .build(); + BuildOptions modifiedOptions = + createBuildOptions("--str_option=one", "--platforms=//platforms:two"); BuildOptions mapped = mappingValue.map(modifiedOptions); + // No change because the platform is not in the mapping. assertThat(modifiedOptions).isEqualTo(mapped); } @Test - public void testMapNoMappingIfPlatformIsSetAndNoPlatformMapping() throws Exception { - ImmutableMap flagsToPlatforms = - ImmutableMap.of(createFlags("--cpu=one"), PLATFORM1); - - BuildOptions modifiedOptions = DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS.clone(); - modifiedOptions.get(CoreOptions.class).cpu = "one"; - modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM2); - - ImmutableMap platformsToFlags = ImmutableMap.of(); + public void map_noMappingIfPlatformIsSetAndNoPlatformMapping() throws Exception { PlatformMappingValue mappingValue = - new PlatformMappingValue(platformsToFlags, flagsToPlatforms, BUILD_CONFIG_PLATFORM_OPTIONS); + builder() + // Add a flag mapping that would match. + .addFlags(PLATFORM_ONE, "--str_option=one") + .build(); + + BuildOptions modifiedOptions = + createBuildOptions("--str_option=one", "--platforms=//platforms:two"); BuildOptions mapped = mappingValue.map(modifiedOptions); + // No change because the platform is not in the mapping. assertThat(modifiedOptions).isEqualTo(mapped); } @Test - public void testDefaultKey() { + public void defaultKey() { PlatformMappingValue.Key key = PlatformMappingValue.Key.create(null); assertThat(key.getWorkspaceRelativeMappingPath()) @@ -201,18 +238,10 @@ public void testDefaultKey() { } @Test - public void testCustomKey() { + public void customKey() { PlatformMappingValue.Key key = PlatformMappingValue.Key.create(PathFragment.create("my/path")); assertThat(key.getWorkspaceRelativeMappingPath()).isEqualTo(PathFragment.create("my/path")); assertThat(key.wasExplicitlySetByUser()).isTrue(); } - - private NativeAndStarlarkFlags createFlags(String... nativeFlags) { - return NativeAndStarlarkFlags.builder() - .nativeFlags(ImmutableList.copyOf(nativeFlags)) - .optionsClasses(BUILD_CONFIG_PLATFORM_OPTIONS) - .repoMapping(REPO_MAPPING) - .build(); - } }