From 3ab8a0a5d628a0d958fb2eb1c0d5bb76b442e2f2 Mon Sep 17 00:00:00 2001
From: kshyanashree <109167932+kshyanashree@users.noreply.github.com>
Date: Tue, 21 Feb 2023 05:03:20 -0800
Subject: [PATCH] [6.1.0]Let `aquery` print effective environment for all
 `CommandAction`s (#17274)

* Let `aquery` print effective environment for all `CommandAction`s

Instead of printing the fixed part of the `ActionEnvironment` of all `SpawnActions`, `aquery` now prints the effective environment resolved against an empty client environment of all `AbstractAction`s that implement `CommandAction`.

For `SpawnAction`s, this should not result in any observable change, but C++ actions, which only override `AbstractAction#getEffectiveEnvironment`, now have their fixed environments (e.g. toolchain env vars such as `INCLUDE` with MSVC) emitted.

Work towards #12852

Closes #17108.

PiperOrigin-RevId: 501198850
Change-Id: I035a8cfde32193ca7f1f71030bd183c20edfdc0d

* Removed the function test_does_not_fail_horribly_with_file()

---------

Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>
---
 .../actiongraph/v2/ActionGraphDump.java       | 14 +++++----
 src/test/shell/integration/aquery_test.sh     | 30 +++++++++++++++++++
 2 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/actiongraph/v2/ActionGraphDump.java b/src/main/java/com/google/devtools/build/lib/skyframe/actiongraph/v2/ActionGraphDump.java
index 5264c1fd7aea04..12d50a5732d9c1 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/actiongraph/v2/ActionGraphDump.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/actiongraph/v2/ActionGraphDump.java
@@ -18,6 +18,7 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
+import com.google.devtools.build.lib.actions.AbstractAction;
 import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
 import com.google.devtools.build.lib.actions.ActionExecutionMetadata;
 import com.google.devtools.build.lib.actions.ActionKeyContext;
@@ -32,7 +33,6 @@
 import com.google.devtools.build.lib.analysis.SourceManifestAction;
 import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
 import com.google.devtools.build.lib.analysis.actions.ParameterFileWriteAction;
-import com.google.devtools.build.lib.analysis.actions.SpawnAction;
 import com.google.devtools.build.lib.analysis.actions.Substitution;
 import com.google.devtools.build.lib.analysis.actions.TemplateExpansionAction;
 import com.google.devtools.build.lib.analysis.actions.TemplateExpansionException;
@@ -174,11 +174,15 @@ private void dumpSingleAction(ConfiguredTarget configuredTarget, ActionAnalysisM
     }
 
     // store environment
-    if (action instanceof SpawnAction) {
-      SpawnAction spawnAction = (SpawnAction) action;
+    if (action instanceof AbstractAction && action instanceof CommandAction) {
+      AbstractAction spawnAction = (AbstractAction) action;
+      // Some actions (e.g. CppCompileAction) don't override getEnvironment, but only
+      // getEffectiveEnvironment. Since calling the latter with an empty client env returns the
+      // fixed part of the full ActionEnvironment with the default implementations provided by
+      // AbstractAction, we can call getEffectiveEnvironment here to handle these actions as well.
       // TODO(twerth): This handles the fixed environment. We probably want to output the inherited
-      // environment as well.
-      ImmutableMap<String, String> fixedEnvironment = spawnAction.getEnvironment().getFixedEnv();
+      // environment as well. 
+      ImmutableMap<String, String> fixedEnvironment = spawnAction.getEffectiveEnvironment(Map.of());
       for (Map.Entry<String, String> environmentVariable : fixedEnvironment.entrySet()) {
         actionBuilder.addEnvironmentVariables(
             AnalysisProtosV2.KeyValuePair.newBuilder()
diff --git a/src/test/shell/integration/aquery_test.sh b/src/test/shell/integration/aquery_test.sh
index 9493d3e34b8ec7..83adfcc4f70e28 100755
--- a/src/test/shell/integration/aquery_test.sh
+++ b/src/test/shell/integration/aquery_test.sh
@@ -30,9 +30,15 @@ source "$(rlocation "io_bazel/src/test/shell/integration_test_setup.sh")" \
 
 case "$(uname -s | tr [:upper:] [:lower:])" in
 msys*|mingw*|cygwin*)
+  declare -r is_macos=false
   declare -r is_windows=true
   ;;
+darwin)
+  declare -r is_macos=true
+  declare -r is_windows=false
+  ;;
 *)
+  declare -r is_macos=false
   declare -r is_windows=false
   ;;
 esac
@@ -1659,6 +1665,30 @@ EOF
   fi
 }
 
+function test_cpp_compile_action_env() {
+  local pkg="${FUNCNAME[0]}"
+  mkdir -p "$pkg"
+
+  touch "$pkg/main.cpp"
+  cat > "$pkg/BUILD" <<'EOF'
+cc_binary(
+    name = "main",
+    srcs = ["main.cpp"],
+)
+EOF
+  bazel aquery --output=textproto \
+     "mnemonic(CppCompile,//$pkg:main)" >output 2> "$TEST_log" || fail "Expected success"
+  cat output >> "$TEST_log"
+
+  if "$is_macos"; then
+    assert_contains '  key: "XCODE_VERSION_OVERRIDE"' output
+  elif "$is_windows"; then
+    assert_contains '  key: "INCLUDE"' output
+  else
+    assert_contains '  key: "PWD"' output
+  fi
+}
+
 # TODO(bazel-team): The non-text aquery output formats don't correctly handle
 # non-ASCII fields (input/output paths, environment variables, etc).
 function DISABLED_test_unicode_textproto() {