Skip to content

Commit

Permalink
Remove explicitly set target platform value and replace it by fallback.
Browse files Browse the repository at this point in the history
The original value for the target platform was set at the dawn of time when we didn't have auto-calculation for it yet. Now that we do the explicit setting interferes with platform mapping which assumes that the target platform value is empty unless the user explicitly requests otherwise.

This change introduces a new flag, --target_platform_fallsback which can be used to set the fallback in case there is no matching mapping. Because it uses the old defaults no behavioral change should be observed as long as no mappings exist.

Step 5/N towards the platforms mapping functionality for #6426

RELNOTES: None.
PiperOrigin-RevId: 241031610
  • Loading branch information
aragos authored and copybara-github committed Mar 29, 2019
1 parent e937767 commit ad87481
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,6 @@ 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
Expand Down Expand Up @@ -92,6 +90,21 @@ 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 = "",
Expand Down Expand Up @@ -229,7 +242,7 @@ public Label computeTargetPlatform() {
return computeHostPlatform();
} else {
// Use the legacy target platform
return LEGACY_DEFAULT_TARGET_PLATFORM;
return targetPlatformFallback;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
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;
Expand Down Expand Up @@ -64,6 +63,8 @@ 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 {
Expand All @@ -88,7 +89,7 @@ public void testMappingFileDoesNotExistDefaultLocation() throws Exception {
platformMappingValue.map(key, DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS);

assertThat(toMappedOptions(mapped).get(PlatformOptions.class).platforms)
.containsExactly(LEGACY_DEFAULT_TARGET_PLATFORM);
.containsExactly(DEFAULT_TARGET_PLATFORM);
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
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.collect.ImmutableList;
Expand Down Expand Up @@ -57,6 +56,8 @@ public class PlatformMappingValueTest {
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 testMapNoMappings() throws OptionsParsingException {
Expand All @@ -70,7 +71,7 @@ public void testMapNoMappings() throws OptionsParsingException {
mappingValue.map(key, DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS);

assertThat(toMappedOptions(mapped).get(PlatformOptions.class).platforms)
.containsExactly(LEGACY_DEFAULT_TARGET_PLATFORM);
.containsExactly(DEFAULT_TARGET_PLATFORM);
}

@Test
Expand Down Expand Up @@ -148,7 +149,7 @@ public void testMapFlagsToPlatformNoneMatching() throws Exception {
mappingValue.map(keyForOptions(modifiedOptions), DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS);

assertThat(toMappedOptions(mapped).get(PlatformOptions.class).platforms)
.containsExactly(LEGACY_DEFAULT_TARGET_PLATFORM);
.containsExactly(DEFAULT_TARGET_PLATFORM);
}

@Test
Expand Down

0 comments on commit ad87481

Please sign in to comment.