Skip to content

Commit

Permalink
Automated rollback of commit 34f20a1.
Browse files Browse the repository at this point in the history
*** Reason for rollback ***

See unknown commit

*** Original change description ***

Replace HOST transition with EXEC transition for Starlark rules.

PiperOrigin-RevId: 429580659
  • Loading branch information
tetromino authored and copybara-github committed Feb 18, 2022
1 parent 9a8cac7 commit bdc6c5c
Show file tree
Hide file tree
Showing 12 changed files with 90 additions and 48 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory;
import com.google.devtools.build.lib.analysis.config.HostTransition;
import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition;
import com.google.devtools.build.lib.analysis.config.TransitionFactories;
import com.google.devtools.build.lib.analysis.config.transitions.SplitTransition;
Expand Down Expand Up @@ -227,9 +228,8 @@ && containsNonNoneKey(arguments, ALLOW_SINGLE_FILE_ARG)) {
throw Starlark.errorf(
"late-bound attributes must not have a split configuration transition");
}
// TODO(b/203203933): Officially deprecate HOST transition and remove this.
if (trans.equals("host")) {
builder.cfg(ExecutionTransitionFactory.create());
builder.cfg(HostTransition.createFactory());
} else if (trans.equals("exec")) {
builder.cfg(ExecutionTransitionFactory.create());
} else if (trans instanceof ExecutionTransitionFactory) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
"//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
"//src/main/java/com/google/devtools/build/lib/analysis:config/execution_transition_factory",
"//src/main/java/com/google/devtools/build/lib/analysis:config/host_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
"//src/main/java/com/google/devtools/build/lib/analysis:file_provider",
"//src/main/java/com/google/devtools/build/lib/analysis:make_variable_supplier",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
import com.google.devtools.build.lib.analysis.RunfilesSupport;
import com.google.devtools.build.lib.analysis.ViewCreationFailedException;
import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory;
import com.google.devtools.build.lib.analysis.config.HostTransition;
import com.google.devtools.build.lib.analysis.util.AnalysisTestCase;
import com.google.devtools.build.lib.analysis.util.MockRule;
import com.google.devtools.build.lib.cmdline.Label;
Expand Down Expand Up @@ -92,7 +92,7 @@ public ConfiguredTarget create(RuleContext context)
"native_test",
attr("deps", LABEL_LIST).allowedFileTypes(),
attr("host_deps", LABEL_LIST)
.cfg(ExecutionTransitionFactory.create())
.cfg(HostTransition.createFactory())
.allowedFileTypes());

private static final RuleDefinition NATIVE_LIB_RULE =
Expand All @@ -103,7 +103,7 @@ public ConfiguredTarget create(RuleContext context)
"native_lib",
attr("deps", LABEL_LIST).allowedFileTypes(),
attr("host_deps", LABEL_LIST)
.cfg(ExecutionTransitionFactory.create())
.cfg(HostTransition.createFactory())
.allowedFileTypes());

