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

Add arm64 support to CMake build #64

Merged
merged 1 commit into from
Jul 30, 2021
Merged

Conversation

neonichu
Copy link
Member

This is pretty speculative, I just copied the SwiftSupport.cmake from swift-argument-parser which seems to have support for arm64.

@neonichu
Copy link
Member Author

Testing this out first via cross-repo testing on a Swift PR.

@tomerd
Copy link
Member

tomerd commented Jul 20, 2021

cc @lorentey @compnerd @shahmishal

if this works I think we would also need a new tag and update swift build script to pull it

@tomerd
Copy link
Member

tomerd commented Jul 23, 2021

@neonichu @lorentey @compnerd we need this (+ new tag) to unblock swiftlang/swift-package-manager#3632. do you think this is ready?

@lorentey
Copy link
Member

I'm ready to hit the merge button & tag a release if y'all say it works as expected.

@swift-ci test

@compnerd
Copy link
Contributor

I think that this is slightly regressing the behavior. I think that there are now out of sync (foundation, dispatch, etc have a copy as well). The install layout has evolved a bit from the 5.1/5.2 days. All the platforms now support the directory layout as well. Windows prefers that layout with the module being triple named rather than just arch I believe.

@neonichu
Copy link
Member Author

neonichu commented Jul 23, 2021

@compnerd Do you have a suggestion on which version of this file we should standardize? Is Foundation the canonical one?

@compnerd
Copy link
Contributor

The changes that I believe haven't been synced are the ones to _install_target here:
https://github.com/compnerd/swift-win32/blob/main/cmake/Modules/SwiftSupport.cmake#L53-L92

The changes that you have to the get_swift_host_arch we can get away with as we do not expect someone to define CMAKE_OSX_DEPLOYMENT_TARGET on non-Darwin targets, but this seems somewhat fragile. I think that this might be a better check:

elseif ("${CMAKE_SYSTEM_PROCESSOR}" MATCHES "AArch64|ARM64")
  if(CMAKE_SYSTEM_NAME MATCHES Darwin)
    set("${result_var_name}" "arm64" PARENT_SCOPE)
  else()
    set("${result_var_name}" "aarch64" PARENT_SCOPE)
  endif()

@neonichu neonichu force-pushed the arm64-cmake branch 2 times, most recently from c58270c to 5881b4b Compare July 23, 2021 21:23
@neonichu
Copy link
Member Author

OK, thanks. I updated it, but it seems like we do need the lowercase "arm64" in the check of CMAKE_SYSTEM_PROCESSOR, otherwise this doesn't work on my M1 machine.

@compnerd
Copy link
Contributor

Hmm, I wonder if we should do string(TOUPPER or string(TOLOWER to make sure that we do the case insensitive match. That is rather annoying, but we shouldn't be gating this on that.

@compnerd
Copy link
Contributor

I think that you missed the update for the second half of the change (the install location).

@neonichu
Copy link
Member Author

@swift-ci please test

@neonichu
Copy link
Member Author

OK, I removed the unnecessary changes and tested this locally, seems to work fine.

@neonichu neonichu changed the title [DNM] Add arm64 support to CMake build Add arm64 support to CMake build Jul 23, 2021
@compnerd
Copy link
Contributor

LGTM; we should verify that the Linux ARM64 builds are unimpacted, but otherwise, LGTM

@neonichu
Copy link
Member Author

Actually, I think I flipped it with my last edits, it'll now use "aarch64" on Darwin.

This is pretty speculative, I just copied the `SwiftSupport.cmake` from swift-argument-parser which seems to have support for arm64.
@tomerd
Copy link
Member

tomerd commented Jul 27, 2021

@compnerd @lorentey how are we feeling about this?

@lorentey
Copy link
Member

I'm happy to merge & tag as soon as someone confirms this is what we want. 😉

@neonichu, did you mean to swap the spellings? IIUC Swift Argument Parser uses arm64 on Darwin, and aarch64 elsewhere.

@neonichu
Copy link
Member Author

@swift-ci please test

@neonichu
Copy link
Member Author

@lorentey Yes, absolutely. I failed to push my latest changes. Interestingly, it seemed to have no impact on being able to build, either version worked.

@lorentey lorentey merged commit 0959ba7 into apple:main Jul 30, 2021
@neonichu neonichu deleted the arm64-cmake branch July 30, 2021 17:00
@tomerd
Copy link
Member

tomerd commented Jul 30, 2021

@lorentey I guess we need a new tag, so we can pull this into toolchain + SwiftPM. Do you have any other pending changes?

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