Copilot commented on code in PR #2008:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2008#discussion_r2273801735
##########
extensions/python/PythonDependencyInstaller.cpp:
##########
@@ -93,25 +128,30 @@ void
PythonDependencyInstaller::createVirtualEnvIfSpecified() const {
}
if (!std::filesystem::exists(virtualenv_path_) ||
std::filesystem::is_empty(virtualenv_path_)) {
logger_->log_info("Creating python virtual env at: {}",
virtualenv_path_.string());
- auto venv_command = "\"" + python_binary_ + "\" -m venv \"" +
virtualenv_path_.string() + "\"";
- auto return_value =
std::system(encapsulateCommandInQuotesIfNeeded(venv_command).c_str());
- if (return_value != 0) {
- throw PythonScriptException(fmt::format("The following command creating
python virtual env failed: '{}'", venv_command));
+ auto venv_command = "\"" + python_binary_ + "\" -m venv \"" +
virtualenv_path_.string() + "\" 2>&1";
+ auto result = executeProcess(venv_command);
+ if (result.first != 0) {
+ logger_->log_error("The following command creating python virtual env
failed: '{}'\nSetup process output:\n{}", venv_command, result.second);
+ throw PythonScriptException(fmt::format("The following command creating
python virtual env failed: '{}'\nSetup process output:\n{}", venv_command,
result.second));
}
}
}
void PythonDependencyInstaller::runInstallCommandInVirtualenv(const
std::string& install_command) const {
std::string command_with_virtualenv;
#if WIN32
- command_with_virtualenv.append("\"").append((virtualenv_path_ / "Scripts" /
"activate.bat").string()).append("\" && ");
+ command_with_virtualenv.append("\"").append((virtualenv_path_ / "Scripts" /
"activate.bat").string()).append("\" 2>&1 && ");
#else
- command_with_virtualenv.append(". \"").append((virtualenv_path_ / "bin" /
"activate").string()).append("\" && ");
+ command_with_virtualenv.append(". \"").append((virtualenv_path_ / "bin" /
"activate").string()).append("\" 2>&1 && ");
#endif
command_with_virtualenv.append(install_command);
- auto return_value =
std::system(encapsulateCommandInQuotesIfNeeded(command_with_virtualenv).c_str());
- if (return_value != 0) {
- throw PythonScriptException(fmt::format("The following command to install
python packages failed: '{}'", command_with_virtualenv));
+
+ auto result = executeProcess(command_with_virtualenv + " 2>&1");
Review Comment:
The stderr redirection '2>&1' is being added twice - once at line 143/145
when building the command and again here. This duplication could lead to
unexpected behavior. Remove the '2>&1' from this line since it's already
included in the command construction.
```suggestion
auto result = executeProcess(command_with_virtualenv);
```
##########
extensions/python/PythonDependencyInstaller.cpp:
##########
@@ -93,25 +128,30 @@ void
PythonDependencyInstaller::createVirtualEnvIfSpecified() const {
}
if (!std::filesystem::exists(virtualenv_path_) ||
std::filesystem::is_empty(virtualenv_path_)) {
logger_->log_info("Creating python virtual env at: {}",
virtualenv_path_.string());
- auto venv_command = "\"" + python_binary_ + "\" -m venv \"" +
virtualenv_path_.string() + "\"";
- auto return_value =
std::system(encapsulateCommandInQuotesIfNeeded(venv_command).c_str());
- if (return_value != 0) {
- throw PythonScriptException(fmt::format("The following command creating
python virtual env failed: '{}'", venv_command));
+ auto venv_command = "\"" + python_binary_ + "\" -m venv \"" +
virtualenv_path_.string() + "\" 2>&1";
Review Comment:
The stderr redirection '2>&1' is added here but the executeProcess function
is designed to capture both stdout and stderr from the pipe. Adding '2>&1' may
be redundant since popen with 'r' mode typically captures stdout, and this
redirection might not work as expected across all platforms.
```suggestion
auto venv_command = "\"" + python_binary_ + "\" -m venv \"" +
virtualenv_path_.string() + "\"";
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
