Skip to content
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

Merged
merged 1 commit into from
Jul 20, 2023
Merged

feat: Support Macos on CI #1769

merged 1 commit into from
Jul 20, 2023

Conversation

Mixficsol
Copy link
Collaborator

@Mixficsol Mixficsol commented Jul 20, 2023

Supports Macos on CI
feat: #1758

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
Copy link

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:

  1. 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
    
  2. 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
    
  3. 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.

  4. 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.

  5. Add a newline at the end of the file.

@Mixficsol Mixficsol changed the title success push feat: Support Macos on CI Jul 20, 2023
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
Copy link

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:

  1. 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.

  2. 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 the python3 command. This ensures consistency across different environments.

  3. 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.

  4. 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.

  5. 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.

  6. 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.

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
Copy link

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:

  1. 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.

  2. 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.

  3. 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.

  4. 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.

  5. Script Execution Permissions: You set the execution permission for the script start_master_and_slave.sh using chmod +x. Make sure this is necessary and the script is not already executable by default.

  6. 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.

working-directory: ${{github.workspace}}/build
run: |
python3 ../tests/integration/pika_replication_test.py
python3 ../tests/unit/Blpop_Brpop_test.py
Copy link

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:

  1. It looks like the code patch adds a new job called build_on_macos that runs on macOS.
  2. The job starts by checking out the repository using actions/checkout@v3.
  3. The dependencies are cached using the actions/cache@v2 action. It caches the INSTALL_LOCATION directory and the ~/.cache/pip directory.
  4. Dependencies are installed using Homebrew (brew) and Python packages are installed using pip.
  5. The environment variables CXX and CC are exported to use specific versions of GCC.
  6. The build process uses CMake to configure and build the project.
  7. There are commented out sections for testing and running Python tests. Uncomment them if needed.
  8. There is a step to start Pika master and slave processes.
  9. 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.

working-directory: ${{github.workspace}}/build
run: |
python3 ../tests/integration/pika_replication_test.py
python3 ../tests/unit/Blpop_Brpop_test.py
Copy link

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:

  1. 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.

  2. 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.

  3. Explicit Compiler Configuration: The code sets CXX and CC 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.

  4. 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.

  5. 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.

  6. 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.

@Mixficsol
Copy link
Collaborator Author

Mixficsol commented Jul 20, 2023

@machinly 目前在Macos环境的Python E2E测试是有问题的,我打算现在先放这些测试,后续我想把python的测试执行写在pikatests.sh里面,这样就不用每次往下面加了

@Mixficsol Mixficsol requested review from machinly and AlexStocks July 20, 2023 05:01
@machinly machinly requested review from luky116 and chenbt-hz July 20, 2023 05:27
@AlexStocks AlexStocks merged commit 45c3f78 into OpenAtomFoundation:unstable Jul 20, 2023
@Mixficsol Mixficsol deleted the success_Macos branch July 20, 2023 05:56
bigdaronlee163 pushed a commit to bigdaronlee163/pika that referenced this pull request Jun 8, 2024
cheniujh pushed a commit to cheniujh/pika that referenced this pull request Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants