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

ci: use prebuilt librocksdb in github actions #2316

Merged
merged 28 commits into from
Nov 8, 2024
Merged

Conversation

lklimek
Copy link
Contributor

@lklimek lklimek commented Nov 6, 2024

Issue being fixed or feature implemented

Building librocksdb takes much time. Each task executing this process

the librocksdb-sys allows use of precompiled rocksdb library.

What was done?

  1. Build librocksdb-sys static library in a separate task. All other tasks just copy this library.
  2. In case of Docker, build deps image with librocksdb static library in separate workflow step.
  3. Moved all tests to free github-hosted runners.

How Has This Been Tested?

GHA

Breaking Changes

None

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
  • I have made corresponding changes to the documentation if needed

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

Summary by CodeRabbit

  • New Features

    • Introduced a new GitHub Actions workflow for rebuilding and caching Rust dependencies.
    • Added a new input parameter force for the librocksdb library build process.
    • Introduced a new input parameter rebuild-deps for optional rebuilding of cached Rust dependencies.
  • Improvements

    • Updated the testing workflows to include the installation of librocksdb across multiple job steps.
    • Upgraded the environment to use ubuntu-24.04 for various jobs, including linting, testing, and documentation publishing.
    • Enhanced the release process with new input parameters for conditional job execution.
  • Bug Fixes

    • Ensured necessary environment variables are set for jobs dependent on librocksdb.

Copy link
Contributor

coderabbitai bot commented Nov 6, 2024

Walkthrough

This pull request introduces modifications to various GitHub Actions workflows, primarily focusing on the integration of the librocksdb library. A new input parameter force has been added to the librocksdb action to allow forced rebuilds. Additionally, workflows for testing Rust packages have been updated to include steps for installing librocksdb. The runner environment has been upgraded to Ubuntu 24.04 across multiple workflows, and a new job for rebuilding cached Rust dependencies has been introduced.

Changes

File Path Change Summary
.github/actions/librocksdb/action.yaml New input parameter force added for rebuilding; retains version and bucket.
.github/workflows/tests-rs-package.yml Updated to ubuntu-24.04; added job step Install librocksdb in jobs lint, unused_deps, test, and check_each_feature; added env vars ROCKSDB_STATIC and ROCKSDB_LIB_DIR.
.github/workflows/tests.yml New job build-rust-deps added; updated changes job to use ubuntu-24.04 and actions/checkout@v4; modified lint-runner and test-runner in rs-packages job.
.github/workflows/cached.yml New workflow for rebuilding cached Rust dependencies created, using actions/checkout@v4 and precompiling librocksdb.
.github/workflows/docs.yml Runner environment updated to ubuntu-24.04 for documentation publishing.
.github/workflows/manage-runs.yml Job environment updated to ubuntu-24.04 for cancelling merged or closed PR runs.
.github/workflows/pr.yml Job environment updated to ubuntu-24.04 for Pull Request Linter.
.github/workflows/release-docker-image.yml Job environment updated to ubuntu-24.04 for building and publishing Docker images.
.github/workflows/release.yml New input parameter only_drive added; job matrix updated to ubuntu-24.04 for package releases.
.github/workflows/tests-codeql.yml Job environment updated to ubuntu-24.04 for CodeQL analysis.
.github/workflows/tests-js-package.yml Runner updated to ubuntu-24.04 for linting and testing jobs; new inputs added for workflow call.

Possibly related PRs

  • ci: prebuild dev containers #2184: This PR introduces a new GitHub Actions workflow for prebuilding development containers, which includes a custom action located in .github/actions/librocksdb, directly related to the modifications made in the main PR regarding the librocksdb library.
  • ci: do not run test on push #2308: This PR modifies the CI process by changing the triggers for tests in the workflow, which may indirectly relate to the overall workflow management that includes the librocksdb library, although it does not directly affect the librocksdb modifications.

Suggested reviewers

  • QuantumExplorer
  • shumkov

Poem

🐇 In the burrow where we play,
A new action hops our way!
With librocksdb built so fine,
Our tests will run, all in line.
From linting checks to features bright,
We build and play from morn till night! 🌟


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@lklimek lklimek changed the title chore(build): optimize github actions build: optimize github actions Nov 6, 2024
@lklimek lklimek changed the title build: optimize github actions build: use prebuilt librocksdb in github actions Nov 6, 2024
@shumkov shumkov changed the title build: use prebuilt librocksdb in github actions ci: use prebuilt librocksdb in github actions Nov 8, 2024
name: Prebuild and cache librocksdb
runs-on: ubuntu-24.04
steps:
- name: Checkout
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We must switch to AWS cache otherwise it's not going to work https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/caching-dependencies-to-speed-up-workflows#matching-a-cache-key

