Repository: bazelbuild/bazel
Issue: SDKROOT, DEVELOPER_DIR, INCLUDE, LIB missing from Environment Variables in Aquery and Extra Actions
Labels: type: bug
P4
platform: apple
team-Rules-CPP
team-Rules-ObjC

### Description of the problem:

When listening to CppCompile and ObjcCompile (mnemonic) actions on macOS and Windows, the key environment variables DEVELOPER_DIR and SDKROOT (Apple Platforms) and INCLUDE and LIB and [other essentials](https://docs.microsoft.com/en-us/cpp/build/building-on-the-command-line) (Windows) are missing from the environment variables in aquery and extra actions output, despite being needed to run the action successfully.

For Apple platforms, the [bazel-supplied, wrapped Xcode compiler](https://github.com/bazelbuild/bazel/blob/master/tools/osx/crosstool/wrapped_clang.cc) is dependent on these environment variables, and crashes if they aren't provided. For Windows, MSVC is dependent on these variables for system headers and libraries. Those environment variables provide key information about the compilation command.

On Apple platforms, some other, less-standard variables are supplied: XCODE_VERSION_OVERRIDE, APPLE_SDK_VERSION_OVERRIDE, and APPLE_SDK_PLATFORM. On Windows...not so much.

### What underlying problem are you trying to solve?

The lack of these variables significantly complicates building C++ developer tooling around Bazel. 

In my case, I'm [integrating Bazel with clangd autocomplete in a wide variety of editors](https://github.com/hedronvision/bazel-compile-commands-extractor), to make development using Bazel more fun. We're linked to from the bazel docs :) 

But I'd imagine this would also be useful for Kythe--the complication databases it generates would be incomplete without this, and they use action_listeners/extra_action, while we use aquery. As above, the problem exists in both.

The workaround I'm currently using on macOS using is to go direct to the system, re-inferring SDKROOT and DEVELOPER_DIR from the system Xcode commands. On Windows, I inspect the top level cc_toolchain. While these generally work, they'll miss any configuration the user does in the build graph. 

More generally, it seems odd that those key variables are missing for the core cc rules. It feels like they'd be the primary purpose of being able to query the environment of C++ actions. It feels like there might be something even more general broken here.

### Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Just run `bazel aquery "mnemonic('(Objc|Cpp)Compile',//...)"` on a cc_library on macOS or Windows and look at the (lack of) those environment variables.

Reproing the extra action issue is more involved, and probably is just another manifestation of the same underlying problem. But I think the fastest way to do that would be to grab Kythe's implementation of an extractor (https://github.com/kythe/kythe/blob/master/kythe/cxx/extractor/README.md) and examine the extra_actions_base_proto.


Issue comments:
As an add-on: Seems like all these variables are missing from aquery output, too.
It seems like maybe there's something much more general broken with environment variable reporting for actions.

I ran back into an equivalent problem on Windows and with aquery, and it really does complicate and embrittle tooling.

I've updated the original report to reflect that.

@gregestren @oquenchil, might this merit different labels/priorities if its a more general, non-apple-specific, non-extra-action-specific bug?


I don't have expertise on this part of the code base (aquery, extra actions, environment variable consumption). I'm going to defer to @oquenchil .
This is blocking us too. Is there any meaningful progress here?
(Sadly not that I know of.) But I'm curious what you're building on top of aquery/extra_actions, @sualehasif!
I think this may be an _even_ more general bug where toolchain environment variables are missing from aquery output.

(I've been trying to help a user over at https://github.com/hedronvision/bazel-compile-commands-extractor/issues/83#issuecomment-1312067222 and noticing that EM_BIN_PATH and the other emscripten environment variables are missing from aquery output, despite being set in [the toolchain](https://github.com/emscripten-core/emsdk/blob/005063304bb4e575f97b7c5f50368f937c06cb54/bazel/emscripten_toolchain/toolchain.bzl))

@oquenchil, any chance of a higher prioritization, given it's now clear it's a more general issue (not platform-specific), present across the action querying mechanisms (not just extra action), and causing problems for others, too?
Is this just for C++ rules and it works for others like Java?
I'm pretty sure it also affects Objective-C, at least, but I'm not sure whether it affects other language toolchains/Java.
[It's been a problem everywhere I've looked, but I haven't needed to dive into the Bazel toolchains for other languages just yet.] 
This is complicating tooling for me as well. I've implemented a toolchain to use the EWDK to build windows drivers, and it is a real bummer to not be able to query the INCLUDE environment variable for this.
@meisterT is this something that aquery could get to?
Regarding `DEVELOPER_DIR` and `SDKROOT`: I suspect that these are missing from `aquery` output as the real values for these variable are only determined at execution time by running `xcode-locator` and are then injected into spawns that need them (see https://cs.opensource.google/bazel/bazel/+/abe26b84df03eac149f731f1e71fd9a5f0138f0a:src/main/java/com/google/devtools/build/lib/exec/local/XcodeLocalEnvProvider.java;l=68). Ultimately that seems to be necessary as the values have to be host-dependent absolute paths and thus shouldn't be part of the action key.
Regarding the missing toolchain variables on all OSes: Based on a cursory glance, it looks to me as if `aquery` only emits environment variables for `SpawnAction`s: https://cs.opensource.google/bazel/bazel/+/abe26b84df03eac149f731f1e71fd9a5f0138f0a:src/main/java/com/google/devtools/build/lib/skyframe/actiongraph/v2/ActionGraphDump.java;l=177. But the C++ compilation and linking actions aren't `SpawnAction`s. Maybe that `if` could just check for `AbstractAction` instead?
Thanks so much for diving in, guys.
Can confirm that @fmeum's good work in https://github.com/bazelbuild/bazel/pull/17108 surfaced the emscripten environment variables--but the Apple (and I presume the Windows) variables from the original issue are still missing.

Still think this might be worth fixing (at least by starlarkificiation time). 

Pull request review comments:
@kshyanashree This test made it in by mistake, only the one after it is part of the original PR.
Hi @fmeum! I removed this test and ran the presubmits, but still checks are failing. Could you please look into it?

Patch that resolved the issue:
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()


Test patch:
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() {