-
Notifications
You must be signed in to change notification settings - Fork 53
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
Add the native ARM64 toolchain build to the main CI #847
base: main
Are you sure you want to change the base?
Conversation
path: ${{ github.workspace }}/tmp/amd64 | ||
|
||
- uses: actions/download-artifact@v4 | ||
with: | ||
name: installer-arm64 | ||
name: installer-arm64-built-on-amd64 |
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.
We shouldn't be changing the name of the artifact, the native vs cross should generate the same content.
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.
The problem is that the upload-artifact fails and causes the entire job to fail if something with the same name has already been uploaded and the order between x64/arm64, which depends on the build progress speed, is unpredictable. Can you think of a better workaround?
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.
That shouldn't matter - we can download only the native builds (i.e. the amd64 installer from x64, the arm64 installer from arm64).
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.
Wdym? The issue is that an artifact name conflict will cause an error in the CI run. The way it's set up in this PR, both amd64/arm64 release installers come from the amd64 build and the arm64 build is only building to check the buildability.
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; I'm saying we change that. Only the AMD64 build comes from the AMD64 build and only the ARM64 build comes from the ARM64 build.
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 think the main issue is that you are building the same artifacts with both an arm64 and an x64 host (see my other comment). Basically, only one host should be building for one given target. Anything more is redundant.
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.
As above and what compnerd and I discussed in other places, the premise is that we want to keep three builds: x64-built-on-x64, arm64-built-on-x64, arm64-built-on-arm64. They are redundant from the point of view of producing release binaries but they are not from the point of view of ensuring the three builds work.
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.
If we want to keep maintaining the arm64-built-on-x64 build then yes, we don't have a choice, we need the ability to differentiate between the 2 builds or we are going to hit a conflict with the artifact name.
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.
Gotcha
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.
@compnerd Are you okay with this suffix approach?
@@ -662,17 +827,17 @@ jobs: | |||
|
|||
release: | |||
runs-on: ubuntu-latest | |||
needs: [context, windows-build] | |||
needs: [context, windows-build-x64] |
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.
This should be dependent on both of the architecture builds.
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.
Done. windows-build-arm64
currently fails. Do you know of a way to make it continue on an arm64 failure?
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.
You can mark a job as being ignored IIRC, I don't remember the syntax off hand, but the GHA documentation includes it.
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.
Added continue-on-error: true
to windows-build
.
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.
Used if success() || failure()
on release
instead of continue-on-error
as it didn't work.
01d7d08
to
4e8d8c8
Compare
@compnerd PTAL |
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 think that the native only build usage would greatly simplify this change.
path: ${{ github.workspace }}/tmp/amd64 | ||
|
||
- uses: actions/download-artifact@v4 | ||
with: | ||
name: installer-arm64 | ||
name: installer-arm64-built-on-amd64 |
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.
That shouldn't matter - we can download only the native builds (i.e. the amd64 installer from x64, the arm64 installer from arm64).
What do you mean by "the native only build usage" exactly? |
path: ${{ github.workspace }}/tmp/amd64 | ||
|
||
- uses: actions/download-artifact@v4 | ||
with: | ||
name: installer-arm64 | ||
name: installer-arm64-built-on-amd64 |
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 think the main issue is that you are building the same artifacts with both an arm64 and an x64 host (see my other comment). Basically, only one host should be building for one given target. Anything more is redundant.
4e8d8c8
to
096bf82
Compare
release
job depends only onwindows-build-x64
.