fgerlits commented on code in PR #2008:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2008#discussion_r2290575344
##########
extensions/python/PythonDependencyInstaller.cpp:
##########
@@ -93,25 +134,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.exit_code != 0) {
+ logger_->log_error("The following command creating python virtual env
failed: '{}'\nSetup process output:\n{}", venv_command, result.output);
+ throw PythonScriptException(fmt::format("The following command creating
python virtual env failed: '{}'\nSetup process output:\n{}", venv_command,
result.output));
}
}
}
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");
+ if (result.exit_code != 0) {
+ logger_->log_error("Failed to install python packages to virtualenv.
Install process output:\n{}", result.output);
+ throw PythonScriptException(fmt::format("Failed to install python packages
to virtualenv. Install process output:\n{}", result.output));
Review Comment:
we should log (and include in the exception text) `command_with_virtualenv`
here, too
##########
extensions/python/PythonDependencyInstaller.cpp:
##########
@@ -93,25 +134,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.exit_code != 0) {
+ logger_->log_error("The following command creating python virtual env
failed: '{}'\nSetup process output:\n{}", venv_command, result.output);
+ throw PythonScriptException(fmt::format("The following command creating
python virtual env failed: '{}'\nSetup process output:\n{}", venv_command,
result.output));
}
}
}
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:
Nitpicking, but I would append the `" 2>&1"` to `command_with_virtualenv` in
line 153, so when we log the command, we are logging the exact command we
executed.
--
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]
