From bb5ec5c9b484770e0b0f470da723bdce442a090b Mon Sep 17 00:00:00 2001 From: Kyle Edwards Date: Fri, 2 Jun 2023 10:56:48 -0400 Subject: cmUVProcessChain: Do some internal refactoring Move most of cmUVProcessChain::InternalData::Finish() to Prepare() so that error codes can be checked before attepting to spawn any processes. Check some error codes that weren't being checked before. Change return type of Finish() to void as it can't fail. --- Source/cmUVProcessChain.cxx | 46 ++++++++++++++++++++------------------------- 1 file changed, 20 insertions(+), 26 deletions(-) diff --git a/Source/cmUVProcessChain.cxx b/Source/cmUVProcessChain.cxx index ed5f38b..436ada2 100644 --- a/Source/cmUVProcessChain.cxx +++ b/Source/cmUVProcessChain.cxx @@ -66,6 +66,7 @@ struct cmUVProcessChain::InternalData StreamData OutputStreamData; StreamData ErrorStreamData; + cm::uv_pipe_ptr TempErrorPipe; unsigned int ProcessesCompleted = 0; std::vector> Processes; @@ -73,7 +74,7 @@ struct cmUVProcessChain::InternalData bool Prepare(const cmUVProcessChainBuilder* builder); bool AddCommand(const cmUVProcessChainBuilder::ProcessConfiguration& config, bool first, bool last); - bool Finish(); + void Finish(); static const Status* GetStatus(const ProcessData& data); }; @@ -207,12 +208,23 @@ bool cmUVProcessChain::InternalData::Prepare( return false; } - errorData.BuiltinStream.init(*this->Loop, 0); + if (errorData.BuiltinStream.init(*this->Loop, 0) < 0) { + return false; + } if (uv_pipe_open(errorData.BuiltinStream, pipeFd[0]) < 0) { return false; } errorData.Stdio.flags = UV_INHERIT_FD; errorData.Stdio.data.fd = pipeFd[1]; + + if (this->TempErrorPipe.init(*this->Loop, 0) < 0) { + return false; + } + if (uv_pipe_open(this->TempErrorPipe, errorData.Stdio.data.fd) < 0) { + return false; + } + + errorData.Streambuf.open(errorData.BuiltinStream); break; } @@ -235,10 +247,13 @@ bool cmUVProcessChain::InternalData::Prepare( outputData.Stdio.flags = UV_INHERIT_FD; outputData.Stdio.data.fd = errorData.Stdio.data.fd; } else { - outputData.BuiltinStream.init(*this->Loop, 0); + if (outputData.BuiltinStream.init(*this->Loop, 0) < 0) { + return false; + } outputData.Stdio.flags = static_cast(UV_CREATE_PIPE | UV_WRITABLE_PIPE); outputData.Stdio.data.stream = outputData.BuiltinStream; + outputData.Streambuf.open(outputData.BuiltinStream); } break; @@ -313,31 +328,10 @@ bool cmUVProcessChain::InternalData::AddCommand( return process.Process.spawn(*this->Loop, options, &process) >= 0; } -bool cmUVProcessChain::InternalData::Finish() +void cmUVProcessChain::InternalData::Finish() { - if (this->Builder->Stdio[cmUVProcessChainBuilder::Stream_OUTPUT].Type == - cmUVProcessChainBuilder::Builtin && - !this->Builder->MergedBuiltinStreams) { - this->OutputStreamData.Streambuf.open( - this->OutputStreamData.BuiltinStream); - } - - if (this->Builder->Stdio[cmUVProcessChainBuilder::Stream_ERROR].Type == - cmUVProcessChainBuilder::Builtin) { - cm::uv_pipe_ptr tmpPipe; - if (tmpPipe.init(*this->Loop, 0) < 0) { - return false; - } - if (uv_pipe_open(tmpPipe, this->ErrorStreamData.Stdio.data.fd) < 0) { - return false; - } - tmpPipe.reset(); - - this->ErrorStreamData.Streambuf.open(this->ErrorStreamData.BuiltinStream); - } - + this->TempErrorPipe.reset(); this->Valid = true; - return true; } cmUVProcessChain::cmUVProcessChain() -- cgit v0.12 From 5be0cd9f3c07b60b1f60ea678c1d82c38185ca27 Mon Sep 17 00:00:00 2001 From: Kyle Edwards Date: Fri, 2 Jun 2023 14:34:42 -0400 Subject: cmUVProcessChain: Manually create pipes between processes Previously, we were letting libuv create the output pipe for each process. However, in order to facilitate use cases where process spawn is expected to fail (in the case of a missing executable), we want pipe creation to be separate from process creation. Manually create all of the pipes. --- Source/cmUVProcessChain.cxx | 91 +++++++++++++++++++++++++++++++++------------ 1 file changed, 68 insertions(+), 23 deletions(-) diff --git a/Source/cmUVProcessChain.cxx b/Source/cmUVProcessChain.cxx index 436ada2..6517e60 100644 --- a/Source/cmUVProcessChain.cxx +++ b/Source/cmUVProcessChain.cxx @@ -5,11 +5,9 @@ #include "cmUVProcessChain.h" #include -#include #include #include #include // IWYU pragma: keep -#include #include #include @@ -53,6 +51,7 @@ struct cmUVProcessChain::InternalData { cmUVProcessChain::InternalData* Data; cm::uv_process_ptr Process; + cm::uv_pipe_ptr InputPipe; cm::uv_pipe_ptr OutputPipe; bool Finished = false; Status ProcessStatus; @@ -66,14 +65,17 @@ struct cmUVProcessChain::InternalData StreamData OutputStreamData; StreamData ErrorStreamData; + cm::uv_pipe_ptr TempOutputPipe; cm::uv_pipe_ptr TempErrorPipe; unsigned int ProcessesCompleted = 0; std::vector> Processes; bool Prepare(const cmUVProcessChainBuilder* builder); - bool AddCommand(const cmUVProcessChainBuilder::ProcessConfiguration& config, - bool first, bool last); + bool SpawnProcess( + std::size_t index, + const cmUVProcessChainBuilder::ProcessConfiguration& config, bool first, + bool last); void Finish(); static const Status* GetStatus(const ProcessData& data); @@ -168,9 +170,9 @@ cmUVProcessChain cmUVProcessChainBuilder::Start() const return chain; } - for (auto it = this->Processes.begin(); it != this->Processes.end(); ++it) { - if (!chain.Data->AddCommand(*it, it == this->Processes.begin(), - it == std::prev(this->Processes.end()))) { + for (std::size_t i = 0; i < this->Processes.size(); i++) { + if (!chain.Data->SpawnProcess(i, this->Processes[i], i == 0, + i == this->Processes.size() - 1)) { return chain; } } @@ -247,12 +249,27 @@ bool cmUVProcessChain::InternalData::Prepare( outputData.Stdio.flags = UV_INHERIT_FD; outputData.Stdio.data.fd = errorData.Stdio.data.fd; } else { + int pipeFd[2]; + if (cmGetPipes(pipeFd) < 0) { + return false; + } + if (outputData.BuiltinStream.init(*this->Loop, 0) < 0) { return false; } - outputData.Stdio.flags = - static_cast(UV_CREATE_PIPE | UV_WRITABLE_PIPE); - outputData.Stdio.data.stream = outputData.BuiltinStream; + if (uv_pipe_open(outputData.BuiltinStream, pipeFd[0]) < 0) { + return false; + } + outputData.Stdio.flags = UV_INHERIT_FD; + outputData.Stdio.data.fd = pipeFd[1]; + + if (this->TempOutputPipe.init(*this->Loop, 0) < 0) { + return false; + } + if (uv_pipe_open(this->TempOutputPipe, outputData.Stdio.data.fd) < 0) { + return false; + } + outputData.Streambuf.open(outputData.BuiltinStream); } break; @@ -263,16 +280,46 @@ bool cmUVProcessChain::InternalData::Prepare( break; } + bool first = true; + for (std::size_t i = 0; i < this->Builder->Processes.size(); i++) { + this->Processes.emplace_back(cm::make_unique()); + auto& process = *this->Processes.back(); + process.Data = this; + + if (!first) { + auto& prevProcess = *this->Processes[i - 1]; + + int pipeFd[2]; + if (cmGetPipes(pipeFd) < 0) { + return false; + } + + if (prevProcess.OutputPipe.init(*this->Loop, 0) < 0) { + return false; + } + if (uv_pipe_open(prevProcess.OutputPipe, pipeFd[1]) < 0) { + return false; + } + if (process.InputPipe.init(*this->Loop, 0) < 0) { + return false; + } + if (uv_pipe_open(process.InputPipe, pipeFd[0]) < 0) { + return false; + } + } + + first = false; + } + return true; } -bool cmUVProcessChain::InternalData::AddCommand( +bool cmUVProcessChain::InternalData::SpawnProcess( + std::size_t index, const cmUVProcessChainBuilder::ProcessConfiguration& config, bool first, bool last) { - this->Processes.emplace_back(cm::make_unique()); - auto& process = *this->Processes.back(); - process.Data = this; + auto& process = *this->Processes[index]; auto options = uv_process_options_t(); @@ -296,20 +343,14 @@ bool cmUVProcessChain::InternalData::AddCommand( if (first) { stdio[0].flags = UV_IGNORE; } else { - assert(this->Processes.size() >= 2); - auto& prev = *this->Processes[this->Processes.size() - 2]; stdio[0].flags = UV_INHERIT_STREAM; - stdio[0].data.stream = prev.OutputPipe; + stdio[0].data.stream = process.InputPipe; } if (last) { stdio[1] = this->OutputStreamData.Stdio; } else { - if (process.OutputPipe.init(*this->Loop, 0) < 0) { - return false; - } stdio[1] = uv_stdio_container_t(); - stdio[1].flags = - static_cast(UV_CREATE_PIPE | UV_WRITABLE_PIPE); + stdio[1].flags = UV_INHERIT_STREAM; stdio[1].data.stream = process.OutputPipe; } stdio[2] = this->ErrorStreamData.Stdio; @@ -325,11 +366,15 @@ bool cmUVProcessChain::InternalData::AddCommand( processData->Data->ProcessesCompleted++; }; - return process.Process.spawn(*this->Loop, options, &process) >= 0; + bool result = process.Process.spawn(*this->Loop, options, &process) >= 0; + process.InputPipe.reset(); + process.OutputPipe.reset(); + return result; } void cmUVProcessChain::InternalData::Finish() { + this->TempOutputPipe.reset(); this->TempErrorPipe.reset(); this->Valid = true; } -- cgit v0.12 From 891b60d691812c45b92d4ad2127e1559f80f8332 Mon Sep 17 00:00:00 2001 From: Kyle Edwards Date: Fri, 2 Jun 2023 16:02:23 -0400 Subject: cmUVProcessChain: Add Status::SpawnResult field --- ...lsLinuxELFObjdumpGetRuntimeDependenciesTool.cxx | 5 +- ...lsMacOSMachOOToolGetRuntimeDependenciesTool.cxx | 5 +- ...sWindowsPEDumpbinGetRuntimeDependenciesTool.cxx | 5 +- ...sWindowsPEObjdumpGetRuntimeDependenciesTool.cxx | 5 +- Source/cmLDConfigLDConfigTool.cxx | 5 +- Source/cmUVProcessChain.cxx | 61 +++-- Source/cmUVProcessChain.h | 5 +- Source/cmake.cxx | 2 +- Source/cmcmd.cxx | 10 +- Tests/CMakeLib/testUVProcessChain.cxx | 263 ++++++++++++++++----- Tests/CMakeLib/testUVProcessChainHelper.cxx | 4 +- 11 files changed, 250 insertions(+), 120 deletions(-) diff --git a/Source/cmBinUtilsLinuxELFObjdumpGetRuntimeDependenciesTool.cxx b/Source/cmBinUtilsLinuxELFObjdumpGetRuntimeDependenciesTool.cxx index 566e4a4..c5c41e0 100644 --- a/Source/cmBinUtilsLinuxELFObjdumpGetRuntimeDependenciesTool.cxx +++ b/Source/cmBinUtilsLinuxELFObjdumpGetRuntimeDependenciesTool.cxx @@ -35,7 +35,7 @@ bool cmBinUtilsLinuxELFObjdumpGetRuntimeDependenciesTool::GetFileInfo( builder.AddCommand(command); auto process = builder.Start(); - if (!process.Valid()) { + if (!process.Valid() || process.GetStatus(0).SpawnResult != 0) { std::ostringstream e; e << "Failed to start objdump process for:\n " << file; this->SetError(e.str()); @@ -73,8 +73,7 @@ bool cmBinUtilsLinuxELFObjdumpGetRuntimeDependenciesTool::GetFileInfo( this->SetError(e.str()); return false; } - auto status = process.GetStatus(); - if (!status[0] || status[0]->ExitStatus != 0) { + if (process.GetStatus(0).ExitStatus != 0) { std::ostringstream e; e << "Failed to run objdump on:\n " << file; this->SetError(e.str()); diff --git a/Source/cmBinUtilsMacOSMachOOToolGetRuntimeDependenciesTool.cxx b/Source/cmBinUtilsMacOSMachOOToolGetRuntimeDependenciesTool.cxx index 6d97720..cf4a8fe 100644 --- a/Source/cmBinUtilsMacOSMachOOToolGetRuntimeDependenciesTool.cxx +++ b/Source/cmBinUtilsMacOSMachOOToolGetRuntimeDependenciesTool.cxx @@ -34,7 +34,7 @@ bool cmBinUtilsMacOSMachOOToolGetRuntimeDependenciesTool::GetFileInfo( .AddCommand(command); auto process = builder.Start(); - if (!process.Valid()) { + if (!process.Valid() || process.GetStatus(0).SpawnResult != 0) { std::ostringstream e; e << "Failed to start otool process for:\n " << file; this->SetError(e.str()); @@ -88,8 +88,7 @@ bool cmBinUtilsMacOSMachOOToolGetRuntimeDependenciesTool::GetFileInfo( this->SetError(e.str()); return false; } - auto status = process.GetStatus(); - if (!status[0] || status[0]->ExitStatus != 0) { + if (process.GetStatus(0).ExitStatus != 0) { std::ostringstream e; e << "Failed to run otool on:\n " << file; this->SetError(e.str()); diff --git a/Source/cmBinUtilsWindowsPEDumpbinGetRuntimeDependenciesTool.cxx b/Source/cmBinUtilsWindowsPEDumpbinGetRuntimeDependenciesTool.cxx index f342884..3b02c2e 100644 --- a/Source/cmBinUtilsWindowsPEDumpbinGetRuntimeDependenciesTool.cxx +++ b/Source/cmBinUtilsWindowsPEDumpbinGetRuntimeDependenciesTool.cxx @@ -33,7 +33,7 @@ bool cmBinUtilsWindowsPEDumpbinGetRuntimeDependenciesTool::GetFileInfo( builder.AddCommand(command); auto process = builder.Start(); - if (!process.Valid()) { + if (!process.Valid() || process.GetStatus(0).SpawnResult != 0) { std::ostringstream e; e << "Failed to start dumpbin process for:\n " << file; this->SetError(e.str()); @@ -56,8 +56,7 @@ bool cmBinUtilsWindowsPEDumpbinGetRuntimeDependenciesTool::GetFileInfo( this->SetError(e.str()); return false; } - auto status = process.GetStatus(); - if (!status[0] || status[0]->ExitStatus != 0) { + if (process.GetStatus(0).ExitStatus != 0) { std::ostringstream e; e << "Failed to run dumpbin on:\n " << file; this->SetError(e.str()); diff --git a/Source/cmBinUtilsWindowsPEObjdumpGetRuntimeDependenciesTool.cxx b/Source/cmBinUtilsWindowsPEObjdumpGetRuntimeDependenciesTool.cxx index f14de55..2139b4b 100644 --- a/Source/cmBinUtilsWindowsPEObjdumpGetRuntimeDependenciesTool.cxx +++ b/Source/cmBinUtilsWindowsPEObjdumpGetRuntimeDependenciesTool.cxx @@ -34,7 +34,7 @@ bool cmBinUtilsWindowsPEObjdumpGetRuntimeDependenciesTool::GetFileInfo( builder.AddCommand(command); auto process = builder.Start(); - if (!process.Valid()) { + if (!process.Valid() || process.GetStatus(0).SpawnResult != 0) { std::ostringstream e; e << "Failed to start objdump process for:\n " << file; this->SetError(e.str()); @@ -57,8 +57,7 @@ bool cmBinUtilsWindowsPEObjdumpGetRuntimeDependenciesTool::GetFileInfo( this->SetError(e.str()); return false; } - auto status = process.GetStatus(); - if (!status[0] || status[0]->ExitStatus != 0) { + if (process.GetStatus(0).ExitStatus != 0) { std::ostringstream e; e << "Failed to run objdump on:\n " << file; this->SetError(e.str()); diff --git a/Source/cmLDConfigLDConfigTool.cxx b/Source/cmLDConfigLDConfigTool.cxx index 0752b33..7ce1a59 100644 --- a/Source/cmLDConfigLDConfigTool.cxx +++ b/Source/cmLDConfigLDConfigTool.cxx @@ -43,7 +43,7 @@ bool cmLDConfigLDConfigTool::GetLDConfigPaths(std::vector& paths) builder.SetBuiltinStream(cmUVProcessChainBuilder::Stream_OUTPUT) .AddCommand(ldConfigCommand); auto process = builder.Start(); - if (!process.Valid()) { + if (!process.Valid() || process.GetStatus(0).SpawnResult != 0) { this->Archive->SetError("Failed to start ldconfig process"); return false; } @@ -61,8 +61,7 @@ bool cmLDConfigLDConfigTool::GetLDConfigPaths(std::vector& paths) this->Archive->SetError("Failed to wait on ldconfig process"); return false; } - auto status = process.GetStatus(); - if (!status[0] || status[0]->ExitStatus != 0) { + if (process.GetStatus(0).ExitStatus != 0) { this->Archive->SetError("Failed to run ldconfig"); return false; } diff --git a/Source/cmUVProcessChain.cxx b/Source/cmUVProcessChain.cxx index 6517e60..a9e57e6 100644 --- a/Source/cmUVProcessChain.cxx +++ b/Source/cmUVProcessChain.cxx @@ -53,8 +53,9 @@ struct cmUVProcessChain::InternalData cm::uv_process_ptr Process; cm::uv_pipe_ptr InputPipe; cm::uv_pipe_ptr OutputPipe; - bool Finished = false; Status ProcessStatus; + + void Finish(); }; const cmUVProcessChainBuilder* Builder = nullptr; @@ -72,13 +73,11 @@ struct cmUVProcessChain::InternalData std::vector> Processes; bool Prepare(const cmUVProcessChainBuilder* builder); - bool SpawnProcess( + void SpawnProcess( std::size_t index, const cmUVProcessChainBuilder::ProcessConfiguration& config, bool first, bool last); void Finish(); - - static const Status* GetStatus(const ProcessData& data); }; cmUVProcessChainBuilder::cmUVProcessChainBuilder() @@ -171,10 +170,8 @@ cmUVProcessChain cmUVProcessChainBuilder::Start() const } for (std::size_t i = 0; i < this->Processes.size(); i++) { - if (!chain.Data->SpawnProcess(i, this->Processes[i], i == 0, - i == this->Processes.size() - 1)) { - return chain; - } + chain.Data->SpawnProcess(i, this->Processes[i], i == 0, + i == this->Processes.size() - 1); } chain.Data->Finish(); @@ -182,15 +179,6 @@ cmUVProcessChain cmUVProcessChainBuilder::Start() const return chain; } -const cmUVProcessChain::Status* cmUVProcessChain::InternalData::GetStatus( - const cmUVProcessChain::InternalData::ProcessData& data) -{ - if (data.Finished) { - return &data.ProcessStatus; - } - return nullptr; -} - bool cmUVProcessChain::InternalData::Prepare( const cmUVProcessChainBuilder* builder) { @@ -285,6 +273,7 @@ bool cmUVProcessChain::InternalData::Prepare( this->Processes.emplace_back(cm::make_unique()); auto& process = *this->Processes.back(); process.Data = this; + process.ProcessStatus.Finished = false; if (!first) { auto& prevProcess = *this->Processes[i - 1]; @@ -314,7 +303,7 @@ bool cmUVProcessChain::InternalData::Prepare( return true; } -bool cmUVProcessChain::InternalData::SpawnProcess( +void cmUVProcessChain::InternalData::SpawnProcess( std::size_t index, const cmUVProcessChainBuilder::ProcessConfiguration& config, bool first, bool last) @@ -360,16 +349,17 @@ bool cmUVProcessChain::InternalData::SpawnProcess( options.exit_cb = [](uv_process_t* handle, int64_t exitStatus, int termSignal) { auto* processData = static_cast(handle->data); - processData->Finished = true; processData->ProcessStatus.ExitStatus = exitStatus; processData->ProcessStatus.TermSignal = termSignal; - processData->Data->ProcessesCompleted++; + processData->Finish(); }; - bool result = process.Process.spawn(*this->Loop, options, &process) >= 0; + if ((process.ProcessStatus.SpawnResult = + process.Process.spawn(*this->Loop, options, &process)) < 0) { + process.Finish(); + } process.InputPipe.reset(); process.OutputPipe.reset(); - return result; } void cmUVProcessChain::InternalData::Finish() @@ -451,19 +441,15 @@ std::vector cmUVProcessChain::GetStatus() std::vector statuses( this->Data->Processes.size(), nullptr); for (std::size_t i = 0; i < statuses.size(); i++) { - statuses[i] = this->GetStatus(i); + statuses[i] = &this->GetStatus(i); } return statuses; } -const cmUVProcessChain::Status* cmUVProcessChain::GetStatus( +const cmUVProcessChain::Status& cmUVProcessChain::GetStatus( std::size_t index) const { - auto const& process = *this->Data->Processes[index]; - if (process.Finished) { - return &process.ProcessStatus; - } - return nullptr; + return this->Data->Processes[index]->ProcessStatus; } bool cmUVProcessChain::Finished() const @@ -474,8 +460,12 @@ bool cmUVProcessChain::Finished() const std::pair cmUVProcessChain::Status::GetException() const { + if (this->SpawnResult) { + return std::make_pair(ExceptionCode::Spawn, + uv_strerror(this->SpawnResult)); + } #ifdef _WIN32 - if ((this->ExitStatus & 0xF0000000) == 0xC0000000) { + if (this->Finished && (this->ExitStatus & 0xF0000000) == 0xC0000000) { // Child terminated due to exceptional behavior. switch (this->ExitStatus) { case STATUS_CONTROL_C_EXIT: @@ -550,9 +540,8 @@ cmUVProcessChain::Status::GetException() const } } } - return std::make_pair(ExceptionCode::None, ""); #else - if (this->TermSignal) { + if (this->Finished && this->TermSignal) { switch (this->TermSignal) { # ifdef SIGSEGV case SIGSEGV: @@ -709,6 +698,12 @@ cmUVProcessChain::Status::GetException() const } } } - return std::make_pair(ExceptionCode::None, ""); #endif + return std::make_pair(ExceptionCode::None, ""); +} + +void cmUVProcessChain::InternalData::ProcessData::Finish() +{ + this->ProcessStatus.Finished = true; + this->Data->ProcessesCompleted++; } diff --git a/Source/cmUVProcessChain.h b/Source/cmUVProcessChain.h index f92742f..547a944 100644 --- a/Source/cmUVProcessChain.h +++ b/Source/cmUVProcessChain.h @@ -74,11 +74,14 @@ public: Illegal, Interrupt, Numerical, + Spawn, Other, }; struct Status { + int SpawnResult; + bool Finished; int64_t ExitStatus; int TermSignal; @@ -102,7 +105,7 @@ public: bool Valid() const; bool Wait(int64_t milliseconds = -1); std::vector GetStatus() const; - const Status* GetStatus(std::size_t index) const; + const Status& GetStatus(std::size_t index) const; bool Finished() const; private: diff --git a/Source/cmake.cxx b/Source/cmake.cxx index f30d4d3..791569c 100644 --- a/Source/cmake.cxx +++ b/Source/cmake.cxx @@ -3916,7 +3916,7 @@ std::function cmake::BuildWorkflowStep( return [builder]() -> int { auto chain = builder.Start(); chain.Wait(); - return static_cast(chain.GetStatus().front()->ExitStatus); + return static_cast(chain.GetStatus(0).ExitStatus); }; } #endif diff --git a/Source/cmcmd.cxx b/Source/cmcmd.cxx index 9929e85..1bbd0ac 100644 --- a/Source/cmcmd.cxx +++ b/Source/cmcmd.cxx @@ -2008,7 +2008,7 @@ int cmcmd::RunPreprocessor(const std::vector& command, .SetBuiltinStream(cmUVProcessChainBuilder::Stream_ERROR) .AddCommand(command); auto process = builder.Start(); - if (!process.Valid()) { + if (!process.Valid() || process.GetStatus(0).SpawnResult != 0) { std::cerr << "Failed to start preprocessor."; return 1; } @@ -2016,8 +2016,7 @@ int cmcmd::RunPreprocessor(const std::vector& command, std::cerr << "Failed to wait for preprocessor"; return 1; } - auto status = process.GetStatus(); - if (!status[0] || status[0]->ExitStatus != 0) { + if (process.GetStatus(0).ExitStatus != 0) { auto* errorStream = process.ErrorStream(); if (errorStream) { std::cerr << errorStream->rdbuf(); @@ -2130,7 +2129,7 @@ int cmcmd::RunLLVMRC(std::vector const& args) .AddCommand(resource_compile); auto process = builder.Start(); result = 0; - if (!process.Valid()) { + if (!process.Valid() || process.GetStatus(0).SpawnResult != 0) { std::cerr << "Failed to start resource compiler."; result = 1; } else { @@ -2144,8 +2143,7 @@ int cmcmd::RunLLVMRC(std::vector const& args) if (result != 0) { return result; } - auto status = process.GetStatus(); - if (!status[0] || status[0]->ExitStatus != 0) { + if (process.GetStatus(0).ExitStatus != 0) { auto* errorStream = process.ErrorStream(); if (errorStream) { std::cerr << errorStream->rdbuf(); diff --git a/Tests/CMakeLib/testUVProcessChain.cxx b/Tests/CMakeLib/testUVProcessChain.cxx index 7027689..e0a67e9 100644 --- a/Tests/CMakeLib/testUVProcessChain.cxx +++ b/Tests/CMakeLib/testUVProcessChain.cxx @@ -20,7 +20,6 @@ struct ExpectedStatus { - bool Finished; bool MatchExitStatus; bool MatchTermSignal; cmUVProcessChain::Status Status; @@ -28,38 +27,6 @@ struct ExpectedStatus std::string ExceptionString; }; -static const std::vector status1 = { - { false, false, false, { 0, 0 }, cmUVProcessChain::ExceptionCode::None, "" }, - { false, false, false, { 0, 0 }, cmUVProcessChain::ExceptionCode::None, "" }, - { false, false, false, { 0, 0 }, cmUVProcessChain::ExceptionCode::None, "" }, -}; - -static const std::vector status2 = { - { true, true, true, { 0, 0 }, cmUVProcessChain::ExceptionCode::None, "" }, - { false, false, false, { 0, 0 }, cmUVProcessChain::ExceptionCode::None, "" }, - { false, false, false, { 0, 0 }, cmUVProcessChain::ExceptionCode::None, "" }, -}; - -static const std::vector status3 = { - { true, true, true, { 0, 0 }, cmUVProcessChain::ExceptionCode::None, "" }, - { true, true, true, { 1, 0 }, cmUVProcessChain::ExceptionCode::None, "" }, -#ifdef _WIN32 - { true, - true, - true, - { STATUS_ACCESS_VIOLATION, 0 }, - cmUVProcessChain::ExceptionCode::Fault, - "Access violation" }, -#else - { true, - false, - true, - { 0, SIGABRT }, - cmUVProcessChain::ExceptionCode::Other, - "Subprocess aborted" }, -#endif -}; - static const char* ExceptionCodeToString(cmUVProcessChain::ExceptionCode code) { switch (code) { @@ -73,6 +40,8 @@ static const char* ExceptionCodeToString(cmUVProcessChain::ExceptionCode code) return "Interrupt"; case cmUVProcessChain::ExceptionCode::Numerical: return "Numerical"; + case cmUVProcessChain::ExceptionCode::Spawn: + return "Spawn"; case cmUVProcessChain::ExceptionCode::Other: return "Other"; default: @@ -83,9 +52,10 @@ static const char* ExceptionCodeToString(cmUVProcessChain::ExceptionCode code) bool operator==(const cmUVProcessChain::Status* actual, const ExpectedStatus& expected) { - if (!expected.Finished) { - return !actual; - } else if (!actual) { + if (expected.Status.SpawnResult != actual->SpawnResult) { + return false; + } + if (expected.Status.Finished != actual->Finished) { return false; } if (expected.MatchExitStatus && @@ -96,7 +66,7 @@ bool operator==(const cmUVProcessChain::Status* actual, expected.Status.TermSignal != actual->TermSignal) { return false; } - if (expected.Finished && + if (expected.Status.Finished && std::make_pair(expected.ExceptionCode, expected.ExceptionString) != actual->GetException()) { return false; @@ -150,39 +120,96 @@ static void printResults( { std::cout << "Expected: " << std::endl; for (auto const& e : expected) { - if (e.Finished) { - std::cout << " ExitStatus: " - << printExpected(e.MatchExitStatus, e.Status.ExitStatus) - << ", TermSignal: " - << printExpected(e.MatchTermSignal, e.Status.TermSignal) - << ", ExceptionCode: " - << printExpected(e.Finished, - ExceptionCodeToString(e.ExceptionCode)) - << ", ExceptionString: \"" - << printExpected(e.Finished, e.ExceptionString) << '"' - << std::endl; - } else { - std::cout << " null" << std::endl; - } + std::cout << " SpawnResult: " << e.Status.SpawnResult + << ", Finished: " << e.Status.Finished << ", ExitStatus: " + << printExpected(e.MatchExitStatus, e.Status.ExitStatus) + << ", TermSignal: " + << printExpected(e.MatchTermSignal, e.Status.TermSignal) + << ", ExceptionCode: " + << printExpected(e.Status.Finished, + ExceptionCodeToString(e.ExceptionCode)) + << ", ExceptionString: \"" + << printExpected(e.Status.Finished, e.ExceptionString) << '"' + << std::endl; } std::cout << "Actual:" << std::endl; for (auto const& a : actual) { - if (a) { - auto exception = a->GetException(); - std::cout << " ExitStatus: " << a->ExitStatus - << ", TermSignal: " << a->TermSignal << ", ExceptionCode: " - << ExceptionCodeToString(exception.first) - << ", ExceptionString: \"" << exception.second << '"' - << std::endl; - } else { - std::cout << " null" << std::endl; - } + auto exception = a->GetException(); + std::cout << " SpawnResult: " << a->SpawnResult + << ", Finished: " << a->Finished + << ", ExitStatus: " << a->ExitStatus + << ", TermSignal: " << a->TermSignal + << ", ExceptionCode: " << ExceptionCodeToString(exception.first) + << ", ExceptionString: \"" << exception.second << '"' + << std::endl; } } static bool checkExecution(cmUVProcessChainBuilder& builder, std::unique_ptr& chain) { + static const std::vector status1 = { + { false, + false, + { 0, false, 0, 0 }, + cmUVProcessChain::ExceptionCode::None, + "" }, + { false, + false, + { 0, false, 0, 0 }, + cmUVProcessChain::ExceptionCode::None, + "" }, + { false, + false, + { 0, false, 0, 0 }, + cmUVProcessChain::ExceptionCode::None, + "" }, + }; + + static const std::vector status2 = { + { true, + true, + { 0, true, 0, 0 }, + cmUVProcessChain::ExceptionCode::None, + "" }, + { false, + false, + { 0, false, 0, 0 }, + cmUVProcessChain::ExceptionCode::None, + "" }, + { false, + false, + { 0, false, 0, 0 }, + cmUVProcessChain::ExceptionCode::None, + "" }, + }; + + static const std::vector status3 = { + { true, + true, + { 0, true, 0, 0 }, + cmUVProcessChain::ExceptionCode::None, + "" }, + { true, + true, + { 0, true, 1, 0 }, + cmUVProcessChain::ExceptionCode::None, + "" }, +#ifdef _WIN32 + { true, + true, + { 0, true, STATUS_ACCESS_VIOLATION, 0 }, + cmUVProcessChain::ExceptionCode::Fault, + "Access violation" }, +#else + { false, + true, + { 0, true, 0, SIGABRT }, + cmUVProcessChain::ExceptionCode::Other, + "Subprocess aborted" }, +#endif + }; + std::vector status; chain = cm::make_unique(builder.Start()); @@ -201,7 +228,7 @@ static bool checkExecution(cmUVProcessChainBuilder& builder, return false; } - if (chain->Wait(6000)) { + if (chain->Wait(9000)) { std::cout << "Wait() returned true, should be false" << std::endl; return false; } @@ -481,6 +508,113 @@ bool testUVProcessChainCwdChanged(const char* helperCommand) return true; } +bool testUVProcessChainSpawnFail(const char* helperCommand) +{ + static const std::vector status1 = { + { false, + false, + { 0, false, 0, 0 }, + cmUVProcessChain::ExceptionCode::None, + "" }, + { false, + false, + { UV_ENOENT, true, 0, 0 }, + cmUVProcessChain::ExceptionCode::Spawn, + uv_strerror(UV_ENOENT) }, +#ifdef _WIN32 + { true, + true, + { 0, true, STATUS_ACCESS_VIOLATION, 0 }, + cmUVProcessChain::ExceptionCode::Fault, + "Access violation" }, +#else + { false, + true, + { 0, true, 0, SIGABRT }, + cmUVProcessChain::ExceptionCode::Other, + "Subprocess aborted" }, +#endif + }; + + static const std::vector status2 = { +#ifdef _WIN32 + { true, + true, + { 0, true, 0, 0 }, + cmUVProcessChain::ExceptionCode::None, + "" }, +#else + { false, + true, + { 0, true, 0, SIGPIPE }, + cmUVProcessChain::ExceptionCode::Other, + "SIGPIPE" }, +#endif + { false, + false, + { UV_ENOENT, true, 0, 0 }, + cmUVProcessChain::ExceptionCode::Spawn, + uv_strerror(UV_ENOENT) }, +#ifdef _WIN32 + { true, + true, + { 0, true, STATUS_ACCESS_VIOLATION, 0 }, + cmUVProcessChain::ExceptionCode::Fault, + "Access violation" }, +#else + { false, + true, + { 0, true, 0, SIGABRT }, + cmUVProcessChain::ExceptionCode::Other, + "Subprocess aborted" }, +#endif + }; + + std::vector status; + + cmUVProcessChainBuilder builder; + builder.AddCommand({ helperCommand, "echo" }) + .AddCommand({ "this_command_is_for_cmake_and_should_never_exist" }) + .AddCommand({ helperCommand, "dedup" }) + .SetBuiltinStream(cmUVProcessChainBuilder::Stream_OUTPUT) + .SetBuiltinStream(cmUVProcessChainBuilder::Stream_ERROR); + + auto chain = builder.Start(); + if (!chain.Valid()) { + std::cout << "Valid() returned false, should be true" << std::endl; + return false; + } + + // Some platforms, like Solaris 10, take a long time to report a trapped + // subprocess to the parent process (about 1.7 seconds in the case of + // Solaris 10.) Wait 3 seconds to give it enough time. + if (chain.Wait(3000)) { + std::cout << "Wait() did not time out" << std::endl; + return false; + } + + status = chain.GetStatus(); + if (!resultsMatch(status, status1)) { + std::cout << "GetStatus() did not produce expected output" << std::endl; + printResults(status, status1); + return false; + } + + if (!chain.Wait()) { + std::cout << "Wait() timed out" << std::endl; + return false; + } + + status = chain.GetStatus(); + if (!resultsMatch(status, status2)) { + std::cout << "GetStatus() did not produce expected output" << std::endl; + printResults(status, status2); + return false; + } + + return true; +} + int testUVProcessChain(int argc, char** const argv) { if (argc < 2) { @@ -518,5 +652,10 @@ int testUVProcessChain(int argc, char** const argv) return -1; } + if (!testUVProcessChainSpawnFail(argv[1])) { + std::cout << "While executing testUVProcessChainSpawnFail().\n"; + return -1; + } + return 0; } diff --git a/Tests/CMakeLib/testUVProcessChainHelper.cxx b/Tests/CMakeLib/testUVProcessChainHelper.cxx index 99743e7..fcc45b0 100644 --- a/Tests/CMakeLib/testUVProcessChainHelper.cxx +++ b/Tests/CMakeLib/testUVProcessChainHelper.cxx @@ -32,13 +32,13 @@ int main(int argc, char** argv) std::string command = argv[1]; if (command == "echo") { - std::this_thread::sleep_for(std::chrono::milliseconds(3000)); + std::this_thread::sleep_for(std::chrono::milliseconds(6000)); std::cout << "HELLO world!" << std::flush; std::cerr << "1" << std::flush; return 0; } if (command == "capitalize") { - std::this_thread::sleep_for(std::chrono::milliseconds(9000)); + std::this_thread::sleep_for(std::chrono::milliseconds(12000)); std::string input = getStdin(); for (auto& c : input) { c = static_cast(std::toupper(c)); -- cgit v0.12