-
Notifications
You must be signed in to change notification settings - Fork 21
Add build for x86_64 x musl x debug x asan #115
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?
Conversation
WalkthroughIntroduces ENABLE_SANITIZERS across CI, Dockerfile.musl, and musl-release.sh; adds a musl ASAN workflow variant Changes
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🔇 Additional comments (2)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/workflows/build-reusable.yml(5 hunks)Dockerfile.musl(6 hunks)musl-release.sh(1 hunks)
🔇 Additional comments (8)
Dockerfile.musl (4)
7-7: ARG propagation pattern is correct.The global declaration of
ENABLE_SANITIZERSat line 7 followed by redeclaration in each build stage (lines 17, 46, 71) correctly implements Docker's ARG scoping. Each stage can now access the value if needed.Also applies to: 17-17, 46-46, 71-71
87-91: Conditional ENABLE_ASSERTS logic looks sound.When sanitizers are enabled (non-empty
ENABLE_SANITIZERS), assertions are forced on viaENABLE_ASSERTS="ON". Otherwise, they default to"AUTO". This is a sensible configuration for debug builds with sanitizers, as assertions help catch issues.
46-46: Informational:ENABLE_SANITIZERSdeclared but unused inbuild_icustage.The
build_icustage declares the ARG but does not propagate it to ICU's compiler flags. This appears intentional—building ICU without sanitizers as a dependency. Confirm this is the desired behavior, especially if ICU should be instrumented for consistent sanitizer coverage.
111-112: Verify CMake option names and values.Lines 111–112 pass
-DENABLE_SANITIZERS="$ENABLE_SANITIZERS"and-DENABLE_ASSERTS="$ENABLE_ASSERTS"to CMake. Ensure these option names and values ("AUTO","ON", and comma-separated sanitizer names) are valid in the WebKit build system.musl-release.sh (1)
43-43: ENABLE_SANITIZERS export is correct.Line 43 properly exports
ENABLE_SANITIZERSwith a default empty value, making it available to the Docker build step..github/workflows/build-reusable.yml (3)
292-297: Matrix entry correctly aligns with PR objectives.The new
bun-webkit-linux-amd64-musl-debug-asanentry properly specifies:
- x86_64 target:
os: linux-x64-gh- Debug build:
CMAKE_BUILD_TYPE: "Debug"- Sanitizers enabled:
ENABLE_SANITIZERS: "address,undefined"This matches the PR goal of limiting asan builds to x86_64 and debug to reduce build time.
338-342: ENABLE_SANITIZERS properly propagated through workflow steps.Environment variable is correctly set from the matrix (line 339) and passed to
musl-release.sh(line 342). For matrix entries withoutENABLE_SANITIZERS, the env var will be unset, whichmusl-release.shhandles safely (defaults to empty string).
463-466: Artifact handling is consistent.
- Download step (lines 463–466) correctly names the artifact
bun-webkit-linux-amd64-musl-debug-asan- Rename step (line 495) handles the artifact consistently with others
- Release step (line 545) includes the asan artifact in the musl release files section
Naming and sequencing are consistent with existing artifacts.
Also applies to: 495-495, 545-545
This PR adds build for ASAN x Musl.
To reduce build time, we limit builds to x86_64 and debug.