Skip to content

Conversation

@sosukesuzuki
Copy link
Member

This PR adds build for ASAN x Musl.

To reduce build time, we limit builds to x86_64 and debug.

@coderabbitai
Copy link

coderabbitai bot commented Oct 27, 2025

Walkthrough

Introduces ENABLE_SANITIZERS across CI, Dockerfile.musl, and musl-release.sh; adds a musl ASAN workflow variant bun-webkit-linux-amd64-musl-debug-asan, and propagates sanitizer flags and artifact handling through build, run, and release steps.

Changes

Cohort / File(s) Summary
Musl sanitizer pipeline & artifacts
.github/workflows/build-reusable.yml, Dockerfile.musl, musl-release.sh
Adds ENABLE_SANITIZERS to workflow matrix and steps; introduces bun-webkit-linux-amd64-musl-debug-asan variant with artifact download/rename and release packaging. Adds ENABLE_SANITIZERS build-arg in Dockerfile.musl, sets ENABLE_ASSERTS conditionally, and forwards sanitizer flags to CMake. musl-release.sh exports ENABLE_SANITIZERS and passes it to docker buildx build via --build-arg.

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The pull request description is largely incomplete compared to the required template. The description is missing critical sections including a Bugzilla bug reference (which the template states "All changes should be associated with a bug"), a reviewed-by line, and a detailed file-level breakdown of changes. While the description does provide context about what the PR accomplishes and the rationale for the build limitations, it does not follow the expected template structure with bug tracking and change documentation. Update the pull request description to include a reference to the associated Bugzilla bug (format: https://bugs.webkit.org/show_bug.cgi?id=#####), add a "Reviewed by" line, and provide a detailed breakdown of which files were changed and what modifications were made to each file. This aligns with the repository's pull request template requirements.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The title "Add build for x86_64 x musl x debug x asan" is fully aligned with the changeset. The pull request adds a new build variant for the x86_64 architecture with musl, debug configuration, and ASAN (AddressSanitizer) support across the workflow, Dockerfile, and build scripts. The title is concise, specific about what's being added, and clearly communicates the primary change a teammate would understand when scanning history.

📜 Recent 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 7ef045b and 806ace7.

📒 Files selected for processing (1)
  • musl-release.sh (1 hunks)
🔇 Additional comments (2)
musl-release.sh (2)

43-43: ✓ Consistent export pattern for optional sanitizer configuration.

The addition of ENABLE_SANITIZERS export follows the established pattern (matching LTO_FLAG on line 9) and correctly defaults to an empty string for optional feature flags.


48-48: ✓ Past quoting concern has been addressed.

The docker buildx invocation now properly quotes the ENABLE_SANITIZERS variable (--build-arg ENABLE_SANITIZERS="$ENABLE_SANITIZERS"), which resolves the previous nitpick about consistency and robustness. The quoting is now aligned with the LTO_FLAG pattern and other build-arg values on the same line.

Summary

The changes to musl-release.sh are well-implemented and address the prior review feedback:

Line 43: Clean addition of ENABLE_SANITIZERS export with default empty value, following the existing LTO_FLAG pattern
Line 48: Docker buildx invocation properly quotes the ENABLE_SANITIZERS variable, resolving the previous nitpick about consistency

The propagation of the sanitizer flags from the script through to Dockerfile.musl is now complete and consistent with the broader PR objective of adding ASAN build support for x86_64 musl debug configurations.

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@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: 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 26e6460 and 7ef045b.

📒 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_SANITIZERS at 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 via ENABLE_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_SANITIZERS declared but unused in build_icu stage.

The build_icu stage 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_SANITIZERS with 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-asan entry 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 without ENABLE_SANITIZERS, the env var will be unset, which musl-release.sh handles 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

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.

2 participants