Skip to content
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

ci: add arm64 build #94

Merged
merged 35 commits into from
Dec 6, 2023
Merged

ci: add arm64 build #94

merged 35 commits into from
Dec 6, 2023

Conversation

supervacuus
Copy link
Collaborator

No description provided.

@supervacuus
Copy link
Collaborator Author

@Swatinem and @compnerd: I've added a Windows ARM64 build here so that we can at least make sure that a cross-compilation using cmake + msbuild works and we don't break that in future updates.

In my tests, the CMAKE_SYSTEM_PROCESSOR always reflects the host processor, even if I enable the cross-compile VS env-vars. Changing those to CMAKE_GENERATOR_PLATFORM seems to do the trick and should also work non-cross ARM64 builds since the system-processor is what the generator-platform defaults to anyway.

I will clean this up tomorrow, but maybe you can have a look if that breaks in your setup @compnerd? Thx!

@supervacuus
Copy link
Collaborator Author

We also need to make sure that is documented somewhere, because this ARM64 build on Windows requires CMake 3.26 because of the ASM_MARMASM language. We can also think about increasing the cmake_minimum_required (currently 3.12).

@compnerd
Copy link

@supervacuus CMAKE_SYSTEM_PROCESSOR is the host, CMAKE_HOST_SYSTEM_PROCESSOR is the build. https://cmake.org/cmake/help/latest/variable/CMAKE_HOST_SYSTEM_PROCESSOR.html

@compnerd
Copy link

I can test the CMAKE_GENERATOR_PLATFORM change, however, that is not really ideal as that means that cross-compiling with non-multi-generator targets will not work (e.g. clang+ninja). As it so happens we are currently using MSBuild, so we can get away with it, but just trying to future proof the work.

@compnerd
Copy link

One final thing - this won't work. You will miscompile flatc if you do this. I already did the CI work for this - https://github.com/thebrowsercompany/sentry-native/blob/master/.github/workflows/bcny-ci.yml. I would be more than happy to see that reside here (with the minor request that we could also add binary releases).

Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

lgtm, but I’m completely at a loss when it comes to CMake.

.github/workflows/build.yml Outdated Show resolved Hide resolved
@supervacuus
Copy link
Collaborator Author

I might need to communicate my intention with this PR. Also, I got sick and will continue with this next week.

@supervacuus CMAKE_SYSTEM_PROCESSOR is the host, CMAKE_HOST_SYSTEM_PROCESSOR is the build. https://cmake.org/cmake/help/latest/variable/CMAKE_HOST_SYSTEM_PROCESSOR.html

If you do a cross-compilation, CMAKE_SYSTEM_PROCESSOR should represent the target architecture, while CMAKE_HOST_SYSTEM_PROCESSOR is the host of the build doing the cross-compilation. But when using the MSBuild generator, the architecture (-A) generator parameter doesn't automatically set CMAKE_SYSTEM_PROCESSOR according to that rule because, at the time it is determined, the target platform of the toolchain is not known (this is an age-old issue in CMake).

I can test the CMAKE_GENERATOR_PLATFORM change, however, that is not really ideal as that means that cross-compiling with non-multi-generator targets will not work (e.g. clang+ninja). As it so happens we are currently using MSBuild, so we can get away with it, but just trying to future proof the work.

I agree, so I would only use the generator platform for MSBuild. I am not proposing to replace CMAKE_SYSTEM_PROCESSOR but rather ignore toolchain parameters when the MSBuild architecture was specified because that prevents the use of conflicting arguments. This is what I meant by cleaning up.

One final thing - this won't work. You will miscompile flatc if you do this.

Can you elaborate?

I already did the CI work for this - https://github.com/thebrowsercompany/sentry-native/blob/master/.github/workflows/bcny-ci.yml.

Passing toolchain parameters by hand is fine if you want to do this in your build. Still, if we introduce this in the Native SDK, I would like to limit the chance of misconfiguration, and the CMake build script should do the right thing (rather than expect it from the user at the command line). I am okay with introducing a toolchain file, though.

Btw in your Stage Sentry you can simply invoke

cmake --install out --config RelWithDebInfo --prefix <your-stage-prefix>

You'd only need to move your lib and bin files to win64 instead of tracking each artifact.

I would be more than happy to see that reside here (with the minor request that we could also add binary releases).

Providing Windows binaries would make a lot of sense. But I don't want to release them as long as we do not have an ARM64 Windows runner for our tests.

@compnerd
Copy link

One final thing - this won't work. You will miscompile flatc if you do this.
Can you elaborate?

Sure, what I expect to happen is that you would build flatc as an ARM64 binary. Since GHA does not have a Windows ARM64 host yet, it would be that you are cross-compiling from AMD64. The ARM64 binary cannot run on AMD64 and thus would fail. I am categorising this as "miscompiled" as you built it for the host rather than build architecture.

@supervacuus supervacuus marked this pull request as ready for review December 6, 2023 13:57
@supervacuus supervacuus requested a review from a team December 6, 2023 13:57
@supervacuus supervacuus merged commit 89991e9 into getsentry Dec 6, 2023
4 checks passed
@supervacuus supervacuus deleted the ci/arm64_build branch December 6, 2023 13:57
@supervacuus
Copy link
Collaborator Author

supervacuus commented Dec 6, 2023

Sure, what I expect to happen is that you would build flatc as an ARM64 binary. Since GHA does not have a Windows ARM64 host yet, it would be that you are cross-compiling from AMD64. The ARM64 binary cannot run on AMD64 and thus would fail. I am categorising this as "miscompiled" as you built it for the host rather than build architecture.

Please excuse my ignorance, but where is flatc built or executed in this build configuration?

Again, to be clear, the function of the build in the crashpad fork ci is that we have an early warning setup that checks whether the build works correctly across supported platforms before we update sentry-native (either because we update from upstream or due to any local changes we do).

Testing - or executing any build artifacts as part of the build - currently only happens in sentry-native. We also don't (re-)use the build artifacts downstream because there are too many combinations (not only of CLI params but also because some downstream SDKs require specific compiler + standard library setups, which would complicate the build here unnecessarily).

@compnerd
Copy link

compnerd commented Dec 6, 2023

Please excuse my ignorance, but where is flatc built or executed in this build configuration?

Ah that's me mixing up recent work. Sort about the confusion!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants