-
-
Notifications
You must be signed in to change notification settings - Fork 29
add CI with ASAN #910
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
base: main
Are you sure you want to change the base?
add CI with ASAN #910
Conversation
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA new GitHub Actions workflow file has been added to the repository. The workflow is triggered by pushes to the main branch and specific pull request events. It sets up a Ruby environment on the latest Ubuntu version with AddressSanitizer enabled. The workflow performs steps such as code checkout, Ruby setup, DuckDB caching and building, dependency installation, building with deprecated API disabled, and finally running tests. Changes
Sequence Diagram(s)sequenceDiagram
participant W as Workflow Trigger
participant R as Runner
W->>R: Trigger on push/PR event
R->>R: Checkout Code
R->>R: Set Up Ruby Environment
R->>R: Cache/Build DuckDB
R->>R: Bundle Install Dependencies
R->>R: Build with DUCKDB_API_NO_DEPRECATED Disabled
R->>R: Standard Build Execution
R->>R: Run Tests with AddressSanitizer
Poem
🪧 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 (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/ruby_asan_on_ubuntu.yml (4)
1-2
: Descriptive Workflow Naming
Consider renaming the workflow from "Ubuntu" to something like "Ruby ASAN on Ubuntu" to more clearly reflect its purpose of running AddressSanitizer tests for Ruby.
29-35
: DuckDB Cache Configuration
The DuckDB cache step is well-configured to cache builds based on the DuckDB version. In the future, if build-related flags are added or modified, consider including them in the cache key to prevent stale caches.
36-47
: DuckDB Build Step
This step clones the specified DuckDB version from GitHub, builds it, and organizes the built artifacts correctly. A couple of suggestions:
- Avoid using the interactive flag (
-i
) in thecp -rip
commands to prevent potential prompts in a CI environment; consider changing tocp -rp
.- Optionally, remove the temporary clone directory (
duckdb-tmp-v$DUCKDB_VERSION
) after the build to conserve disk space.
67-72
: ASAN Test Execution
The test step runs the test suite withRUBYOPT=-W:deprecated
andASAN_TEST=1
, aligning with the PR's goal of incorporating AddressSanitizer testing. If suppressing deprecated warnings is meant to avoid noise, consider adding inline documentation to clarify its intent to future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ruby_asan_on_ubuntu.yml
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: test (head, 1.2.0)
- GitHub Check: test (3.4.1, 1.1.1)
- GitHub Check: test (3.4.2, 1.1.1)
- GitHub Check: test (3.3.6, 1.1.1)
- GitHub Check: test (3.3.6, 1.1.3)
- GitHub Check: test (3.3.6, 1.1.3)
- GitHub Check: test (3.3.6, 1.2.0)
- GitHub Check: test (3.3.6, 1.2.0)
- GitHub Check: test (3.2.6, 1.1.1)
- GitHub Check: test (3.2.6, 1.1.3)
- GitHub Check: test (3.2.6, 1.2.0)
- GitHub Check: test (3.1.6, 1.1.1)
- GitHub Check: test (3.1.6, 1.1.3)
- GitHub Check: test (asan, 1.1.1)
- GitHub Check: test (3.1.6, 1.2.0)
- GitHub Check: test (asan, 1.1.3)
🔇 Additional comments (7)
.github/workflows/ruby_asan_on_ubuntu.yml (7)
3-12
: Trigger Conditions and Branch Filtering
The "on" block correctly triggers on pushes to the main branch and the key pull request events (opened, synchronize, reopened). This setup effectively ensures that CI runs on the relevant code changes.
13-20
: Job Matrix Configuration
The job matrix is set withruby: ['asan']
and multiple DuckDB versions ('1.1.3'
,'1.1.1'
,'1.2.0'
). Ensure that the identifier"asan"
correctly corresponds to an AddressSanitizer-enabled Ruby build in your environment.
21-23
: Checkout Step Validation
The checkout step usingactions/checkout@v4
is implemented correctly to retrieve the repository’s code.
24-28
: Ruby Environment Setup
The Ruby setup step usingruby/setup-ruby@v1
withruby-version: ${{ matrix.ruby }}
is correctly configured. Just double-check that the "asan" tag resolves to the intended ASAN-enabled Ruby version.
48-53
: Bundle Install Step
The bundle install command (bundle install --jobs 4 --retry 3
) is configured appropriately to speed up dependency resolution with parallel jobs and retries.
54-60
: Deprecated API Build Test
The build step that setsDUCKDB_API_NO_DEPRECATED=1
ensures that deprecated APIs are disabled during this test build, and the subsequentbundle exec rake clean
prepares the environment for the next build. Verify that the cleaning does not remove any artifacts required downstream.
61-66
: Standard Build Step
This build step re-invokes the build process without the deprecated API flag. Confirm that having both build variants is intentional; if they serve distinct purposes (e.g., testing vs. production), the separation is justified. Otherwise, consider consolidating them to simplify the workflow.
e9cbf15
to
e855b5e
Compare
- add ruby-asan CI test - skip some failed test
aaa4869
to
5b132ac
Compare
Summary by CodeRabbit