chore: switch docker base image to ubuntu noble, use system rocksdb for faster compilation #555
Conversation
…or faster compilation
WalkthroughThe changes in this pull request involve updates to multiple configuration files and workflows for the project. A new environment section is added to the Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant CI/CD
participant Docker
participant Application
Developer->>CI/CD: Push changes
CI/CD->>Docker: Build image with new base
Docker->>Application: Install dependencies
Application-->>Docker: Dependencies installed
CI/CD-->>Developer: Build complete
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
tedil
changed the title
|
Can we also use this for test/lint? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
.github/workflows/release-please.yml (1)
40-43: LGTM! Consider adding a comment for clarity.The addition of the "Install dependencies" step is a good improvement. It ensures that
librocksdb-devis available for the build process, which aligns with the PR objective of facilitating faster compilation.Consider adding a brief comment explaining why
librocksdb-devis needed. This can help future maintainers understand the purpose of this step. For example:- name: Install dependencies run: | # Install librocksdb-dev for building the project sudo apt-get update -y sudo apt-get install -y librocksdb-devREADME.md (2)
115-123: LGTM: Clear instructions for RocksDB installationThe instructions for installing pre-built RocksDB are clear and provide good options for both Ubuntu and Conda users. This aligns well with the PR objective of facilitating faster compilation.
Consider adding a brief explanation of why using a pre-built RocksDB version is recommended (e.g., "This significantly reduces compile times by avoiding the need to build RocksDB from source.").
124-139: LGTM: Comprehensive instructions for library path configurationThe instructions for setting library paths are clear and provide good flexibility with both
.cargo/config.tomland environment variable options. This aligns well with the PR objective of improving the build process.Consider adding a note about the potential need to adjust paths when using Conda, as the current example paths are for the system package manager. This could prevent confusion for Conda users.
utils/docker/Dockerfile (1)
78-78: Simplify the ENTRYPOINT commandIf your entrypoint script (
entrypoint.sh) has the appropriate shebang (e.g.,#!/bin/bash) at the top and has execute permissions, you can simplify theENTRYPOINTdirective by removing the explicit call to/bin/bash.Apply this diff to simplify the
ENTRYPOINT:-ENTRYPOINT ["/bin/bash", "/entrypoint.sh"] +ENTRYPOINT ["/entrypoint.sh"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- .cargo/config.toml (1 hunks)
- .github/workflows/release-please.yml (1 hunks)
- README.md (1 hunks)
- utils/docker/Dockerfile (2 hunks)
🧰 Additional context used
🔇 Additional comments (6)
.cargo/config.toml (1)
5-7: LGTM! Consider verifying library installations in the Docker image.The addition of the
[env]section withROCKSDB_LIB_DIRandSNAPPY_LIB_DIRenvironment variables is appropriate for the switch to the "ubuntu noble" base image. This change allows the project to use system-installed libraries, which should facilitate faster compilation as mentioned in the PR objectives.To ensure that these libraries are correctly installed and accessible in the new Docker image, consider running the following verification script:
This script will help confirm that the libraries are present in the expected location within the Docker image.
.github/workflows/release-please.yml (1)
40-43: Verify the impact on workflow execution time.The addition of the "Install dependencies" step is well-integrated into the existing workflow. It's conditionally executed only when a release is created, which is an efficient approach. However, it's worth verifying that this addition doesn't significantly impact the overall workflow execution time.
To check the impact on execution time, you can run the following script:
This script will help you assess if there's any noticeable increase in execution time due to the new step.
✅ Verification successful
Workflow execution time impact verified.
The "Install dependencies" step adds approximately 10-11 minutes to the workflow execution time when it runs. Since this step is conditionally executed only when a release is created, it does not affect the majority of workflow runs. The integration is efficient and maintains the overall workflow performance.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Compare workflow run times before and after this change # Get the workflow run times for the last 10 runs gh run list --workflow=.github/workflows/release-please.yml --limit=10 --json durationMs,createdAt,conclusion,headSha # Note: Analyze the output to compare run times before and after the change. # Look for the run with the commit SHA that introduced this change and compare # the durations of runs before and after it.Length of output: 355
Script:
#!/bin/bash # Description: Calculate workflow run durations and compare before and after the change # Fetch the last 10 workflow runs gh run list --workflow=.github/workflows/release-please.yml --limit=10 --json startedAt,updatedAt,headSha > runs.json # Check if jq is installed if ! command -v jq &> /dev/null then echo "jq could not be found. Please install jq to run this script." exit fi # Calculate duration in milliseconds for each run jq '.[] | {headSha, durationMs: ( ( ( .updatedAt | fromdateiso8601 ) - ( .startedAt | fromdateiso8601 ) ) * 1000 )}' runs.json > run_durations.json # Output the durations cat run_durations.jsonLength of output: 1109
README.md (3)
113-114: LGTM: New section added for building instructionsThe addition of a "Building from scratch" section aligns well with the PR objectives of facilitating faster compilation processes. This will provide valuable guidance to developers.
140-148: LGTM: Clear build and installation instructionsThe instructions for building and installing the project are clear, concise, and properly formatted. They provide a logical conclusion to the "Building from scratch" section and align well with the PR objectives of improving the build and installation process.
113-148: Overall: Excellent addition to the READMEThe new "Building from scratch" section is a valuable addition to the README, providing clear and comprehensive instructions for setting up the project with pre-built RocksDB. This aligns perfectly with the PR objectives of facilitating faster compilation processes.
The instructions are well-structured, offering flexibility in installation methods and configuration options. The code blocks and explanations are clear and easy to follow.
These changes will significantly improve the developer experience, especially for those new to the project or working on different environments.
utils/docker/Dockerfile (1)
19-19: Verify the availability of 'librocksdb-dev' in Ubuntu repositoriesEnsure that the package
librocksdb-devis available in the Ubuntu repositories for the specified Ubuntu version. If it's not available, you may need to add a PPA, build RocksDB from source, or use an alternative method to include it.Run the following script to check if
librocksdb-devis available:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
.github/workflows/conventional-prs.yml (1)
12-12: Approved: Good choice to specify Ubuntu 24.04The change from
ubuntu-latesttoubuntu-24.04is a good move. It ensures consistency in the CI environment and aligns with the PR's objective of switching to Ubuntu Noble. This specificity can help prevent unexpected issues that might arise from using different Ubuntu versions.Consider adding a comment explaining the reason for choosing this specific version. This can help future maintainers understand the decision and make informed choices when updating. For example:
runs-on: ubuntu-24.04 # Using Ubuntu Noble for consistency with our Docker base image.github/workflows/automerge.yml (2)
10-10: Approved: Specific Ubuntu version aligns with PR objectivesThe change from
ubuntu-latesttoubuntu-24.04aligns with the PR objective of switching to "ubuntu noble" and provides more control over the environment version. This can lead to improved consistency and stability in the CI/CD pipeline.However, keep in mind that using a specific version might require manual updates in the future when a new LTS version is released. Consider setting up a reminder or automated process to review and update this version periodically.
Line range hint
1-20: Suggestion: Consider adding safety checks before auto-mergingWhile the current workflow is functional, it could benefit from additional safety measures before auto-merging Dependabot PRs. Consider adding a step to check for successful status checks or other relevant conditions before proceeding with the merge.
Here's an example of how you could modify the workflow to include this:
jobs: dependabot: runs-on: ubuntu-24.04 if: ${{ github.actor == 'dependabot[bot]' }} steps: - name: Check if PR can be merged run: | gh pr view "$PR_URL" --json mergeStateStatus -q .mergeStateStatus | grep -q CLEAN env: PR_URL: ${{github.event.pull_request.html_url}} GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - name: Enable auto-merge for Dependabot PRs if: success() run: gh pr merge --auto --squash "$PR_URL" env: PR_URL: ${{github.event.pull_request.html_url}} GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}This modification adds a step to check if the PR can be merged before proceeding with the auto-merge, providing an extra layer of safety.
.github/workflows/docker-build.yml (1)
Line range hint
1-62: Consider expanding the workflow to include testing and lintingThe current workflow focuses on building and pushing the Docker image. To address the comment from user holtgrewe and improve the overall quality assurance process, consider adding steps for testing and linting the Docker image within this workflow.
Here's a suggestion for additional steps you could add after the "Build and push Docker image" step:
- name: Test Docker image run: | docker run --rm ${{ steps.meta.outputs.tags }} [your-test-command-here] - name: Lint Docker image run: | docker run --rm -v /var/run/docker.sock:/var/run/docker.sock hadolint/hadolint hadolint ${{ steps.meta.outputs.tags }}Replace
[your-test-command-here]with an appropriate command to test your Docker image. The linting step uses Hadolint, a popular Dockerfile linter. Adjust as needed based on your specific requirements and tooling preferences..github/workflows/rust.yml (2)
35-35: LGTM: Runner OS update and dependency installationThe change to
ubuntu-24.04is consistent with the PR objective. The new step for installing dependencies is crucial for the project's build process.Consider moving the dependency installation to a separate shell script (e.g.,
install_dependencies.sh) to improve maintainability and reusability across different workflows. This would look like:- name: Install dependencies run: ./.github/scripts/install_dependencies.shAlso applies to: 40-43
70-70: LGTM: Runner OS update and dependency installationThe change to
ubuntu-24.04is consistent with the other jobs. The dependency installation step is correctly added to ensure all required packages are available for testing.To reduce duplication, consider creating a reusable workflow for the dependency installation step. This can be achieved by creating a new workflow file (e.g.,
.github/workflows/install_dependencies.yml) and using it in all jobs:# .github/workflows/install_dependencies.yml name: Install Dependencies on: workflow_call: jobs: install: runs-on: ubuntu-24.04 steps: - name: Install dependencies run: | sudo apt-get update -y sudo apt-get install -y librocksdb-dev libsnappy-dev libsqlite3-devThen, in this workflow, you can use:
- uses: ./.github/workflows/install_dependencies.ymlThis approach would centralize the dependency installation process, making it easier to maintain and update across all jobs.
Also applies to: 77-80
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- .github/workflows/automerge.yml (1 hunks)
- .github/workflows/conventional-prs.yml (1 hunks)
- .github/workflows/docker-build.yml (2 hunks)
- .github/workflows/release-please.yml (3 hunks)
- .github/workflows/rust.yml (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/release-please.yml
🧰 Additional context used
🔇 Additional comments (4)
.github/workflows/conventional-prs.yml (1)
Line range hint
2-7: Security consideration: Review usage ofpull_request_targetThe workflow is currently triggered on
pull_request_target, which runs with repository secrets even for PRs from forks. While this is necessary for some actions that need write permissions, it can pose a security risk if not handled carefully.Please ensure that this workflow doesn't checkout or run any code from the PR. If it does, consider using
pull_requestinstead, or implement proper security checks before running any external code.To verify the current usage, you can run:
If these commands return any results, review those steps carefully to ensure they don't introduce security risks.
✅ Verification successful
Security Review: Usage of
pull_request_targetVerifiedThe workflow does not checkout code or run any scripts from the PR, mitigating potential security risks associated with
pull_request_target.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the workflow checks out PR code or runs any scripts rg --type yaml 'uses: actions/checkout@' .github/workflows/conventional-prs.yml rg --type yaml 'run:' .github/workflows/conventional-prs.ymlLength of output: 142
.github/workflows/docker-build.yml (1)
21-21: LGTM! Consider applying this change to other workflow files.The update to
ubuntu-24.04aligns well with the PR objective of switching to "ubuntu noble". This change can provide more stability and reproducibility in the CI/CD pipeline.To ensure consistency across the project, let's verify if other workflow files need a similar update:
If the script returns results, consider updating those files as well for consistency.
.github/workflows/rust.yml (2)
13-13: LGTM: Runner OS update and formatting adjustmentThe change from
ubuntu-latesttoubuntu-24.04aligns with the PR objective of switching to Ubuntu Noble. The minor formatting adjustment in the "Check format" step doesn't affect functionality.Also applies to: 31-31
Line range hint
1-114: Summary: Workflow updates align with PR objectivesThe changes to this workflow file successfully implement the switch to Ubuntu 24.04 (Noble) across all jobs, addressing the PR's main objective. The addition of dependency installation steps in the Linting and Testing jobs ensures that all necessary packages are available for the build process.
These updates should contribute to a more consistent and reliable CI process. However, consider the following suggestions for further improvement:
- Move the dependency installation to a separate script or reusable workflow to reduce duplication and improve maintainability.
- Verify if the Formatting job also requires the newly added dependencies.
- Consider updating the
Swatinem/rust-cache@v2step to cache the newly installed system dependencies, potentially speeding up subsequent runs.To ensure all job dependencies are correctly specified, please run the following command:
This will help confirm that the job execution order is correct given the new dependency installation steps.
✅ Verification successful
Job dependencies correctly configured
The
Testingjob appropriately depends on theFormattingjob, ensuring the correct execution order within the CI workflow. No further issues detected regarding job dependencies.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify job dependencies grep -n "needs:" .github/workflows/rust.ymlLength of output: 68
🧰 Tools
🪛 actionlint
46-46: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
utils/docker/Dockerfile (1)
49-50: Build command updated correctlyThe build command has been appropriately updated to use the Rust toolchain installed in the previous steps. This ensures that the correct Rust environment is used for building the application.
Consider adding the
--lockedflag tocargo buildto ensure reproducible builds:-RUN /root/.cargo/bin/cargo build --release +RUN /root/.cargo/bin/cargo build --release --lockedThis flag ensures that the exact versions of dependencies specified in
Cargo.lockare used, further enhancing build reproducibility..github/workflows/rust.yml (2)
35-43: LGTM: Consistent OS update and dependency installationThe changes to use
ubuntu-24.04and install system dependencies align with the PR objectives. This should help with faster compilation as intended.Consider using a multi-line YAML string for better readability:
run: | sudo apt-get update -y sudo apt-get install -y \ librocksdb-dev \ libsnappy-dev \ libsqlite3-dev
Line range hint
1-107: Summary of changes and action itemsThe changes in this workflow file generally align with the PR objectives:
- Switching to Ubuntu 24.04 (Noble) across all jobs.
- Adding steps to install system dependencies, including RocksDB.
However, there are a few points that need attention:
- Consider improving the readability of the dependency installation steps.
- Clarify the reason for changing the cache key to "covtest".
- Explain the switch from tarpaulin to llvm-cov for code coverage, and update the PR description accordingly.
Overall, these changes should contribute to faster compilation as intended. Please address the above points to ensure all changes are well-understood and documented.
🧰 Tools
🪛 actionlint
46-46: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- .github/workflows/rust.yml (4 hunks)
- utils/docker/Dockerfile (2 hunks)
🧰 Additional context used
🔇 Additional comments (7)
utils/docker/Dockerfile (3)
31-31: Improved dependency caching strategyThe changes to copy
build.rs,Cargo.toml, andCargo.lockfiles together into the project directory is a good optimization. This approach improves the caching of dependencies, which can lead to faster subsequent builds.This change aligns with best practices for Rust projects in Docker and should help in reducing build times.
Also applies to: 37-39
65-68: Improved readability for COPY commandThe formatting change to split the COPY command across multiple lines improves the readability of the Dockerfile without changing its functionality.
This is a good practice for maintaining clean and easily understandable Dockerfiles, especially as they grow in complexity.
77-78: Improved clarity for EXPOSE commandThe update to the EXPOSE command, specifying that it's for the application port, enhances the clarity of the Dockerfile.
This change improves documentation within the Dockerfile, making it easier for other developers to understand the purpose of the exposed port.
.github/workflows/rust.yml (4)
13-13: LGTM: Specific Ubuntu version for improved stabilityChanging from
ubuntu-latesttoubuntu-24.04aligns with the PR objective and provides better stability and reproducibility in CI runs.
31-32: LGTM: Improved formattingThe indentation adjustment for the
runcommand improves readability and consistency with other step properties.
Line range hint
70-95: LGTM with a question: Consistent changes and cache key modificationThe changes to use
ubuntu-24.04and install system dependencies are consistent with other jobs and align with the PR objectives.However, could you please clarify the reason for changing the cache key to "covtest"? This might affect caching behavior, and it would be helpful to understand the intention behind this change.
🧰 Tools
🪛 actionlint
83-83: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
99-107: Please clarify: Switch from tarpaulin to llvm-cov for code coverageThe code coverage steps have been significantly changed, replacing tarpaulin with llvm-cov. While this change may have benefits, it wasn't mentioned in the PR objectives. Could you please:
- Explain the reasoning behind this switch?
- Confirm that this change has been tested and works correctly with Codecov?
- Update the PR description to include this change?
Also, ensure that the
CODECOV_TOKENsecret is properly set in the repository settings.
| # Install Rust toolchain and dependencies for compilation of C code (e.g., rocksdb) | ||
| RUN apt-get update && \ | ||
| apt-get install -y clang | ||
| # Add the needed Cargo components. | ||
| RUN rustup component add rustfmt | ||
| apt-get install -y unzip wget curl build-essential clang librocksdb-dev libsqlite3-dev && \ | ||
| curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y && \ | ||
| . $HOME/.cargo/env && \ | ||
| rustup component add rustfmt |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing Rust installation process
The current approach of manually installing Rust aligns with the PR objective of using the system rocksdb for faster compilation. However, this method can potentially lead to longer build times and maintenance overhead.
Consider the following optimizations:
- Use specific versions for installed packages to ensure reproducibility:
- apt-get install -y unzip wget curl build-essential clang librocksdb-dev libsqlite3-dev && \
+ apt-get install -y unzip=6.0-26ubuntu3.1 wget=1.21.2-2ubuntu1 curl=7.81.0-1ubuntu1.15 build-essential=12.9ubuntu3 clang=1:14.0-55~exp2 librocksdb-dev=6.20.3-1 libsqlite3-dev=3.37.2-2ubuntu0.3 && \- Combine RUN commands to reduce layers and use
--no-install-recommendsto minimize image size:
-RUN apt-get update && \
- apt-get install -y unzip wget curl build-essential clang librocksdb-dev libsqlite3-dev && \
- curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y && \
- . $HOME/.cargo/env && \
- rustup component add rustfmt
+RUN apt-get update && \
+ apt-get install -y --no-install-recommends \
+ unzip=6.0-26ubuntu3.1 \
+ wget=1.21.2-2ubuntu1 \
+ curl=7.81.0-1ubuntu1.15 \
+ build-essential=12.9ubuntu3 \
+ clang=1:14.0-55~exp2 \
+ librocksdb-dev=6.20.3-1 \
+ libsqlite3-dev=3.37.2-2ubuntu0.3 && \
+ curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y && \
+ . $HOME/.cargo/env && \
+ rustup component add rustfmt && \
+ apt-get clean && \
+ rm -rf /var/lib/apt/lists/*These optimizations will help maintain a balance between using system libraries and ensuring a reproducible, efficient build process.
Committable suggestion was skipped due to low confidence.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #555 +/- ##
==========================================
- Coverage 73.14% 70.78% -2.36%
==========================================
Files 79 80 +1
Lines 8199 13151 +4952
==========================================
+ Hits 5997 9309 +3312
- Misses 2202 3842 +1640 ☔ View full report in Codecov by Sentry. |
Solves #554
Summary by CodeRabbit
New Features
Improvements
Bug Fixes