Change host transitions to execution transitions in genrule.

RELNOTES[INC]: genrule switched to use exec transition instead of host. This can break targets with hardcoded output paths. To avoid using hardcoded paths use make variables, see https://docs.bazel.build/versions/4.2.2/be/make-variables.html#predefined_label_variables PiperOrigin-RevId: 420943186
bazelbuild · Jan 11, 2022 · d988e8b · d988e8b
1 parent bcd11ae
commit d988e8b
Show file tree

Hide file tree

Showing 10 changed files with 46 additions and 17 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BUILD
@@ -15,7 +15,7 @@ java_library(
     srcs = glob(["*.java"]),
     deps = [
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
-        "//src/main/java/com/google/devtools/build/lib/analysis:config/host_transition",
+        "//src/main/java/com/google/devtools/build/lib/analysis:config/execution_transition_factory",
         "//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment",
         "//src/main/java/com/google/devtools/build/lib/packages",
         "//src/main/java/com/google/devtools/build/lib/rules/genrule",

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRuleRule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRuleRule.java
@@ -19,7 +19,7 @@
 
 import com.google.devtools.build.lib.analysis.RuleDefinition;
 import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
-import com.google.devtools.build.lib.analysis.config.HostTransition;
+import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory;
 import com.google.devtools.build.lib.packages.RuleClass;
 import com.google.devtools.build.lib.rules.genrule.GenRuleBaseRule;
 
@@ -41,7 +41,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
         .setOutputToGenfiles()
         .add(
             attr("$genrule_setup", LABEL)
-                .cfg(HostTransition.createFactory())
+                .cfg(ExecutionTransitionFactory.create())
                 .value(env.getToolsLabel(GENRULE_SETUP_LABEL)))
 
         // TODO(bazel-team): stamping doesn't seem to work. Fix it or remove attribute.

diff --git a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBaseRule.java b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBaseRule.java
@@ -24,7 +24,6 @@
 import com.google.devtools.build.lib.analysis.RuleDefinition;
 import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
 import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory;
-import com.google.devtools.build.lib.analysis.config.HostTransition;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.AttributeMap;
 import com.google.devtools.build.lib.packages.BuildType;
@@ -83,7 +82,7 @@ public RuleClass build(
         <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
         .add(
             attr("tools", LABEL_LIST)
-                .cfg(HostTransition.createFactory())
+                .cfg(ExecutionTransitionFactory.create())
                 .allowedFileTypes(FileTypeSet.ANY_FILE))
 
         /* <!-- #BLAZE_RULE(genrule).ATTRIBUTE(exec_tools) -->

diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/BUILD b/src/main/java/com/google/devtools/build/lib/rules/python/BUILD
@@ -22,6 +22,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/build_options",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/build_options_cache",
+        "//src/main/java/com/google/devtools/build/lib/analysis:config/core_options",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/configuration_transition",

diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java
@@ -640,7 +640,7 @@ public boolean shouldWarnAboutHostVersionUponFailure() {
       return false;
     }
     // Only warn in the host config.
-    if (!ruleContext.getConfiguration().isHostConfiguration()) {
+    if (!ruleContext.getConfiguration().isToolConfiguration()) {
       return false;
     }
 

diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonVersionTransition.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonVersionTransition.java
@@ -21,6 +21,7 @@
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.analysis.config.BuildOptionsCache;
 import com.google.devtools.build.lib.analysis.config.BuildOptionsView;
+import com.google.devtools.build.lib.analysis.config.CoreOptions;
 import com.google.devtools.build.lib.analysis.config.FragmentOptions;
 import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
 import com.google.devtools.build.lib.events.EventHandler;
@@ -68,12 +69,17 @@ private PythonVersionTransition() {}
 
   @Override
   public ImmutableSet<Class<? extends FragmentOptions>> requiresOptionFragments() {
-    return ImmutableSet.of(PythonOptions.class);
+    return ImmutableSet.of(PythonOptions.class, CoreOptions.class);
   }
 
   @Override
   public BuildOptions patch(BuildOptionsView options, EventHandler eventHandler) {
-    PythonVersion newVersion = determineNewVersion(options);
+    // If this happens after exec transition, keep the same version (to reproduce and keep behaviour
+    // of the host transition, that happens after this one)
+    PythonVersion newVersion =
+        options.get(CoreOptions.class).isExec
+            ? options.get(PythonOptions.class).getPythonVersion()
+            : determineNewVersion(options);
     checkArgument(newVersion.isTargetValue(), newVersion);
 
     PythonOptions opts = options.get(PythonOptions.class);

diff --git a/...st/java/com/google/devtools/build/lib/analysis/ConfigurationsForLateBoundTargetsTest.java b/...st/java/com/google/devtools/build/lib/analysis/ConfigurationsForLateBoundTargetsTest.java
@@ -18,6 +18,7 @@
 import static com.google.devtools.build.lib.packages.Attribute.attr;
 import static com.google.devtools.build.lib.packages.BuildType.LABEL;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
@@ -121,11 +122,17 @@ public void lateBoundAttributeInHostConfiguration() throws Exception {
         "    name = 'latebound_dep')");
     update("//foo:gen");
     assertThat(getConfiguredTarget("//foo:foo", getHostConfiguration())).isNotNull();
-    ConfiguredTarget dep =
-        Iterables.getOnlyElement(
+    // TODO(b/203203933) Fix LateboundDefault-s to return exec configuration
+    ImmutableList<ConfiguredTarget> deps =
+        ImmutableList.copyOf(
             SkyframeExecutorTestUtils.getExistingConfiguredTargets(
                 skyframeExecutor, Label.parseAbsolute("//foo:latebound_dep", ImmutableMap.of())));
-    assertThat(getConfiguration(dep)).isEqualTo(getHostConfiguration());
+    assertThat(deps).hasSize(2);
+    ConfiguredTarget dep =
+        deps.stream()
+            .filter(d -> getConfiguration(d).equals(getHostConfiguration()))
+            .findFirst()
+            .get();
     // This is technically redundant, but slightly stronger in checking that the host configuration
     // doesn't happen to match what the patch would have done.
     assertThat(LateBoundSplitUtil.getOptions(getConfiguration(dep)).fooFlag).isEmpty();

diff --git a/...t/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java b/...t/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java
@@ -417,7 +417,7 @@ public void testToolsAreHostConfiguration() throws Exception {
           foundSrc = true;
           break;
         case "tool":
-          assertConfigurationsEqual(getHostConfiguration(), getConfiguration(prereq));
+          assertThat(getConfiguration(prereq).isToolConfiguration()).isTrue();
           foundTool = true;
           break;
         case GENRULE_SETUP_PATH:

diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsTest.java
@@ -267,13 +267,28 @@ public void targetDeps() throws Exception {
     }
   }
 
+  /**
+   * Tests dependencies in attribute with host transition.
+   *
+   * <p>Note: This cannot be used to test exec transition, because mocks don't set up toolchain
+   * contexts.
+   */
   @Test
   public void hostDeps() throws Exception {
+    scratch.file(
+        "a/host_rule.bzl",
+        "host_rule = rule(",
+        "  implementation = lambda ctx: [],",
+        "  attrs = {'tools': attr.label_list(cfg = 'host')},",
+        ")");
     scratch.file(
         "a/BUILD",
+        "load('//a:host_rule.bzl', 'host_rule')",
         "cc_binary(name = 'host_tool', srcs = ['host_tool.cc'])",
-        "genrule(name = 'gen', srcs = [], cmd = '', outs = ['gen.out'], tools = [':host_tool'])");
+        "host_rule(name = 'gen', tools = [':host_tool'])");
+
     ConfiguredTarget toolDep = Iterables.getOnlyElement(getConfiguredDeps("//a:gen", "tools"));
+
     assertThat(getConfiguration(toolDep).isHostConfiguration()).isTrue();
   }
 

diff --git a/src/test/py/bazel/runfiles_test.py b/src/test/py/bazel/runfiles_test.py
@@ -305,12 +305,13 @@ def testLegacyExternalRunfilesOption(self):
     exit_code, _, stderr = self.RunBazel(
         args=["build", "--nolegacy_external_runfiles", ":gen"], cwd=work_dir)
     self.AssertExitCode(exit_code, 0, stderr)
+    [exec_dir] = [f for f in os.listdir(bazel_output) if "exec" in f]
     if self.IsWindows():
-      manifest_path = os.path.join(bazel_output,
-                                   "host/bin/bin.exe.runfiles_manifest")
+      manifest_path = os.path.join(bazel_output, exec_dir,
+                                   "bin/bin.exe.runfiles_manifest")
     else:
-      manifest_path = os.path.join(bazel_output,
-                                   "host/bin/bin.runfiles_manifest")
+      manifest_path = os.path.join(bazel_output, exec_dir,
+                                   "bin/bin.runfiles_manifest")
     self.AssertFileContentNotContains(manifest_path, "__main__/external/A")