@Before
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.analysis.config.BuildOptions;
import com.google.devtools.build.lib.analysis.config.BuildOptionsView;
import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory;
import com.google.devtools.build.lib.analysis.config.HostTransition;
import com.google.devtools.build.lib.analysis.config.TransitionFactories;
import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition;
import com.google.devtools.build.lib.analysis.config.transitions.SplitTransition;
Expand Down Expand Up @@ -102,12 +102,11 @@ public void testDoublePropertySet() {
Attribute.Builder<String> builder =
attr("x", STRING)
.mandatory()
.cfg(ExecutionTransitionFactory.create())
.cfg(HostTransition.createFactory())
.undocumented("")
.value("y");
assertThrows(IllegalStateException.class, () -> builder.mandatory());
assertThrows(
IllegalStateException.class, () -> builder.cfg(ExecutionTransitionFactory.create()));
assertThrows(IllegalStateException.class, () -> builder.cfg(HostTransition.createFactory()));
assertThrows(IllegalStateException.class, () -> builder.undocumented(""));
assertThrows(IllegalStateException.class, () -> builder.value("z"));

Expand Down Expand Up @@ -280,8 +279,8 @@ public void testSplitTransitionProvider() throws Exception {
@Test
public void testHostTransition() throws Exception {
Attribute attr =
attr("foo", LABEL).cfg(ExecutionTransitionFactory.create()).allowedFileTypes().build();
assertThat(attr.getTransitionFactory().isTool()).isTrue();
attr("foo", LABEL).cfg(HostTransition.createFactory()).allowedFileTypes().build();
assertThat(attr.getTransitionFactory().isHost()).isTrue();
assertThat(attr.getTransitionFactory().isSplit()).isFalse();
}

Expand Down
2 changes: 1 addition & 1 deletion src/test/java/com/google/devtools/build/lib/packages/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,8 @@ java_test(
"//src/main/java/com/google/devtools/build/lib/actions:thread_state_receiver",
"//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
"//src/main/java/com/google/devtools/build/lib/analysis:config/build_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/execution_transition_factory",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment",
"//src/main/java/com/google/devtools/build/lib/analysis:config/host_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transition_factories",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/configuration_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/no_transition",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ java_test(
"//src/main/java/com/google/devtools/build/lib/analysis:config/build_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/execution_transition_factory",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/host_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transition_factories",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/patch_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import com.google.devtools.build.lib.analysis.config.BuildOptionsView;
import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory;
import com.google.devtools.build.lib.analysis.config.FragmentOptions;
import com.google.devtools.build.lib.analysis.config.HostTransition;
import com.google.devtools.build.lib.analysis.config.TransitionFactories;
import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
import com.google.devtools.build.lib.analysis.test.TestConfiguration.TestOptions;
Expand Down Expand Up @@ -207,7 +208,7 @@ public void testAlias_filtering() throws Exception {
"rule_with_host_dep",
attr("host_dep", LABEL)
.allowedFileTypes(FileTypeSet.ANY_FILE)
.cfg(ExecutionTransitionFactory.create()),
.cfg(HostTransition.createFactory()),
attr("$impl_dep", LABEL)
.allowedFileTypes(FileTypeSet.ANY_FILE)
.value(Label.parseAbsoluteUnchecked("//test:other")));
Expand Down Expand Up @@ -265,7 +266,7 @@ private void createConfigRulesAndBuild() throws Exception {
attr("target", LABEL).allowedFileTypes(FileTypeSet.ANY_FILE),
attr("host", LABEL)
.allowedFileTypes(FileTypeSet.ANY_FILE)
.cfg(ExecutionTransitionFactory.create()),
.cfg(HostTransition.createFactory()),
attr("exec", LABEL)
.allowedFileTypes(FileTypeSet.ANY_FILE)
.cfg(ExecutionTransitionFactory.create()),
Expand Down Expand Up @@ -360,7 +361,7 @@ public void testConfig_target() throws Exception {
}

@Test
public void testConfig_noMoreHostTransition() throws Exception {
public void testConfig_hostTransition() throws Exception {
createConfigRulesAndBuild();

getHelper().setWholeTestUniverseScope("test:my_rule");
Expand All @@ -370,21 +371,20 @@ public void testConfig_noMoreHostTransition() throws Exception {
.isEqualTo("No target (in) //test:target_dep could be found in the 'host' configuration");
assertConfigurableQueryCode(
targetResult.getFailureDetail(), ConfigurableQuery.Code.TARGET_MISSING);
EvalThrowsResult hostDepResult = evalThrows("config(//test:host_dep, host)", true);
assertThat(hostDepResult.getMessage())
.isEqualTo("No target (in) //test:host_dep could be found in the 'host' configuration");
assertConfigurableQueryCode(
hostDepResult.getFailureDetail(), ConfigurableQuery.Code.TARGET_MISSING);
EvalThrowsResult execDepResult = evalThrows("config(//test:exec_dep, host)", true);
assertThat(execDepResult.getMessage())
.isEqualTo("No target (in) //test:exec_dep could be found in the 'host' configuration");
assertConfigurableQueryCode(
execDepResult.getFailureDetail(), ConfigurableQuery.Code.TARGET_MISSING);
EvalThrowsResult hostResult = evalThrows("config(//test:dep, host)", true);
assertThat(eval("config(//test:host_dep, host)")).isEqualTo(eval("//test:host_dep"));
EvalThrowsResult hostResult = evalThrows("config(//test:exec_dep, host)", true);
assertThat(hostResult.getMessage())
.isEqualTo("No target (in) //test:dep could be found in the 'host' configuration");
.isEqualTo("No target (in) //test:exec_dep could be found in the 'host' configuration");
assertConfigurableQueryCode(
hostResult.getFailureDetail(), ConfigurableQuery.Code.TARGET_MISSING);

BuildConfigurationValue configuration =
getConfiguration(Iterables.getOnlyElement(eval("config(//test:dep, host)")));

assertThat(configuration).isNotNull();
assertThat(configuration.isHostConfiguration()).isTrue();
assertThat(configuration.isExecConfiguration()).isFalse();
assertThat(configuration.isToolConfiguration()).isTrue();
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,6 @@ public void factorEquivalentNodes() throws Exception {
"}"));
}

// TODO(b/203203933): Replace "host" with "exec" throughout this test.
@Test
public void nullAndHostDeps() throws Exception {
writeFile(
Expand All @@ -160,8 +159,6 @@ public void nullAndHostDeps() throws Exception {
List<String> output = getOutput("deps(//test:a)");
String firstNode = output.get(2);
String configHash = firstNode.substring(firstNode.indexOf("(") + 1, firstNode.length() - 2);
String hostNode = output.get(6);
String execConfigHash = hostNode.substring(hostNode.indexOf("(") + 1, hostNode.length() - 2);
assertThat(getOutput("deps(//test:a)"))
.isEqualTo(
withConfigHash(
Expand All @@ -171,8 +168,8 @@ public void nullAndHostDeps() throws Exception {
" \"//test:a (%s)\"",
" \"//test:a (%s)\" -> \"//test:b (%s)\"",
" \"//test:a (%s)\" -> \"//test:file.src (null)\"",
" \"//test:a (%s)\" -> \"//test:host_dep (" + execConfigHash + ")\"",
" \"//test:host_dep (" + execConfigHash + ")\"",
" \"//test:a (%s)\" -> \"//test:host_dep (HOST)\"",
" \"//test:host_dep (HOST)\"",
" \"//test:file.src (null)\"",
" \"//test:b (%s)\"",
"}"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -697,6 +697,41 @@ public void featureFlagInHostConfiguration_HasNoTransitiveConfigEnforcement() th
assertNoEvents();
}

@Test
public void noDistinctHostConfiguration_DoesNotResultInActionConflicts() throws Exception {
scratch.file(
"test/BUILD",
"load(':host_transition.bzl', 'host_transition')",
"load(':read_flags.bzl', 'read_flags')",
"feature_flag_setter(",
" name = 'target',",
" deps = [':host', ':reader'],",
")",
"host_transition(",
" name = 'host',",
" srcs = [':reader'],",
")",
"read_flags(",
" name = 'reader',",
" flags = [],",
")");

enableManualTrimmingAnd("--nodistinct_host_configuration");
ConfiguredTarget target = getConfiguredTarget("//test:target");
assertNoEvents();
// Note that '//test:reader' is accessed (and creates actions) in both the host and target
// configurations. If these are different but output to the same path (as was the case before
// --nodistinct_host_configuration caused --enforce_transitive_configs_for_config_feature_flag
// to become a no-op), then this causes action conflicts, as described in b/117932061 (for which
// this test is a regression test).
assertThat(getFilesToBuild(target).toList()).hasSize(1);
// Action conflict detection is not enabled for these tests. However, the action conflict comes
// from the outputs of the two configurations of //test:reader being unequal artifacts;
// hence, this test checks that the nested set of artifacts reachable from //test:target only
// contains one artifact, that is, they were deduplicated for being equal.
}


@Test
public void noDistinctHostConfiguration_DisablesEnforcementForBothHostAndTargetConfigs()
throws Exception {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
import com.google.devtools.build.lib.analysis.RunfilesProvider;
import com.google.devtools.build.lib.analysis.actions.SpawnAction;
import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory;
import com.google.devtools.build.lib.analysis.config.HostTransition;
import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.RepositoryName;
Expand All @@ -45,6 +45,7 @@
import com.google.devtools.build.lib.packages.NativeAspectClass;
import com.google.devtools.build.lib.packages.RuleClass;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
import com.google.devtools.build.lib.testutil.TestConstants;
import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
import com.google.devtools.build.lib.util.FileTypeSet;
import java.util.List;
Expand All @@ -69,11 +70,11 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
.add(attr("runtime_deps", LABEL_LIST).allowedFileTypes(FileTypeSet.NO_FILE))
.add(
attr("plugins", LABEL_LIST)
.cfg(ExecutionTransitionFactory.create())
.cfg(HostTransition.createFactory())
.allowedFileTypes(FileTypeSet.NO_FILE))
.add(
attr("exported_plugins", LABEL_LIST)
.cfg(ExecutionTransitionFactory.create())
.cfg(HostTransition.createFactory())
.allowedFileTypes(FileTypeSet.NO_FILE))
.build();
}
Expand Down Expand Up @@ -110,7 +111,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
.add(attr("target_libs", LABEL_LIST).allowedFileTypes(FileTypeSet.NO_FILE))
.add(
attr("host_libs", LABEL_LIST)
.cfg(ExecutionTransitionFactory.create())
.cfg(HostTransition.createFactory())
.allowedFileTypes(FileTypeSet.NO_FILE))
.add(attr("target_attrs", STRING_LIST))
.add(attr("host_attrs", STRING_LIST))
Expand All @@ -119,7 +120,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
.value(Label.parseAbsoluteUnchecked("//test/implicit:implicit_target")))
.add(
attr("$implicit_host", LABEL)
.cfg(ExecutionTransitionFactory.create())
.cfg(HostTransition.createFactory())
.value(Label.parseAbsoluteUnchecked("//test/implicit:implicit_host")))
.build();
}
Expand Down Expand Up @@ -282,7 +283,7 @@ public void testProguardSpecs_originalConfigSentAsInputToAllowlister() throws Ex
.map(path -> path.replaceFirst(TestConstants.PRODUCT_NAME + "-out/[^/]+/", ""))
.collect(Collectors.toList());
List<String> expectedFilesToRun =
getFilesToRun(getConfiguredTarget(TestConstants.PROGUARD_ALLOWLISTER_TARGET))
getFilesToRun(getHostConfiguredTarget(TestConstants.PROGUARD_ALLOWLISTER_TARGET))
.toList()
.stream()
.map(Artifact::getExecPathString)
Expand Down Expand Up @@ -347,11 +348,11 @@ public void testProguardSpecs_pickedUpFromDependencyAttributes() throws Exceptio
Artifact validatedPlugin =
getBinArtifact(
"validated_proguard/plugin/test/plugin.cfg_valid",
getDirectPrerequisite(target, "//test:plugin"));
getHostConfiguredTarget("//test:plugin"));
Artifact validatedExportedPlugin =
getBinArtifact(
"validated_proguard/exported_plugin/test/exported_plugin.cfg_valid",
getDirectPrerequisite(target, "//test:exported_plugin"));
getHostConfiguredTarget("//test:exported_plugin"));

assertThat(getFilesToBuild(target).toList())
.containsExactly(
Expand Down Expand Up @@ -382,16 +383,15 @@ public void testProguardSpecs_customAttributes() throws Exception {
getConfiguredTarget("//test:target"));
Artifact validatedHost =
getBinArtifact(
"validated_proguard/host/test/host.cfg_valid",
getDirectPrerequisite(target, "//test:host"));
"validated_proguard/host/test/host.cfg_valid", getHostConfiguredTarget("//test:host"));
Artifact validatedImplicitTarget =
getBinArtifact(
"validated_proguard/implicit_target/test/implicit/implicit_target.cfg_valid",
getConfiguredTarget("//test/implicit:implicit_target"));
Artifact validatedImplicitHost =
getBinArtifact(
"validated_proguard/implicit_host/test/implicit/implicit_host.cfg_valid",
getDirectPrerequisite(target, "//test/implicit:implicit_host"));
getHostConfiguredTarget("//test/implicit:implicit_host"));

assertThat(getFilesToBuild(target).toList())
.containsExactly(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -759,10 +759,9 @@ public void testAttrBadKeywordArguments() throws Exception {
}

@Test
public void testAttrCfgNoMoreHost() throws Exception {
public void testAttrCfg() throws Exception {
Attribute attr = buildAttribute("a1", "attr.label(cfg = 'host', allow_files = True)");
assertThat(attr.getTransitionFactory().isHost()).isFalse();
assertThat(attr.getTransitionFactory().isTool()).isTrue();
assertThat(attr.getTransitionFactory().isHost()).isTrue();
}

@Test
Expand Down
Loading

0 comments on commit bdc6c5c

Please sign in to comment.