-
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
feat: Support Macos on CI #1769
feat: Support Macos on CI #1769
Conversation
.github/workflows/pika.yml
Outdated
working-directory: ${{github.workspace}}/build | ||
run: | | ||
python3 ../tests/integration/pika_replication_test.py | ||
python3 ../tests/unit/Blpop_Brpop_test.py No newline at end of file |
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.
Here are some observations and suggestions for the code patch:
-
In the step "cache dependencies," the
path
field should be defined in a YAML block style instead of using the pipe symbol. It should look like this:path: - ${{ github.workspace }}/${{ env.INSTALL_LOCATION }} - ~/.cache/pip
-
The commands for installing dependencies in the "install Deps" step could be combined into a single line, separating them with && to ensure they are executed sequentially. For example:
brew update && brew install autoconf protobuf python llvm wget git gcc@10 automake cmake make binutils && python3 -m pip install --upgrade pip && python3 -m pip install redis
-
There is a duplicate export statement for CC in the "Build" step. You can remove the export statement before the cmake command since it's already set before that.
-
The commented-out section for Test, Unit Test, Start pika master and slave, and Run Python E2E Tests suggests that these steps might need further attention. Ensure they are properly validated and uncommented before merging the code.
-
Add a newline at the end of the file.
.github/workflows/pika.yml
Outdated
working-directory: ${{github.workspace}}/build | ||
run: | | ||
python3 ../tests/integration/pika_replication_test.py | ||
python3 ../tests/unit/Blpop_Brpop_test.py No newline at end of file |
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.
Overall, the code patch looks good, but here are a few suggestions for improvement and potential bug risks:
-
Cache Dependencies: It's a good idea to cache dependencies to speed up subsequent builds. However, make sure you're caching the correct paths. Double-check that
${{ env.INSTALL_LOCATION }}
and~/.cache/pip
are the correct paths for your project's dependencies. -
Installing Dependencies: In the "install Deps" step, it's recommended to use the full path for Python 3 (
/usr/local/opt/python
) instead of relying on thepython3
command. This ensures consistency across different environments. -
Building: Be cautious when setting environment variables like
CC
multiple times within the same job. Make sure the changes don't conflict with each other or affect other parts of the workflow. -
Unit Test: Uncommented code suggests that the unit test step is currently disabled. Ensure that the tests have been properly implemented and tested before enabling this step in your workflow.
-
Start pika master and slave: Check if there are any dependencies or prerequisites required for running the script
../tests/integration/start_master_and_slave.sh
. Make sure the necessary permissions and dependencies are properly set up. -
Run Python E2E Tests: Check for any missing dependencies or prerequisites required to run the Python scripts
../tests/integration/pika_replication_test.py
and../tests/unit/Blpop_Brpop_test.py
. Ensure you have the correct versions of Python and any libraries required by these scripts.
Remember to thoroughly test your workflow and make adjustments specific to your project's needs. This review provides general suggestions based on the code provided, but it's important to consider your project's context and requirements.
.github/workflows/pika.yml
Outdated
working-directory: ${{github.workspace}}/build | ||
run: | | ||
python3 ../tests/integration/pika_replication_test.py | ||
python3 ../tests/unit/Blpop_Brpop_test.py No newline at end of file |
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.
Here are some observations and suggestions for your code patch:
-
Cache Dependencies: Caching dependencies is a good practice to speed up subsequent workflow runs. However, it would be best if you used different cache keys for different sets of dependencies to prevent potential conflicts.
-
Environment Setup: It seems that you are setting up the build environment on macOS. Make sure you have considered the specific requirements and versions of the dependencies needed for your project.
-
Commented Code: There are several commented sections in your code. If they are not necessary, it's recommended to remove them to keep the code clean and easier to maintain. If they are intended for future use, consider leaving a clear comment explaining their purpose.
-
Unit Test and Integration Test Execution: It appears that you have separate steps for executing unit tests, starting the Pika master and slave, and running Python E2E tests. Ensure that these steps are working as intended and also provide appropriate error handling and reporting.
-
Script Execution Permissions: You set the execution permission for the script
start_master_and_slave.sh
usingchmod +x
. Make sure this is necessary and the script is not already executable by default. -
File Encoding: The diff doesn't show a newline at the end of the file. While this may not introduce any issues, it's generally good practice to have a newline character at the end of each file.
Overall, it seems like a reasonable code patch, but it would be advisable to thoroughly test and validate the changes to identify any bugs or improvements.
.github/workflows/pika.yml
Outdated
working-directory: ${{github.workspace}}/build | ||
run: | | ||
python3 ../tests/integration/pika_replication_test.py | ||
python3 ../tests/unit/Blpop_Brpop_test.py |
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.
Here are some observations and suggestions for the code patch:
- It looks like the code patch adds a new job called
build_on_macos
that runs on macOS. - The job starts by checking out the repository using
actions/checkout@v3
. - The dependencies are cached using the
actions/cache@v2
action. It caches theINSTALL_LOCATION
directory and the~/.cache/pip
directory. - Dependencies are installed using Homebrew (
brew
) and Python packages are installed usingpip
. - The environment variables
CXX
andCC
are exported to use specific versions of GCC. - The build process uses CMake to configure and build the project.
- There are commented out sections for testing and running Python tests. Uncomment them if needed.
- There is a step to start Pika master and slave processes.
- Finally, there is a step to run Python end-to-end tests.
Possible improvements:
- Add error handling and failure conditions to the steps to handle any unexpected issues during the process.
- Consider using a separate virtual environment for Python dependencies instead of installing them globally.
- Use specific versions or version ranges for Python package installations to ensure reproducibility.
- Ensure that the necessary cleanup/reset steps are performed, especially if this workflow is meant to be run multiple times.
- Perform a thorough test of the code in different scenarios to identify any possible bugs or issues.
.github/workflows/pika.yml
Outdated
working-directory: ${{github.workspace}}/build | ||
run: | | ||
python3 ../tests/integration/pika_replication_test.py | ||
python3 ../tests/unit/Blpop_Brpop_test.py |
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.
The code appears to be a GitHub Actions workflow that builds and tests software. Here are some observations and suggestions for improvement:
-
Cache Usage: The cache step is used to cache dependencies, which can speed up subsequent workflow runs. Make sure the cache key is unique and meaningful to avoid using outdated or irrelevant dependencies.
-
Conditional Installation: The "install Deps" step checks if the cache was hit (
steps.cache.outputs.cache-hit != 'true'
) before installing dependencies. This helps avoid unnecessary installations when the cache is already present. -
Explicit Compiler Configuration: The code sets
CXX
andCC
environment variables to specific paths for the GCC 10 compiler. Consider moving these configuration settings to a separate section (e.g.,env
) at the beginning of your workflow for better organization and ease of maintenance. -
Commented-out Steps: There are commented-out steps for testing purposes. Make sure to remove or uncomment those steps based on your requirements. It's good practice to keep the workflow clean and only include necessary steps.
-
Working Directories: Pay attention to the working directories specified for each step. It seems that the "Unit Test," "Start pika master and slave," and "Run Python E2E Tests" steps use different working directories (
${{github.workspace}}
,${{github.workspace}}/build
). Ensure that you are in the correct directory to execute the desired commands successfully. -
Error Handling: Consider adding error handling or failure handling steps in case any of the previous steps fail. This could involve notifying the team or running cleanup tasks to prevent issues in subsequent workflow runs.
Remember to thoroughly test your workflow and adjust it to fit your specific project's requirements.
@machinly 目前在Macos环境的Python E2E测试是有问题的,我打算现在先放这些测试,后续我想把python的测试执行写在pikatests.sh里面,这样就不用每次往下面加了 |
Supports Macos on CI
feat: #1758