Propagate test envs to xml generation action

meteorcloudy · copybara-github · commit 3551898849a9 · 2020-12-15T06:05:23.000-08:00
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 #4137 Fixes #12579 Closes #12659. PiperOrigin-RevId: 347596753
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
@@ -391,26 +391,30 @@ 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<String, String> testEnv, SpawnResult result) {
     ImmutableList<String> args =
         ImmutableList.of(
             action.getTestXmlGeneratorScript().getExecPath().getCallablePathString(),
             action.getTestLog().getExecPathString(),
             action.getXmlOutputPath().getPathString(),
             Long.toString(result.getWallTime().orElse(Duration.ZERO).getSeconds()),
             Integer.toString(result.exitCode()));
-
-    String testBinaryName =
-        action.getExecutionSettings().getExecutable().getRootRelativePath().getCallablePathString();
+    ImmutableMap.Builder<String, String> 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
@@ -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
@@ -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;
   }
 