Another option is bring back build on merging PRs to default branch (or scheduled run). But it make sense only if we switch completely to default runners.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switched to s3

@@ -17,7 +17,8 @@ on:
jobs:
build-image:
name: Build ${{ inputs.name }} image
runs-on: ["self-hosted", "linux", "arm64", "ubuntu-platform"]
# runs-on: ["self-hosted", "linux", "arm64", "ubuntu-platform"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How much slower it become? If much slower then let's revert it for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved docker to #2318

@@ -42,6 +42,17 @@ jobs:
with:
filters: .github/package-filters/rs-packages.yml

rust-deps:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

build-rust-deps

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

using: composite
steps:
# Cache librocksdb using s3 bucket
- name: Restore local network data
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

.github/workflows/tests.yml Show resolved Hide resolved
@lklimek lklimek marked this pull request as ready for review November 8, 2024 09:07
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (5)
.github/actions/librocksdb/action.yaml (2)

13-16: Consider making the S3 bucket name configurable per environment.

The hardcoded S3 bucket name might be specific to a particular environment. Consider:

  1. Making it a required parameter without a default value
  2. Using a repository secret or environment variable

47-51: Improve output configuration format.

Consider outputting the configuration in a format that can be easily parsed and used by dependent workflows:

-    echo Done.
-    echo Configuration:
-    echo
-    echo "ROCKSDB_STATIC='/opt/rocksdb/usr/local/lib/librocksdb.a'"
-    echo "ROCKSDB_LIB_DIR='/opt/rocksdb/usr/local/lib'"
+    echo "::set-output name=ROCKSDB_STATIC::/opt/rocksdb/usr/local/lib/librocksdb.a"
+    echo "::set-output name=ROCKSDB_LIB_DIR::/opt/rocksdb/usr/local/lib"
.github/workflows/tests.yml (2)

39-54: Consider adding timeout and cache configuration.

The new job for prebuilding librocksdb is well-structured, but could benefit from additional configurations:

  1. Add a timeout to prevent hung builds
  2. Configure caching strategy for the compiled artifacts
  build-rust-deps:
    name: Prebuild and cache some Rust dependencies
    runs-on: ubuntu-24.04
+   timeout-minutes: 30
    env:
      AWS_ACCESS_KEY_ID: ${{ secrets.AWS_ACCESS_KEY_ID }}
      AWS_SECRET_ACCESS_KEY: ${{ secrets.AWS_SECRET_ACCESS_KEY }}
      AWS_REGION: ${{ secrets.AWS_REGION }}

    steps:
      - name: Checkout
        uses: actions/checkout@v4
        with:
          fetch-depth: 1
+     - name: Cache librocksdb
+       uses: actions/cache@v3
+       with:
+         path: ~/.cargo/registry/cache/librocksdb-sys-*
+         key: ${{ runner.os }}-librocksdb-${{ hashFiles('**/Cargo.lock') }}
+         restore-keys: |
+           ${{ runner.os }}-librocksdb-
      - name: Precompile librocksdb
        uses: ./.github/actions/librocksdb

39-54: Consider future optimization for grovedb release builds.

As suggested in a previous review, consider optimizing further by building librocksdb only once in grovedb release. This could be implemented as a separate workflow that:

  1. Builds librocksdb specifically for grovedb release
  2. Caches the result for reuse across different jobs

Would you like assistance in designing this optimization for a follow-up PR?

.github/workflows/tests-rs-package.yml (1)

236-238: Update checkout action to v4 for consistency.

While the librocksdb changes look good, this job is using actions/checkout@v3 while other jobs use v4. Consider updating for consistency.

      - name: Check out repo
-       uses: actions/checkout@v3
+       uses: actions/checkout@v4

Also applies to: 251-252

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 48cca1a and c939f44.

📒 Files selected for processing (3)
  • .github/actions/librocksdb/action.yaml (1 hunks)
  • .github/workflows/tests-rs-package.yml (8 hunks)
  • .github/workflows/tests.yml (4 hunks)
🔇 Additional comments (5)
.github/workflows/tests.yml (1)

