Skip to content

Conversation

@sosukesuzuki
Copy link
Member

What does this PR do?

Previously our build script displays the following error message, when running bun run debug on Alpine:

CMake Error at cmake/tools/SetupWebKit.cmake:97 (file):
  file RENAME failed to rename

    /root/bun/build/debug/cache/bun-webkit

  to

    /root/bun/build/debug/cache/webkit-6d0f3aac0b817cc0

  because: No such file or directory

Call Stack (most recent call first):
  cmake/targets/BuildBun.cmake:1174 (include)
  CMakeLists.txt:56 (include)

This isn't easy to understand.

This PR improves build error message for asan x musl.

How did you verify your code works?

Tested on my local env.

@robobun
Copy link
Collaborator

robobun commented Oct 27, 2025

Updated 9:08 PM PT - Oct 26th, 2025

@sosukesuzuki, your commit 9118d17 has 1 failures in Build #30317 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 24116

That installs a local version of the PR into your bun-24116 executable, so you can run:

bun-24116 --bun

@sosukesuzuki sosukesuzuki marked this pull request as ready for review October 27, 2025 03:20
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 27, 2025

Walkthrough

Adds a runtime guard in the CMake build configuration that prevents building WebKit with ASAN enabled on Linux systems using musl. When this unsupported combination is detected, the build aborts with a fatal error and provides alternative options.

Changes

Cohort / File(s) Change summary
ASAN-musl build guard
cmake/tools/SetupWebKit.cmake
Adds a runtime guard that aborts the build with a fatal error when ASAN is enabled with musl ABI on Linux, including guidance on alternative options.

Pre-merge checks

✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "Add detailed build error on asan x musl" directly and clearly summarizes the main change from the changeset. The raw summary confirms that the change adds a runtime guard to abort the build with a detailed fatal error when attempting to build with both AddressSanitizer and musl libc, which matches the title's focus. The title is concise, specific, and avoids vague terminology or noise.
Description Check ✅ Passed The pull request description follows the required template structure exactly, including both mandatory sections: "What does this PR do?" and "How did you verify your code works?". The first section explains the problem (unclear error message on Alpine) and the solution (improving the error message for asan x musl), while the second section confirms the change was tested locally. Both sections contain substantive content that addresses the objectives of the pull request.

📜 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 b7ae21d and 9118d17.

📒 Files selected for processing (1)
  • cmake/tools/SetupWebKit.cmake (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Format
🔇 Additional comments (1)
cmake/tools/SetupWebKit.cmake (1)

80-87: Excellent improvement to developer experience!

This check effectively prevents the cryptic CMake error and provides clear, actionable guidance. The error message is well-structured with specific alternatives and available options.

Verification complete:

  • ✓ Build script build:debug:noasan confirmed to exist and properly disables ASAN with -DENABLE_ASAN=OFF
  • ✓ Available musl builds (debug, release, lto) are accurate and match the cmake build type logic

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

@Jarred-Sumner
Copy link
Collaborator

we could try enabling with ASAN

@sosukesuzuki
Copy link
Member Author

oven-sh/WebKit#115

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.

4 participants