Skip to content

Add Windows 11 arm to ci #7517

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

mcbarton
Copy link
Contributor

According to https://github.blog/changelog/2025-04-14-windows-arm64-hosted-runners-now-available-in-public-preview/ Windows arm Github runners are now available for free for public repositories. Therefore I have added them to binaryen ci.

@@ -368,8 +368,11 @@ jobs:

# Windows + gcc needs work before the tests will run, so just test the compile
build-mingw:
name: mingw
runs-on: windows-latest
name: mingw-${{ matrix.os }}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't know whether windows arm was needed for this part of ci. Added for now, and will remove if requested.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to keep.

@mcbarton
Copy link
Contributor Author

mcbarton commented Apr 17, 2025

As I modify the create_release workflow as part of this PR I did a test release on my fork here https://github.com/mcbarton/binaryen/releases/tag/test-windows-arm . Once this workflow run has finished running https://github.com/mcbarton/binaryen/actions/runs/14522766598 the artifacts should appear there.

@mcbarton
Copy link
Contributor Author

mcbarton commented Apr 17, 2025

Something went wrong with the test release. The Windows arm binary ended up with x86_64 in the name, and it shouldn't have 11 in the name. I will fix tomorrow.

@mcbarton
Copy link
Contributor Author

@kripken @tlively This PR is ready for review, as my last test release (https://github.com/mcbarton/binaryen/releases/tag/test_release_8) appears to have worked. There is now a Windows arm release binary, as well as a Windows x86_64 one.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems potentially useful, thanks, but I have to admit I don't know how important ARM is for the Windows platform (mainly since I don't know much about Windows these days). Do you have a need for it yourself, or have you seen someone asking for these builds?

@mcbarton
Copy link
Contributor Author

This seems potentially useful, thanks, but I have to admit I don't know how important ARM is for the Windows platform (mainly since I don't know much about Windows these days). Do you have a need for it yourself, or have you seen someone asking for these builds?

I don't have any need for it myself, but someone was asking for Windows arm support for wasi-sdk here WebAssembly/wasi-sdk#522 , and I thought it shouldn't be too difficult to add once Githubs Windows arm runners became available.

I'm trying to add Windows arm support to all the WebAssembly repos. I made a PR for Windows arm support in wasi-sdk here WebAssembly/wasi-sdk#524 , and for the wasi-testsuite here WebAssembly/wasi-testsuite#105 (just waiting for someone to activate the workflow on this one) . I also plan to move onto adding it to the wasi-libc repo next assuming its something the organisation wants/sees a need for.

@@ -368,8 +368,11 @@ jobs:

# Windows + gcc needs work before the tests will run, so just test the compile
build-mingw:
name: mingw
runs-on: windows-latest
name: mingw-${{ matrix.os }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to keep.

@@ -47,6 +47,11 @@ jobs:
run: cmake -S . -B out -DCMAKE_INSTALL_PREFIX=out/install
if: matrix.os == 'windows-latest'

- name: cmake (win arm64)
# -G "Visual Studio 15 2017"
run: cmake -S . -B out -DCMAKE_INSTALL_PREFIX=out-arm64/install
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need a different installation directory here? If we don't, can we just reuse the previous step for both windows builders?

Copy link
Contributor Author

@mcbarton mcbarton Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need a different installation directory due that being the assumption of the folder name in the archive-arm64 stage of the ci already in place (see https://github.com/mcbarton/binaryen/blob/6658bcc05d2c695a9fc9b5dae8ed623a9dfae9ef/.github/workflows/create_release.yml#L92 ) . The reason its done in this way currently is because you create osx x86 and osx arm64 binaries both from the osx arm64 runners as far as I can tell https://github.com/mcbarton/binaryen/blob/6658bcc05d2c695a9fc9b5dae8ed623a9dfae9ef/.github/workflows/create_release.yml#L41

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks 👍

runs-on: ${{ matrix.os }}
strategy:
matrix:
os: [windows-latest,windows-11-arm]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space after comma here and below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -56,7 +61,7 @@ jobs:

- name: strip
run: find out*/install/ -type f -perm -u=x -exec strip -x {} +
if: matrix.os != 'windows-latest'
if: ${{ !startsWith(matrix.os, 'windows') }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is ${{ .. }} needed here but not elsewhere where startsWith i used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was due to me not using startswith much. I didn't know whether I could do the negation of startswith without the ${{ .. }} . Will change to just !startsWith(matrix.os, 'windows')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to revert this change back to what it was previously, as the workflow broke without the brackets

@mcbarton mcbarton requested a review from sbc100 April 23, 2025 19:11
@mcbarton mcbarton requested a review from kripken April 25, 2025 08:43
@kripken
Copy link
Member

kripken commented Apr 28, 2025

I am not opposed to landing this. Leaving to @tlively and @sbc100 to decide.

@tlively
Copy link
Member

tlively commented Apr 29, 2025

@sbc100, any final comments?

Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

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