From ee72abdb61f3c482db318b60fd77f61228789fdc Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 11 Jan 2023 00:07:28 -0800 Subject: [PATCH 1/3] 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 | 49 +++++++++++++++++++ 2 files changed, 57 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 fixedEnvironment = spawnAction.getEnvironment().getFixedEnv(); + ImmutableMap fixedEnvironment = spawnAction.getEffectiveEnvironment(Map.of()); for (Map.Entry 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..85a538c4c65339 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,49 @@ EOF fi } +function test_does_not_fail_horribly_with_file() { + rm -rf peach + mkdir -p peach + cat > "peach/BUILD" <<'EOF' +genrule( + name = "bar", + srcs = ["dummy.txt"], + outs = ["bar_out.txt"], + cmd = "echo unused > bar_out.txt", +) +EOF + + echo "//peach:bar" > query_file + bazel aquery --query_file=query_file > $TEST_log + + expect_log "Target: //peach:bar" "look in $TEST_log" + 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() { From 84e981c8268386e18db92d4de5aebda3ffeab123 Mon Sep 17 00:00:00 2001 From: kshyanashree <109167932+kshyanashree@users.noreply.github.com> Date: Tue, 31 Jan 2023 09:17:37 -0800 Subject: [PATCH 2/3] Removed the function test_does_not_fail_horribly_with_file() --- src/test/shell/integration/aquery_test.sh | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/src/test/shell/integration/aquery_test.sh b/src/test/shell/integration/aquery_test.sh index 85a538c4c65339..83adfcc4f70e28 100755 --- a/src/test/shell/integration/aquery_test.sh +++ b/src/test/shell/integration/aquery_test.sh @@ -1665,25 +1665,6 @@ EOF fi } -function test_does_not_fail_horribly_with_file() { - rm -rf peach - mkdir -p peach - cat > "peach/BUILD" <<'EOF' -genrule( - name = "bar", - srcs = ["dummy.txt"], - outs = ["bar_out.txt"], - cmd = "echo unused > bar_out.txt", -) -EOF - - echo "//peach:bar" > query_file - bazel aquery --query_file=query_file > $TEST_log - - expect_log "Target: //peach:bar" "look in $TEST_log" - expect_log "ActionKey:" -} - function test_cpp_compile_action_env() { local pkg="${FUNCNAME[0]}" mkdir -p "$pkg" From e2e01978dbf08420d77e47d797373ef4fbb5646f Mon Sep 17 00:00:00 2001 From: keertk <110264242+keertk@users.noreply.github.com> Date: Sat, 18 Feb 2023 18:39:42 -0800 Subject: [PATCH 3/3] Update ActionGraphDump.java --- .../build/lib/skyframe/actiongraph/v2/ActionGraphDump.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 fd1ca55c383cee..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 @@ -181,7 +181,7 @@ private void dumpSingleAction(ConfiguredTarget configuredTarget, ActionAnalysisM // 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. + // environment as well. ImmutableMap fixedEnvironment = spawnAction.getEffectiveEnvironment(Map.of()); for (Map.Entry environmentVariable : fixedEnvironment.entrySet()) { actionBuilder.addEnvironmentVariables(