99-103: Verify RAM requirements for Clippy on GitHub-hosted runners.

The transition from self-hosted to GitHub-hosted runners (ubuntu-24.04) requires careful consideration:

  1. Previous comments indicate Clippy failures due to RAM usage
  2. GitHub-hosted runners have limited resources compared to self-hosted runners
#!/bin/bash
# Description: Check resource-intensive operations in Rust packages
# This helps identify potential memory bottlenecks

echo "Checking for resource-intensive build configurations:"
fd Cargo.toml$ | while read -r cargo_file; do
  echo "Analyzing $cargo_file for parallel jobs configuration:"
  rg "cargo.*-j" "$(dirname "$cargo_file")"
done

echo "Checking for large Rust packages that might stress memory:"
fd Cargo.toml$ | while read -r cargo_file; do
  echo "Package size for $(dirname "$cargo_file"):"
  find "$(dirname "$cargo_file")" -type f -name "*.rs" -exec wc -l {} \; | awk '{total += $1} END {print total " lines"}'
done
.github/workflows/tests-rs-package.yml (4)

73-74: LGTM! Good documentation and consistent environment setup.

The comment clearly explains why librocksdb isn't needed for formatting, and the sccache environment variables are consistently configured.

Also applies to: 76-80


204-206: LGTM! Consistent test environment setup.

The librocksdb installation and environment variables are correctly configured for the test job.

Also applies to: 214-215


104-106: Verify the use of non-standard cargo-machete fork.

While the librocksdb changes look good, the job is using a non-standard fork of cargo-machete (lklimek/cargo-machete@feat/workdir). Consider using the official release or document why this fork is necessary.

#!/bin/bash
# Check if the fork is temporary by looking for related TODO/FIXME comments
rg -i "cargo-machete.*feat/workdir" -A 5 -B 5

Also applies to: 120-121


45-47: LGTM! Verify the librocksdb action exists.

The changes look good. The librocksdb installation step and environment variables are correctly placed and configured.

Also applies to: 56-57

✅ Verification successful

The librocksdb action exists and is properly configured

The custom action .github/actions/librocksdb is present and well-structured:

  • Contains proper action metadata and inputs configuration
  • Implements caching using S3 for optimization
  • Includes fallback build process for cache misses
  • Is consistently used across workflow files
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify that the librocksdb action exists
if [ ! -f ".github/actions/librocksdb/action.yml" ] && [ ! -f ".github/actions/librocksdb/action.yaml" ]; then
    echo "Error: librocksdb action file not found"
    exit 1
fi

Length of output: 141


Script:

#!/bin/bash
# Let's check if the action directory exists and list its contents
ls -la .github/actions/librocksdb/ 2>/dev/null || echo "Directory not found"

# Also search for any files containing librocksdb in the .github directory
find .github -type f -exec grep -l "librocksdb" {} \;

Length of output: 374


Script:

#!/bin/bash
# Let's verify the content of the action.yaml to ensure it's properly configured
cat .github/actions/librocksdb/action.yaml

# Also check how it's used in other workflow files
echo "=== Usage in tests.yml ==="
grep -A 5 -B 5 "librocksdb" .github/workflows/tests.yml

Length of output: 2045

.github/actions/librocksdb/action.yaml Outdated Show resolved Hide resolved
.github/actions/librocksdb/action.yaml Show resolved Hide resolved
.github/actions/librocksdb/action.yaml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Show resolved Hide resolved
Copy link
Member

@shumkov shumkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (3)
.github/actions/librocksdb/action.yaml (1)

49-53: Consider using GitHub Actions environment files.

Instead of just echoing the configuration, consider setting these values in the GitHub Actions environment file for better reusability:

-echo "ROCKSDB_STATIC='/opt/rocksdb/usr/local/lib/librocksdb.a'"
-echo "ROCKSDB_LIB_DIR='/opt/rocksdb/usr/local/lib'"
+echo "ROCKSDB_STATIC=/opt/rocksdb/usr/local/lib/librocksdb.a" >> $GITHUB_ENV
+echo "ROCKSDB_LIB_DIR=/opt/rocksdb/usr/local/lib" >> $GITHUB_ENV
+echo "Configuration set in GITHUB_ENV:"
+echo "ROCKSDB_STATIC=$ROCKSDB_STATIC"
+echo "ROCKSDB_LIB_DIR=$ROCKSDB_LIB_DIR"
.github/workflows/tests-js-package.yml (1)

