fix: windows CI by adding cross-platform Makefile support (#3109)#7635
fix: windows CI by adding cross-platform Makefile support (#3109)#7635swar09 wants to merge 16 commits intounicode-org:mainfrom
Conversation
… separators and binary extensions to Makefiles and test scripts.
…EP)` variable with literal slashes.
|
|
|
Good work! I noticed that "Install harfbuzz (Windows)" takes 7m 10s. Can we use a faster installation process for harfbuzz and/or cache the installation so that we don't need to wait for it on every PR moving forward? |
robertbastian
left a comment
robertbastian
left a comment
There was a problem hiding this comment.
it's nice to get this working on windows, but I don't think we should spend CI resources on it
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| os: [ ubuntu-latest, windows-latest ] |
There was a problem hiding this comment.
I don't think the cost of an additional runner is worth it. the linux job is barely worth it, because we never touch the code that this tests
There was a problem hiding this comment.
Yeah I'm on the fence. wdyt about running it only on Windows? Then at least the job is covering something that most of the other jobs don't cover.
There was a problem hiding this comment.
I'm also fine with having Windows makefile support for harfbuzz being best-effort: we don't CI it but we try to maintain it when people ask us to fix things.
There was a problem hiding this comment.
Having it in .github/workflows means that it remains reproducible, which is nice.
Wdyt about making it a trigger-only job (requires pressing a button in the Actions tab) that we run periodically such as when preparing a release (except we don't want to make releases harder)
There was a problem hiding this comment.
Wdyt about making it a trigger-only job (requires pressing a button in the Actions tab) that we run periodically such as when preparing a release (except we don't want to make releases harder)
then you might as well run it locally. all that the CI would test is that harfbuzz is installed correctly – this is not something we need to test because it's outside of ICU4X.
There was a problem hiding this comment.
I think I still have a slight preference to enforce Windows client builds in CI (not just tests).
I don't care much about harfbuzz in particular as a CI thing; I would be happy if we built the cargo example crates without the harfbuzz one.
There was a problem hiding this comment.
then lets run test-c on windows (we already run test-dart). keep in mind that windows CI takes 2x longer than ubuntu
|
Thanks for the feedback! I see the concern about CI . Would you prefer if I
|
I don't think the maintainers have decided yet how we want to handle the CI, but if you want to split the Makefile changes into their own PR, I think that would be landable. |
There was a problem hiding this comment.
Pull request overview
This PR fixes Windows CI compatibility by adding cross-platform support to Makefiles and build scripts. It addresses issue #3109 where the test-cargo CI job was failing on Windows due to missing .exe extensions on binary names and incorrect path quoting. The changes enable the test-cargo workflow to run on both Ubuntu and Windows.
Changes:
- Added Windows to the test-cargo CI job matrix and configured harfbuzz installation for both Linux (apt) and Windows (vcpkg)
- Implemented binary extension detection in Makefiles using
$(OS)environment variable check - Updated DuckScript test automation to detect Windows and apply
.exeextension to binary references - Fixed file path quoting to properly handle spaces in Windows paths
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
.github/workflows/build-test.yml |
Adds Windows to test-cargo job matrix with platform-specific harfbuzz installation |
tools/make/tests.toml |
Adds Windows detection and binary extension logic to DuckScript test task |
examples/cargo/buffer/Makefile |
Adds binary extension detection and applies to all binary references and paths |
examples/cargo/custom_compiled/Makefile |
Adds binary extension detection and applies to icu4x-datagen references |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@swar09 Please resolve conflicts with the main branch. Thanks. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
4 participants
Fix Windows CI by adding cross-platform Makefile support
tools/make/tests.tomlFixes #3109