Skip to content

Commit

Permalink
Propagate test envs to xml generation action
Browse files Browse the repository at this point in the history
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
  • Loading branch information
meteorcloudy authored and copybara-github committed Dec 15, 2020
1 parent b1e9884 commit 3551898
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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()),
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
25 changes: 19 additions & 6 deletions tools/test/windows/tw.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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.
Expand All @@ -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;
Expand All @@ -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());
}
});
Expand Down Expand Up @@ -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)) {
Expand All @@ -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;
}

Expand Down

0 comments on commit 3551898

Please sign in to comment.