Only treat "env" and "env_inherit" attrs specially for native rules

This avoids unexpected behavior in the case of Skylark rules that happen to use the same attribute names. Fixes bazelbuild#12758 PiperOrigin-RevId: 349587545
EngFlow · Mar 5, 2021 · 47d92b2 · 47d92b2
1 parent a023c9f
commit 47d92b2
Show file tree

Hide file tree

Showing 4 changed files with 23 additions and 2 deletions.
diff --git a/src/main/java/com/google/devtools/build/docgen/templates/attributes/binary/env.html b/src/main/java/com/google/devtools/build/docgen/templates/attributes/binary/env.html
@@ -6,6 +6,11 @@
   executed by <code>bazel run</code>.
 </p>
 
+<p>
+  This attribute only applies to native rules, like <code>cc_binary</code>, <code>py_binary</code>,
+  and <code>sh_binary</code>.  It does not apply to Starlark-defined executable rules.
+</p>
+
 <p>
 <em class="harmful">NOTE: The environment variables are not set when you run the target
 outside of bazel (for example, by manually executing the binary in

diff --git a/src/main/java/com/google/devtools/build/docgen/templates/attributes/test/env.html b/src/main/java/com/google/devtools/build/docgen/templates/attributes/test/env.html
@@ -6,3 +6,8 @@
 <p>Specifies additional environment variables to set when the test is
   executed by <code>bazel test</code>.
 </p>
+
+<p>
+  This attribute only applies to native rules, like <code>cc_test</code>, <code>py_test</code>,
+  and <code>sh_test</code>.  It does not apply to Starlark-defined test rules.
+</p>
diff --git a/src/main/java/com/google/devtools/build/docgen/templates/attributes/test/env_inherit.html b/src/main/java/com/google/devtools/build/docgen/templates/attributes/test/env_inherit.html
@@ -3,3 +3,8 @@
 <p>Specifies additional environment variables to inherit from the
   external environment when the test is executed by <code>bazel test</code>.
 </p>
+
+<p>
+  This attribute only applies to native rules, like <code>cc_test</code>, <code>py_test</code>,
+  and <code>sh_test</code>.  It does not apply to Starlark-defined test rules.
+</p>
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java
@@ -464,8 +464,14 @@ private static CommandLine computeArgs(RuleContext ruleContext, CommandLine addi
   }
 
   private static ActionEnvironment computeActionEnvironment(RuleContext ruleContext) {
-    if (!ruleContext.getRule().isAttrDefined("env", Type.STRING_DICT)
-        && !ruleContext.getRule().isAttrDefined("env_inherit", Type.STRING_LIST)) {
+    // Currently, "env" and "env_inherit" are not added to Starlark-defined rules (unlike "args"),
+    // in order to avoid breaking existing Starlark rules that use those attribute names.
+    // TODO(brandjon): Support "env" and "env_inherit" for Starlark-defined rules.
+    boolean isNativeRule =
+        ruleContext.getRule().getRuleClassObject().getRuleDefinitionEnvironmentLabel() == null;
+    if (!isNativeRule
+        || (!ruleContext.getRule().isAttrDefined("env", Type.STRING_DICT)
+            && !ruleContext.getRule().isAttrDefined("env_inherit", Type.STRING_LIST))) {
       return ActionEnvironment.EMPTY;
     }
     TreeMap<String, String> fixedEnv = new TreeMap<>();