Replace Cygwin-based ICU build with MSBuild on Windows - #136
Conversation
- Remove Cygwin dependency from Windows build pipeline - Build ICU as static libraries using MSBuild with /MT (static CRT) - Patch vcxproj files to configure static library build: - ConfigurationType: DynamicLibrary -> StaticLibrary - RuntimeLibrary: /MD -> /MT (or /MDd -> /MTd for Debug) - Add U_STATIC_IMPLEMENTATION preprocessor definition - Add ARM64 platform support parameter to windows-release.ps1 - Update preprocess.pl to support msys in addition to MSWin32 This change simplifies the Windows build by eliminating the Cygwin toolchain while maintaining the same output artifacts. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
/build-preview |
|
🔄 Preview build started Release tag: |
WalkthroughThe pull request updates Windows build infrastructure by removing Cygwin dependency from the build workflow, adding MSYS support to preprocessing logic, and refactoring the Windows release script to use MSBuild-based ICU construction with a new Platform parameter instead of autotools-based approaches. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
windows-release.ps1 (1)
284-284: Hardcoded--config Releaseignores$CMAKE_BUILD_TYPE.The
--configflag is hardcoded toRelease, but$CMAKE_BUILD_TYPEcan beDebug(as seen in the workflow matrix). Debug builds will incorrectly use Release configuration, producing wrong artifacts.🐛 Proposed fix
-cmake --build $WebKitBuild --config Release --target jsc --verbose +cmake --build $WebKitBuild --config $CMAKE_BUILD_TYPE --target jsc --verbose
🤖 Fix all issues with AI agents
In @windows-release.ps1:
- Around line 61-107: Patch-IcuVcxProj currently declares an unused
$Configuration parameter; either remove the parameter from the function
signature and all callers that pass it, or make it actually control the
runtime-library replacements: keep the parameter and replace the unconditional
RuntimeLibrary -replace calls with conditional logic that checks $Configuration
(e.g., if ($Configuration -eq 'Release') apply the MultiThreaded replacement;
elseif ($Configuration -eq 'Debug') apply the MultiThreadedDebug replacement),
leaving other transformations unchanged; finally update the call site(s) that
invoke Patch-IcuVcxProj to either stop passing a configuration (if you removed
the param) or to pass the correct 'Release'/'Debug' string (if you implemented
the conditional behavior).
📜 Review details
Configuration used: Organization 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.ymlSource/JavaScriptCore/inspector/scripts/codegen/preprocess.plwindows-release.ps1
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-01T01:58:56.267Z
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: Source/JavaScriptCore/CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:56.267Z
Learning: Applies to Source/JavaScriptCore/**/*.{cpp,h} : Guard Bun-specific features with USE(BUN_JSC_ADDITIONS) and event-loop integration with USE(BUN_EVENT_LOOP)
Applied to files:
windows-release.ps1
🔇 Additional comments (7)
.github/workflows/build-reusable.yml (1)
260-260: LGTM!The comment clearly documents why Cygwin is no longer needed. This helps future maintainers understand the build system transition.
Source/JavaScriptCore/inspector/scripts/codegen/preprocess.pl (2)
42-42: LGTM!Correctly extends the preprocessor selection to treat MSYS environments equivalently to MSWin32, which aligns with the MSBuild-based Windows build approach.
88-88: LGTM!Consistent with line 42 - ensures the execution method also recognizes MSYS as a Windows-like environment.
windows-release.ps1 (4)
1-25: LGTM!Good implementation of the Platform parameter with proper validation. The architecture mapping logic correctly translates between the parameter values and the VSDevShell expected values.
137-181: LGTM!The MSBuild discovery and ICU build workflow is well-structured with proper error handling. The build order (stubdata → common → i18n) correctly respects ICU's internal dependencies.
309-319: LGTM!The ICU library copying correctly applies the 's' prefix naming convention for static libraries and adds the 'd' suffix for Debug builds, matching Bun's expected naming scheme.
346-348: LGTM!ICU headers are correctly copied to the output include directory, and the comment provides useful context.
| function Patch-IcuVcxProj { | ||
| param( | ||
| [string]$FilePath, | ||
| [string]$Configuration | ||
| ) | ||
|
|
||
| $BackupPath = "$FilePath.bak" | ||
|
|
||
| if (-not (Test-Path $FilePath)) { | ||
| throw "File not found: $FilePath" | ||
| } | ||
|
|
||
| # Create backup if not exists | ||
| if (-not (Test-Path $BackupPath)) { | ||
| Copy-Item $FilePath $BackupPath | ||
| Write-Host " Backed up: $(Split-Path -Leaf $FilePath)" | ||
| } | ||
|
|
||
| $content = Get-Content $FilePath -Raw | ||
|
|
||
| # 1. Change ConfigurationType from DynamicLibrary to StaticLibrary | ||
| $content = $content -replace '<ConfigurationType>DynamicLibrary</ConfigurationType>', '<ConfigurationType>StaticLibrary</ConfigurationType>' | ||
|
|
||
| # 2. Change RuntimeLibrary for Release: MultiThreadedDLL -> MultiThreaded | ||
| $content = $content -replace '<RuntimeLibrary>MultiThreadedDLL</RuntimeLibrary>', '<RuntimeLibrary>MultiThreaded</RuntimeLibrary>' | ||
|
|
||
| # 3. Change RuntimeLibrary for Debug: MultiThreadedDebugDLL -> MultiThreadedDebug | ||
| $content = $content -replace '<RuntimeLibrary>MultiThreadedDebugDLL</RuntimeLibrary>', '<RuntimeLibrary>MultiThreadedDebug</RuntimeLibrary>' | ||
|
|
||
| # 4. Add U_STATIC_IMPLEMENTATION to preprocessor definitions (if not already present) | ||
| if ($content -notmatch 'U_STATIC_IMPLEMENTATION') { | ||
| $content = $content -replace '(<PreprocessorDefinitions>)', '$1U_STATIC_IMPLEMENTATION;' | ||
| } | ||
|
|
||
| # 5. Remove DLL-specific link settings | ||
| $content = $content -replace '<OutputFile>[^<]*\.(dll|DLL)</OutputFile>', '' | ||
| $content = $content -replace '<ImportLibrary>[^<]*</ImportLibrary>', '' | ||
|
|
||
| # For stubdata - remove resource-only DLL settings | ||
| if ($FilePath -match "stubdata") { | ||
| $content = $content -replace '<NoEntryPoint>true</NoEntryPoint>', '' | ||
| $content = $content -replace '<TurnOffAssemblyGeneration>true</TurnOffAssemblyGeneration>', '' | ||
| } | ||
|
|
||
| Set-Content $FilePath $content -NoNewline | ||
| Write-Host " Patched: $(Split-Path -Leaf $FilePath)" | ||
| } |
There was a problem hiding this comment.
Remove unused $Configuration parameter.
The $Configuration parameter is defined but never used in the function body. The function unconditionally applies both Release and Debug runtime library transformations regardless of the configuration passed.
🔧 Proposed fix
function Patch-IcuVcxProj {
param(
- [string]$FilePath,
- [string]$Configuration
+ [string]$FilePath
)And update the call site at line 134:
- Patch-IcuVcxProj -FilePath (Join-Path $IcuSourceDir $file) -Configuration $CMAKE_BUILD_TYPE
+ Patch-IcuVcxProj -FilePath (Join-Path $IcuSourceDir $file)🤖 Prompt for AI Agents
In @windows-release.ps1 around lines 61 - 107, Patch-IcuVcxProj currently
declares an unused $Configuration parameter; either remove the parameter from
the function signature and all callers that pass it, or make it actually control
the runtime-library replacements: keep the parameter and replace the
unconditional RuntimeLibrary -replace calls with conditional logic that checks
$Configuration (e.g., if ($Configuration -eq 'Release') apply the MultiThreaded
replacement; elseif ($Configuration -eq 'Debug') apply the MultiThreadedDebug
replacement), leaving other transformations unchanged; finally update the call
site(s) that invoke Patch-IcuVcxProj to either stop passing a configuration (if
you removed the param) or to pass the correct 'Release'/'Debug' string (if you
implemented the conditional behavior).
|
This may have been caused by a newer build being triggered for the same PR. |
|
I want to try this change on GHA but |
|
🔄 Preview build started Release tag: |
|
This may have been caused by a newer build being triggered for the same PR. |
Summary
This PR is the first step toward supporting ARM64 on Windows. It removes the Cygwin dependency from the Windows build pipeline by switching to MSBuild for building ICU.
Why remove Cygwin?
Changes
windows-release.ps1: Replaced Cygwin/autotools-based ICU build with MSBuild
Patch-IcuVcxProjfunction to configure vcxproj files for static library buildsConfigurationTypefromDynamicLibrarytoStaticLibraryRuntimeLibraryfrom/MDto/MT(static CRT linking)U_STATIC_IMPLEMENTATIONpreprocessor definition$Platformparameter to support ARM64 in the futurebuild-reusable.yml: Removed
cygwin/cygwin-install-actionfrom GitHub Actions workflowpreprocess.pl: Added
msysdetection alongsideMSWin32for compatibilityTesting
Verified locally that:
Test plan
🤖 Generated with Claude Code