From 3551898849a93306ad9b4dfdd7d4667913098efe Mon Sep 17 00:00:00 2001 From: Yun Peng Date: Tue, 15 Dec 2020 06:04:28 -0800 Subject: [PATCH] Propagate test envs to xml generation action Previously, we hardcode the envs of the xml generation action, which caused problem for process-wrapper because it's dynamically linked to some system libraries and the required PATH or LD_LIBRARY_PATH are not set. This change propagate the envs we set for the actual test action to the xml file generation action to make sure the env vars are correctly set and can also be controlled by --action_env and --test_env. Fixes https://github.com/bazelbuild/bazel/issues/4137 Fixes https://github.com/bazelbuild/bazel/issues/12579 Closes #12659. PiperOrigin-RevId: 347596753 --- .../lib/exec/StandaloneTestStrategy.java | 27 +++++++++++-------- .../lib/exec/StandaloneTestStrategyTest.java | 2 +- tools/test/windows/tw.cc | 25 ++++++++++++----- 3 files changed, 36 insertions(+), 18 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java index fd1214daf9cedb..cdc4b8de162cec 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java @@ -391,7 +391,8 @@ private static BuildEventStreamProtos.TestResult.ExecutionInfo extractExecutionI * A spawn to generate a test.xml file from the test log. This is only used if the test does not * generate a test.xml file itself. */ - private static Spawn createXmlGeneratingSpawn(TestRunnerAction action, SpawnResult result) { + private static Spawn createXmlGeneratingSpawn( + TestRunnerAction action, ImmutableMap testEnv, SpawnResult result) { ImmutableList args = ImmutableList.of( action.getTestXmlGeneratorScript().getExecPath().getCallablePathString(), @@ -399,18 +400,21 @@ private static Spawn createXmlGeneratingSpawn(TestRunnerAction action, SpawnResu action.getXmlOutputPath().getPathString(), Long.toString(result.getWallTime().orElse(Duration.ZERO).getSeconds()), Integer.toString(result.exitCode())); - - String testBinaryName = - action.getExecutionSettings().getExecutable().getRootRelativePath().getCallablePathString(); + ImmutableMap.Builder envBuilder = ImmutableMap.builder(); + // "PATH" and "TEST_BINARY" are also required, they should always be set in testEnv. + Preconditions.checkArgument(testEnv.containsKey("PATH")); + Preconditions.checkArgument(testEnv.containsKey("TEST_BINARY")); + envBuilder.putAll(testEnv).put("TEST_NAME", action.getTestName()); + // testEnv only contains TEST_SHARD_INDEX and TEST_TOTAL_SHARDS if the test action is sharded, + // we need to set the default value when the action isn't sharded. + if (!action.isSharded()) { + envBuilder.put("TEST_SHARD_INDEX", "0"); + envBuilder.put("TEST_TOTAL_SHARDS", "0"); + } return new SimpleSpawn( action, args, - ImmutableMap.of( - "PATH", "/usr/bin:/bin", - "TEST_SHARD_INDEX", Integer.toString(action.getShardNum()), - "TEST_TOTAL_SHARDS", Integer.toString(action.getExecutionSettings().getTotalShards()), - "TEST_NAME", action.getTestName(), - "TEST_BINARY", testBinaryName), + envBuilder.build(), // Pass the execution info of the action which is identical to the supported tags set on the // test target. In particular, this does not set the test timeout on the spawn. ImmutableMap.copyOf(action.getExecutionInfo()), @@ -766,7 +770,8 @@ public TestAttemptContinuation execute() if (executionOptions.splitXmlGeneration && fileOutErr.getOutputPath().exists() && !xmlOutputPath.exists()) { - Spawn xmlGeneratingSpawn = createXmlGeneratingSpawn(testAction, spawnResults.get(0)); + Spawn xmlGeneratingSpawn = + createXmlGeneratingSpawn(testAction, spawn.getEnvironment(), spawnResults.get(0)); SpawnStrategyResolver spawnStrategyResolver = actionExecutionContext.getContext(SpawnStrategyResolver.class); // We treat all failures to generate the test.xml here as catastrophic, and won't rerun diff --git a/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java b/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java index be2f78cf9e6b27..8672f92b4d3877 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java @@ -143,7 +143,7 @@ public FakeActionExecutionContext( LostInputsCheck.NONE, fileOutErr, /*eventHandler=*/ null, - /*clientEnv=*/ ImmutableMap.of(), + /*clientEnv=*/ ImmutableMap.of("PATH", "/usr/bin:/bin"), /*topLevelFilesets=*/ ImmutableMap.of(), /*artifactExpander=*/ null, /*actionFileSystem=*/ null, diff --git a/tools/test/windows/tw.cc b/tools/test/windows/tw.cc index 70677bdca4e986..d0338a2f030572 100644 --- a/tools/test/windows/tw.cc +++ b/tools/test/windows/tw.cc @@ -189,6 +189,9 @@ struct Duration { bool FromString(const wchar_t* str); }; +enum class MainType { kTestWrapperMain, kXmlWriterMain }; +enum class DeleteAfterwards { kEnabled, kDisabled }; + void WriteStdout(const std::string& s) { DWORD written; WriteFile(GetStdHandle(STD_OUTPUT_HANDLE), s.c_str(), s.size(), &written, @@ -1603,9 +1606,16 @@ std::string CreateErrorTag(int exit_code) { } } -bool ShouldCreateXml(const Path& xml_log, bool* result) { +bool ShouldCreateXml(const Path& xml_log, const MainType main_type, + bool* result) { *result = true; + // If running from the xml generator binary, we should always create the xml + // file. + if (main_type == MainType::kXmlWriterMain) { + return true; + } + DWORD attr = GetFileAttributesW(AddUncPrefixMaybe(xml_log).c_str()); if (attr != INVALID_FILE_ATTRIBUTES) { // The XML file already exists, maybe the test framework wrote it. @@ -1630,9 +1640,10 @@ bool ShouldCreateXml(const Path& xml_log, bool* result) { bool CreateXmlLog(const Path& output, const Path& test_outerr, const Duration duration, const int exit_code, - const bool delete_afterwards) { + const DeleteAfterwards delete_afterwards, + const MainType main_type) { bool should_create_xml; - if (!ShouldCreateXml(output, &should_create_xml)) { + if (!ShouldCreateXml(output, main_type, &should_create_xml)) { LogErrorWithArg(__LINE__, "Failed to decide if XML log is needed", output.Get()); return false; @@ -1645,7 +1656,7 @@ bool CreateXmlLog(const Path& output, const Path& test_outerr, // Delete the test's outerr file after we have the XML file. // We don't care if this succeeds or not, because the outerr file is not a // declared output. - if (delete_afterwards) { + if (delete_afterwards == DeleteAfterwards::kEnabled) { DeleteFileW(test_outerr.Get().c_str()); } }); @@ -1904,7 +1915,8 @@ int TestWrapperMain(int argc, wchar_t** argv) { Duration test_duration; int result = RunSubprocess(test_path, args, test_outerr, &test_duration); - if (!CreateXmlLog(xml_log, test_outerr, test_duration, result, true) || + if (!CreateXmlLog(xml_log, test_outerr, test_duration, result, + DeleteAfterwards::kEnabled, MainType::kTestWrapperMain) || !ArchiveUndeclaredOutputs(undecl) || !CreateUndeclaredOutputsAnnotations(undecl.annotations_dir, undecl.annotations)) { @@ -1921,7 +1933,8 @@ int XmlWriterMain(int argc, wchar_t** argv) { if (!GetCwd(&cwd) || !ParseXmlWriterArgs(argc, argv, cwd, &test_outerr, &test_xml_log, &duration, &exit_code) || - !CreateXmlLog(test_xml_log, test_outerr, duration, exit_code, false)) { + !CreateXmlLog(test_xml_log, test_outerr, duration, exit_code, + DeleteAfterwards::kDisabled, MainType::kXmlWriterMain)) { return 1; }