Line range hint 54-58: Consider adding test result artifacts

While the test job configuration is correct, consider adding test result artifacts for better visibility and debugging capabilities.

Add these steps after the test execution:

      - name: Upload test results
        if: always()
        uses: actions/upload-artifact@v3
        with:
          name: ${{ inputs.package }}-test-results
          path: |
            packages/${{ inputs.package }}/coverage
            packages/${{ inputs.package }}/test-results
          retention-days: 7
.github/workflows/tests.yml (1)

44-59: LGTM: Well-implemented dependency prebuild job.

The new build-rust-deps job effectively implements the PR's objective to optimize the build process. Good practices include:

  • Proper secrets handling for AWS credentials
  • Minimal git history fetch
  • Integration with librocksdb action

Consider adding a comment explaining the purpose of AWS credentials (e.g., for caching artifacts).

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between c939f44 and 691afb2.

📒 Files selected for processing (11)
  • .github/actions/librocksdb/action.yaml (1 hunks)
  • .github/workflows/cached.yml (1 hunks)
  • .github/workflows/docs.yml (1 hunks)
  • .github/workflows/manage-runs.yml (1 hunks)
  • .github/workflows/pr.yml (1 hunks)
  • .github/workflows/release-docker-image.yml (1 hunks)
  • .github/workflows/release.yml (1 hunks)
  • .github/workflows/tests-codeql.yml (1 hunks)
  • .github/workflows/tests-js-package.yml (2 hunks)
  • .github/workflows/tests-rs-package.yml (10 hunks)
  • .github/workflows/tests.yml (7 hunks)
✅ Files skipped from review due to trivial changes (3)
  • .github/workflows/cached.yml
  • .github/workflows/docs.yml
  • .github/workflows/pr.yml
🚧 Files skipped from review as they are similar to previous changes (1)
  • .github/workflows/tests-rs-package.yml
🧰 Additional context used
📓 Learnings (1)
.github/actions/librocksdb/action.yaml (1)
Learnt from: lklimek
PR: dashpay/platform#2316
File: .github/actions/librocksdb/action.yaml:22-28
Timestamp: 2024-11-08T09:27:23.601Z
Learning: Prefer to use `librocksdb/${{ inputs.version }}/${{ runner.os }}/${{ runner.arch }}` as the cache key for caching librocksdb in workflows; it's sufficient.
🔇 Additional comments (14)
.github/workflows/tests-codeql.yml (2)

Line range hint 1-58: LGTM! Minimal changes approach is appropriate.

The workflow focuses on JavaScript code analysis and doesn't interact with Rust packages or librocksdb. The single environment update is sufficient, and no additional changes are needed for the librocksdb optimization objective.


7-7: Verify compatibility with Ubuntu 24.04 (pre-release).

Ubuntu 24.04 is currently in development and not yet released. While GitHub Actions provides early access to these runners, there might be stability concerns or compatibility issues with certain tools and libraries.

Let's verify the compatibility of the actions used in this workflow with Ubuntu 24.04:

Consider the following recommendations:

  1. Test the workflow thoroughly to ensure all required dependencies are available
  2. Have a fallback plan to revert to Ubuntu 22.04 if issues arise
  3. Monitor the workflow's stability during the transition period
✅ Verification successful

Let me gather more information about the workflow file and its dependencies.


Let me check the Node.js setup action since it's a local action and CodeQL is specifically analyzing JavaScript.


Ubuntu 24.04 upgrade appears safe for CodeQL workflow

Based on the evidence gathered:

  1. The repository has already standardized on Ubuntu 24.04 across multiple workflows (tests, release, docs, etc.)
  2. CodeQL workflow is JavaScript-focused with minimal system dependencies
  3. All critical actions used (checkout@v4, docker/login-action@v3, aws-actions/configure-aws-credentials@v4, codeql-action@v2) are actively maintained
  4. No critical compatibility issues found in action repositories regarding Ubuntu 24.04

While Ubuntu 24.04 is pre-release, GitHub Actions provides official support for it, and the widespread usage across the repository's workflows suggests it's already working reliably in this codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if any issues or discussions mention compatibility problems with Ubuntu 24.04
# for the actions used in this workflow

