MINIFICPP-2606 Improve logging for python virtualenv initialization #2008
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR improves logging for Python virtual environment initialization by enhancing error handling and output capture. The changes focus on better visibility into the Python dependency installation process when issues occur.
- Replaced
std::systemwith a customexecuteProcessfunction to capture command output - Added error handling and detailed logging for virtual environment creation and package installation
- Enhanced Python dependency installer with progress bar suppression and better error reporting
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| PythonDependencyInstaller.cpp | Added executeProcess function for better command execution with output capture and enhanced error logging for virtualenv operations |
| dependency_installer.py | Fixed deprecated AST attribute access, added progress bar suppression, and improved error handling for pip installation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
lordgamez
force-pushed
the
MINIFICPP-2606
branch
from
68e4c5b to
f52f7f6
Compare
| #define pclose _pclose | ||
| #endif | ||
|
|
||
| std::pair<int, std::string> executeProcess(const std::string& command) { |
There was a problem hiding this comment.
For the return type, I'd use an aggregate struct instead.
There was a problem hiding this comment.
Updated in e63c7c0
| std::string result; | ||
| while (fgets(buffer.data(), gsl::narrow<int>(buffer.size()), pipe) != nullptr) { | ||
| result += buffer.data(); | ||
| } |
There was a problem hiding this comment.
An ostringstream would fit this problem better.
There was a problem hiding this comment.
Updated in e63c7c0
| #endif | ||
|
|
||
| std::pair<int, std::string> executeProcess(const std::string& command) { | ||
| std::array<char, 256> buffer{}; |
There was a problem hiding this comment.
At the very least I'd increase the buffer size, and use the common buffer size constant.
| std::array<char, 256> buffer{}; | |
| std::array<char, utils::configuration::DEFAULT_BUFFER_SIZE> buffer{}; |
There was a problem hiding this comment.
Updated in e63c7c0
lordgamez
force-pushed
the
MINIFICPP-2606
branch
from
f52f7f6 to
e63c7c0
Compare
https://issues.apache.org/jira/browse/MINIFICPP-2606
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically main)?
Is your initial contribution a single, squashed commit?
For code changes:
For documentation related changes:
Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.