-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Resolved the issue of skipping Deps dependent downloads under CI for Macos #1773
Conversation
src/pika.cc
Outdated
@@ -32,7 +32,7 @@ extern std::unique_ptr<net::NetworkStatistic> g_network_statistic; | |||
static void version() { | |||
char version[32]; | |||
snprintf(version, sizeof(version), "%d.%d.%d", PIKA_MAJOR, PIKA_MINOR, PIKA_PATCH); | |||
std::cout << "-----------Pika server----------" << std::endl; | |||
std::cout << "-----------Pika servr----------" << std::endl; | |||
std::cout << "pika_version: " << version << std::endl; | |||
std::cout << pika_build_git_sha << std::endl; | |||
std::cout << "pika_build_compile_date: " << pika_build_compile_date << std::endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the provided code patch, there is a typo in the output message being printed. Instead of "Pika servr," it should be "Pika server." It seems to be a simple typo error.
Improvement suggestion:
- Instead of using a fixed-size buffer for the
version
variable, consider using a safer option likestd::string
orstd::ostringstream
to avoid potential buffer overflow issues. Here's an example usingstd::ostringstream
:
#include <sstream>
// ...
static void version() {
std::ostringstream oss;
oss << PIKA_MAJOR << '.' << PIKA_MINOR << '.' << PIKA_PATCH;
std::cout << "-----------Pika server----------" << std::endl;
std::cout << "pika_version: " << oss.str() << std::endl;
std::cout << pika_build_git_sha << std::endl;
std::cout << "pika_build_compile_date: " << pika_build_compile_date << std::endl;
}
This approach eliminates the need for explicitly managing buffer sizes and helps prevent potential bugs related to buffer overflows.
Apart from these suggestions, it's difficult to provide further insights without having access to the context and the rest of the codebase.
@@ -145,7 +145,6 @@ jobs: | |||
key: ${{ runner.os }}-dependencies | |||
|
|||
- name: install Deps | |||
if: ${{ steps.cache.outputs.cache-hit != 'true' }} | |||
run: | | |||
brew update | |||
brew install autoconf protobuf python llvm wget git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the entire context and additional details, it's challenging to provide a comprehensive code review. However, based on the provided code patch, here are a few observations:
- The
if
condition for theinstall Deps
step has been removed (if: ${{ steps.cache.outputs.cache-hit != 'true' }}
). It appears that this condition relied on the output of a previous step namedcache
. Without further information, it's uncertain why this condition was removed. Ensure this change aligns with the intended behavior.
Improvement suggestions:
-
It's generally a good practice to keep comments or documentation in the code. Adding a comment explaining the purpose of the
install Deps
step could be beneficial for future developers who may work on the codebase. -
Consider using a package manager, such as Homebrew, to manage the dependencies rather than installing them individually. This can simplify dependency management, especially in larger projects.
-
If possible, pinning specific versions of the dependencies can enhance stability and reproducibility. This ensures that everyone working on the project uses the same version of the dependencies.
-
Depending on your specific use case, you might want to consider providing a fallback plan in case the dependencies fail to install. For example, you can add error-handling logic or alternative installation methods.
Remember, for a complete code review, it's important to understand the entire codebase, its purpose, and any relevant documentation or requirements.
@@ -145,7 +145,6 @@ jobs: | |||
key: ${{ runner.os }}-dependencies | |||
|
|||
- name: install Deps | |||
if: ${{ steps.cache.outputs.cache-hit != 'true' }} | |||
run: | | |||
brew update | |||
brew install autoconf protobuf python llvm wget git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the code patch you provided, here are some observations and suggestions for improvement:
-
The
if
condition in the "install Deps" step has been commented out (- if: ${{ steps.cache.outputs.cache-hit != 'true' }}
). It seems that this condition was intended to check whether the cache was hit during a previous step. If the condition is necessary for correct execution, you should uncomment it. -
Consider adding a comment explaining the purpose of the "install Deps" step, especially if it's not immediately obvious from the context.
-
Check if the dependencies listed in the
brew install
command (autoconf
,protobuf
,python
,llvm
,wget
, andgit
) are necessary for your project. Ensure that these dependencies are compatible with your target environment and version requirements. -
It's generally a good practice to specify specific versions or version ranges for the dependencies being installed. This helps ensure reproducible builds and avoids unexpected compatibility issues. For example, you could use
brew install autoconf@<version> protobuf@<version>
to install specific versions ofautoconf
andprotobuf
. -
Consider using a package manager specific to your programming language (e.g.,
pip
for Python) to manage language-specific dependencies instead of installing them through Homebrew. This approach allows for better dependency management within the context of your project. -
Make sure to have error handling and validation mechanisms in place to handle potential failures during the installation process. You may want to consider adding
set -e
at the beginning of the script to make it exit immediately if any command returns a non-zero status.
Remember, these suggestions are based solely on the provided code patch. A comprehensive review would require examining the entire codebase, understanding the project's requirements, and considering best practices specific to your coding standards and development guidelines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
… for Macos (OpenAtomFoundation#1773) * just test * fix Macos CI
… for Macos (OpenAtomFoundation#1773) * just test * fix Macos CI
fix: Resolved the issue of skipping Deps dependent downloads under CI for Macos