# Test: Search for relevant GitHub issues. Expect: No critical compatibility issues reported.
gh issue list -R actions/checkout -S "ubuntu 24.04"
gh issue list -R docker/login-action -S "ubuntu 24.04"
gh issue list -R aws-actions/configure-aws-credentials -S "ubuntu 24.04"
gh issue list -R github/codeql-action -S "ubuntu 24.04"

Length of output: 401


Script:

#!/bin/bash
# Check the workflow file content to understand dependencies and configurations
cat .github/workflows/tests-codeql.yml

# Check if there are any other workflow files using ubuntu-24.04
rg "ubuntu-24.04" .github/workflows/

Length of output: 3115


Script:

#!/bin/bash
# Check the local Node.js setup action
cat .github/actions/nodejs/action.yml

# Check CodeQL config
cat .github/codeql/codeql-config.yml

# Check if there are any recent workflow runs using ubuntu-24.04
gh run list --workflow=tests-codeql.yml --limit 5

Length of output: 279

.github/workflows/manage-runs.yml (1)

10-10: LGTM! Verify Ubuntu 24.04 runner availability.

The update to Ubuntu 24.04 aligns with similar changes across other workflow files, maintaining consistency in the CI environment.

Let's verify the runner availability and other workflows using Ubuntu 24.04:

✅ Verification successful

Ubuntu 24.04 runner update is consistent with other workflows

The verification shows that the update to ubuntu-24.04 in manage-runs.yml aligns perfectly with other GitHub-hosted runners in the repository. All standard Ubuntu runners are consistently using version 24.04, while self-hosted runners use a custom ubuntu-platform label. This change maintains uniformity across the CI environment.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify Ubuntu 24.04 runner usage across workflows
# Expected: All workflow files should use consistent Ubuntu version

# Check for any inconsistencies in Ubuntu versions across workflow files
echo "Checking Ubuntu versions in workflow files..."
rg -A 1 "runs-on:" .github/workflows/

Length of output: 3825

.github/actions/librocksdb/action.yaml (1)

12-15: Consider making the S3 bucket name more configurable.

The hardcoded S3 bucket name multi-runner-cache-x1xibo9c might cause issues for repository forks or different environments. Consider using a repository secret or organization-level variable instead.

Let's check if this bucket name is used elsewhere:

.github/workflows/tests-js-package.yml (2)

20-20: LGTM! Runner version update is appropriate.

The upgrade to Ubuntu 24.04 aligns with using the latest LTS version and matches the PR's objective of standardizing on GitHub-hosted runners.


Line range hint 1-13: Verify package name input sanitization

The package input is directly used in commands. While GitHub Actions provides some built-in protection, it's good practice to verify that the package name follows expected patterns.

✅ Verification successful

Package input usage is safe and follows expected patterns

