-
Notifications
You must be signed in to change notification settings - Fork 13
build-and-deploy: add support for arm64 builds #4
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
Conversation
@@ -37,7 +37,7 @@ env: | |||
|
|||
jobs: | |||
build: | |||
runs-on: windows-latest | |||
runs-on: ${{ github.event.inputs.architecture == 'aarch64' && fromJSON('["Windows", "ARM64"]') || 'windows-latest' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't use env.ARCHITECTURE
here so had to resort to github.event.inputs.architecture
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, the environment requires the runner to already run, and that requires runs-on
to be specified ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! I have just one minor request (dropping some unnecessary parentheses) and will be happy to merge this very soon.
@@ -135,6 +135,8 @@ jobs: | |||
flavor: ${{ env.PACKAGE_TO_BUILD == 'mingw-w64-git' && 'build-installers' || 'full' }} | |||
architecture: ${{ env.ARCHITECTURE || 'x86_64' }} | |||
msys: ${{ env.REPO == 'MSYS2-packages' || env.PACKAGE_TO_BUILD == 'git-for-windows-keyring' }} | |||
# We only have to clean up on self-hosted runners | |||
cleanup: ${{ (runner.arch == 'ARM64' && true) || false }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a look at https://docs.github.com/en/actions/learn-github-actions/contexts#runner-context to see whether there would be any indication of a self-hosted runner, but did not find any. Pity.
If only we could detect whether we're running on a self-hosted, non-ephemeral runner, and set cleanup: true
only then...
Would you mind losing the parentheses? We use the idiom 'A && B || C' (where &&
/||
follow C++'s operator precedence order) quite often, without parentheses, so that it looks inconsistent when we use them here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind losing the parentheses?
Done ✅ thanks for sharing the link to that documentation. I guess I got used to the no-mixed-operators
rule in ESLint, which encourages using the parentheses to "clarfiy the developer’s intention" 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that's where that comes from. Yes, I am an unfan of that rule ;-)
@@ -37,7 +37,7 @@ env: | |||
|
|||
jobs: | |||
build: | |||
runs-on: windows-latest | |||
runs-on: ${{ github.event.inputs.architecture == 'aarch64' && fromJSON('["Windows", "ARM64"]') || 'windows-latest' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, the environment requires the runner to already run, and that requires runs-on
to be specified ;-)
9892783
to
fca9b6c
Compare
Note that this only works for self-hosted runners. GitHub Actions will pick up and use the first available arm64 runner. Signed-off-by: Dennis Ameling <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
fca9b6c
to
119a90a
Compare
@@ -209,8 +211,10 @@ jobs: | |||
printf '#!/bin/sh\n\nexec /mingw64/bin/git.exe "$@"\n' >/usr/bin/git | |||
} && | |||
|
|||
MINGW_ARCHS_TO_BUILD=$(test "$ARCHITECTURE" == "aarch64" && echo "clangarm64" || echo "mingw32 mingw64") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dennisameling I hope you don't mind that I edited this: We can either use a Bash-style ternary (with $ARCHITECTURE
) or use GitHub workflow syntax (${{ env.ARCHITECTURE == 'aarch64' && 'clangarm64' || 'mingw32 mingw64' }}
), but we should not mix them. I opted for the former.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me! 👍🏼
Yay!!!! So exciting indeed! 🚀 |
Ah... It failed because |
What did not work was the GPG step. I will have to leave it at that, for now. |
Oh my 😅 that commit history looks really similar to my mess from testing yesterday haha. Thank you so much for going through that! I know it can be a total pain! Let's say if I had some time next week and wanted to kick off the build-and-deploy pipeline for arm64 to build some of the packages (
WDYT? |
The fruit of all of your labor: https://wingit.blob.core.windows.net/aarch64/git-for-windows-aarch64.db |
Note that this only works for self-hosted runners. GitHub Actions will pick up and use the first available arm64 runner.
Here's a successful run for
mingw-w64-openssl
onaarch64
: https://github.com/dennisameling/git-for-windows-automation/actions/runs/3735260060/jobs/6338362688. Note that I had to do some manual fixes as the pipeline assumes it has a GitHub app (GH_APP_ID
andGH_APP_PRIVATE_KEY
) at its disposal. In my run, I simply replaced that with a PAT by doingconst accessToken = ${{ secrets.DENNIS_PAT }}