From 2ae409bc88aaab5683f1890acec21c337b73a812 Mon Sep 17 00:00:00 2001
From: Fabian Meumertzheim <fabian@meumertzhe.im>
Date: Wed, 11 Jan 2023 00:07:28 -0800
Subject: [PATCH] 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
---
 .../actiongraph/v2/ActionGraphDump.java       | 12 +++++---
 src/test/shell/integration/aquery_test.sh     | 30 +++++++++++++++++++
 2 files changed, 38 insertions(+), 4 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..fd1ca55c383cee 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();
+      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 0e5a32ed6c462a..070d45bd1aca90 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
@@ -1679,6 +1685,30 @@ EOF
   expect_log "ActionKey:"
 }
 
+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() {