The package input is safely used with yarn workspace command and matches the existing package names in the monorepo. All package names follow standard npm naming conventions (@dashevo/* and dash*) and the workflow uses proper syntax for workspace commands.

  • The input is used in two places:
    • yarn workspace "${{ inputs.package }}" lint
    • yarn workspace "${{ inputs.package }}" ${{ inputs.test-command }}
  • Package names are properly scoped and validated through yarn workspace command
  • The double-quoted syntax provides additional safety against command injection
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify package names in package.json match expected patterns
# Expected: Package names should follow npm naming conventions

# Search for valid package names in package.json files
rg -U --json '^{\s*"name":\s*"[^"]+"\s*,' | \
  jq -r 'select(.data.lines | test("^{\\s*\\"name\\":\\s*\\"[a-z0-9-_.]+\\"\\s*,")) | .data.lines' | \
  sed 's/.*"name":\s*"\([^"]*\)".*/\1/'

Length of output: 555


Script:

#!/bin/bash
# Let's try a simpler approach to find package.json files and their names
fd package.json --type f --exec sh -c 'echo "=== {} ==="; head -n 10 {}'

Length of output: 6277


Script:

#!/bin/bash
# Let's also check how the package input is used in the workflow file
cat .github/workflows/tests-js-package.yml

Length of output: 2715

.github/workflows/release-docker-image.yml (1)

92-92: LGTM! Verify Docker tools compatibility.

The upgrade to Ubuntu 24.04 aligns with the broader workflow updates across the repository. While this change should work seamlessly for Docker operations, let's verify the availability of required tools.

✅ Verification successful

Docker tools compatibility verified across workflows

The verification shows:

  • Most workflows are consistently using Ubuntu 24.04, aligning with this change
  • Docker actions being used (login-action, metadata-action) are all on recent versions (v3 or v5)
  • Only one workflow using an older Docker action version (prebuild-devcontainers.yml with login-action@v2)
  • The Docker manifest creation job in this workflow uses the latest Docker action versions
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that other workflow files are consistently using Ubuntu 24.04
# and check for any potential Docker-related configurations that might need adjustment

# Check for consistent runner versions across workflow files
echo "Checking runner versions in workflow files..."
rg -A 2 "runs-on:" .github/workflows/

# Look for any Docker-specific configurations that might need adjustment
echo "Checking Docker-related configurations..."
rg "docker/(setup|login|build|metadata)-action@v" .github/workflows/

Length of output: 6524

.github/workflows/tests.yml (4)

5-9: LGTM: Well-structured workflow dispatch input.

The new rebuild-deps input parameter is properly configured with clear description and safe defaults, providing good control over dependency rebuilding.


114-114: LGTM: Consistent runner and action updates.

The updates to ubuntu-24.04 and checkout@v4 are consistently applied across the security audit, dependency version check, and NPM security jobs.

Also applies to: 143-143, 162-162


93-93: Verify RAM requirements for Clippy on GitHub-hosted runners.

The switch from self-hosted to GitHub-hosted runners (ubuntu-24.04) might cause issues with Clippy due to RAM constraints, as indicated by the comments. This could impact the CI reliability.

#!/bin/bash
# Description: Check Clippy memory requirements and GitHub runner specifications

echo "Checking Clippy memory usage patterns:"
rg -B 2 -A 2 "clippy" .github/workflows/

echo -e "\nChecking for any memory-related configurations or issues:"
rg -i "memory|ram" .github/workflows/

Consider:

  1. Profiling Clippy's memory usage
  2. Splitting the linting job into smaller chunks
  3. Using GitHub runners with more RAM if available

Also applies to: 102-108


24-24: Verify Ubuntu version and checkout action consistency.

The upgrade to ubuntu-24.04 and checkout@v4 are good improvements. However, let's verify consistency across all workflow files.

Also applies to: 30-30

✅ Verification successful

Ubuntu and checkout action versions are consistent across workflows

All workflow files consistently use:

  • ubuntu-24.04 as the runner
  • actions/checkout@v4 for the checkout action
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for inconsistent Ubuntu versions and checkout actions
# across all workflow files

echo "Checking workflow files for Ubuntu versions:"
rg "runs-on:\s*(ubuntu-|self-hosted)" .github/workflows/

echo -e "\nChecking workflow files for checkout actions:"
rg "uses:\s*actions/checkout@" .github/workflows/

Length of output: 1231


Script:

#!/bin/bash
# Let's try again with proper quoting and specific patterns
echo "Checking workflow files for Ubuntu versions:"
rg -i "runs-on:\s*ubuntu-" .github/workflows/

echo -e "\nChecking workflow files for checkout action versions:"
rg -i "uses:\s*actions/checkout@" .github/workflows/

Length of output: 3082

.github/workflows/release.yml (3)

Line range hint 11-14: LGTM: Well-structured workflow input parameter addition.

The new only_drive input parameter is properly typed, documented, and provides good control over the release process by allowing targeted Drive-only releases.


Line range hint 184-186: LGTM: Proper implementation of conditional job execution.

The conditional execution using if: ${{ !inputs.only_drive }} is correctly implemented across the DAPI, Test Suite, and Dashmate Helper image jobs, allowing for selective builds when only Drive changes are needed.

Also applies to: 192-194, 202-204


211-215: Verify build dependencies compatibility with Ubuntu 24.04.

The upgrade from Ubuntu 22.04 to 24.04 for multiple package types could potentially impact build dependencies. Please ensure that all required build tools and dependencies (especially for Windows and Debian package builds) are compatible with Ubuntu 24.04.

Let's verify the availability of required build dependencies:

✅ Verification successful

Build dependencies are compatible with Ubuntu 24.04

Based on the workflow file analysis:

  • The main build dependencies are Node.js (installed via custom action), NSIS (installed via apt-get), and Docker
  • The workflow uses standard GitHub actions and custom actions that are platform-agnostic
  • The only system-level package explicitly installed is NSIS, which is available in Ubuntu 24.04 repositories
  • The rest of the build process relies on Node.js/Yarn which are version-controlled through the custom setup action

The upgrade from Ubuntu 22.04 to 24.04 should not impact the build process as all critical dependencies are either explicitly installed or managed through version-controlled actions.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if critical build dependencies are available in Ubuntu 24.04 repositories

# Test: Check if NSIS (for Windows builds) is available in Ubuntu 24.04
apt-cache policy nsis | grep -A1 "Version table"

# Test: Check other build dependencies that might be required
apt-cache policy build-essential dpkg-dev debhelper | grep -A1 "Version table"

Length of output: 285


Script:

#!/bin/bash
# Let's check the workflow file for build dependencies and tools being used
rg -A 5 "apt-get|npm|yarn|pip|cargo|go" .github/workflows/release.yml

# Also check for any setup or installation steps
rg -A 5 "setup-|install|uses:" .github/workflows/release.yml

# Check if there are any dependency files that might list requirements
fd -e json -e toml -e lock package

Length of output: 6403

Comment on lines +25 to +28
- name: Restore cached librocksdb from S3
id: librocksdb-cache
uses: strophy/actions-cache@opendal-update
with:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Consider pinning the forked cache action to a specific commit.

Using a fork of actions/cache without pinning to a specific commit hash could lead to unexpected behavior if the fork is updated. Consider either:

  1. Using the official actions/cache@v4 with S3 support
  2. Pinning the current fork to a specific commit hash

Example:

-      uses: strophy/actions-cache@opendal-update
+      uses: strophy/actions-cache@commit-hash  # Replace with actual commit hash

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +36 to +47
run: |
set -ex
WORKDIR=/tmp/rocksdb-build
mkdir -p ${WORKDIR}/rocksdb
mkdir -p /opt/rocksdb/usr/local/lib/
pushd ${WORKDIR}/rocksdb

# building rocksdb
git clone https://github.com/facebook/rocksdb.git -b v${{ inputs.version }} --depth 1 .
make -j$(nproc) static_lib
make DESTDIR=/opt/rocksdb install-static
set +x
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add cleanup and basic error handling to the build process.

The build process should clean up temporary files and include basic error handling:

 run: |
   set -ex
   WORKDIR=/tmp/rocksdb-build
+  trap 'rm -rf ${WORKDIR}' EXIT ERR
   mkdir -p ${WORKDIR}/rocksdb
   mkdir -p /opt/rocksdb/usr/local/lib/
   pushd ${WORKDIR}/rocksdb

   # building rocksdb
   git clone https://github.com/facebook/rocksdb.git -b v${{ inputs.version }} --depth 1 .
   make -j$(nproc) static_lib
   make DESTDIR=/opt/rocksdb install-static
+  
+  # Verify build success
+  if [[ ! -f /opt/rocksdb/usr/local/lib/librocksdb.a ]]; then
+    echo "Error: Static library not found after build"
+    exit 1
+  fi
   set +x
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
run: |
set -ex
WORKDIR=/tmp/rocksdb-build
mkdir -p ${WORKDIR}/rocksdb
mkdir -p /opt/rocksdb/usr/local/lib/
pushd ${WORKDIR}/rocksdb
# building rocksdb
git clone https://github.com/facebook/rocksdb.git -b v${{ inputs.version }} --depth 1 .
make -j$(nproc) static_lib
make DESTDIR=/opt/rocksdb install-static
set +x
run: |
set -ex
WORKDIR=/tmp/rocksdb-build
trap 'rm -rf ${WORKDIR}' EXIT ERR
mkdir -p ${WORKDIR}/rocksdb
mkdir -p /opt/rocksdb/usr/local/lib/
pushd ${WORKDIR}/rocksdb
# building rocksdb
git clone https://github.com/facebook/rocksdb.git -b v${{ inputs.version }} --depth 1 .
make -j$(nproc) static_lib
make DESTDIR=/opt/rocksdb install-static
# Verify build success
if [[ ! -f /opt/rocksdb/usr/local/lib/librocksdb.a ]]; then
echo "Error: Static library not found after build"
exit 1
fi
set +x

@lklimek lklimek merged commit 5c80069 into v1.6-dev Nov 8, 2024
81 of 85 checks passed
@lklimek lklimek deleted the build/optimize-memory branch November 8, 2024 09:50
@coderabbitai coderabbitai bot mentioned this pull request Nov 13, 2024
6 tasks
@coderabbitai coderabbitai bot mentioned this pull request Nov 21, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants