Add MacOS Arm64 support #131
Conversation
There was a problem hiding this comment.
Overall this looks good, just a few tweaks needed.
Also, I see that you opened the PR against the master branch of your fork. Would it be possible to create a branch and open a new PR from that instead?
That will also give you a chance to remove/clean up the unneeded intermediate commits you've got in here.
| @@ -35,6 +35,8 @@ jobs: | |||
| name: linux-arm | |||
| - os: macos-10.15 | |||
| name: osx-x64 | |||
| - os: macos-11 | |||
There was a problem hiding this comment.
Unless there's a reason why we have to move to macos-11 for this, I'd prefer to stick to macos-10.15 for now. .NET Core 3.1 and .NET 6 both support older versions, so I think it makes sense to compile on the oldest version GitHub Actions has available.
There was a problem hiding this comment.
Afaik, you need Big Sur to target ARM (Xcode 13 depends on Big Sur).
There was a problem hiding this comment.
Yeah, you need Big Sur to compile it for ARM64, since that's the first platform that really supports it anyway.
| @@ -124,6 +124,7 @@ Push-Location $libgit2Directory | |||
| <dllmap os="linux" cpu="arm" wordsize="32" dll="$binaryFilename" target="lib/linux-arm/lib$binaryFilename.so" /> | |||
| <dllmap os="linux" cpu="armv8" wordsize="64" dll="$binaryFilename" target="lib/linux-arm64/lib$binaryFilename.so" /> | |||
| <dllmap os="osx" cpu="x86-64" wordsize="64" dll="$binaryFilename" target="lib/osx-x64/lib$binaryFilename.dylib" /> | |||
| <dllmap os="osx" cpu="arm64" wordsize="64" dll="$binaryFilename" target="lib/osx-arm64/lib$binaryFilename.dylib" /> | |||
There was a problem hiding this comment.
This file is for mono support, so I believe it needs to be:
| <dllmap os="osx" cpu="arm64" wordsize="64" dll="$binaryFilename" target="lib/osx-arm64/lib$binaryFilename.dylib" /> | |
| <dllmap os="osx" cpu="armv8" wordsize="64" dll="$binaryFilename" target="lib/osx-arm64/lib$binaryFilename.dylib" /> |
| @@ -3,4 +3,5 @@ | |||
| <dllmap os="linux" cpu="arm" wordsize="32" dll="git2-b7bad55" target="lib/linux-arm/libgit2-b7bad55.so" /> | |||
| <dllmap os="linux" cpu="armv8" wordsize="64" dll="git2-b7bad55" target="lib/linux-arm64/libgit2-b7bad55.so" /> | |||
| <dllmap os="osx" cpu="x86-64" wordsize="64" dll="git2-b7bad55" target="lib/osx-x64/libgit2-b7bad55.dylib" /> | |||
| <dllmap os="osx" cpu="arm64" wordsize="64" dll="git2-b7bad55" target="lib/osx-arm64/libgit2-b7bad55.dylib" /> | |||
There was a problem hiding this comment.
| <dllmap os="osx" cpu="arm64" wordsize="64" dll="git2-b7bad55" target="lib/osx-arm64/libgit2-b7bad55.dylib" /> | |
| <dllmap os="osx" cpu="armv8" wordsize="64" dll="git2-b7bad55" target="lib/osx-arm64/libgit2-b7bad55.dylib" /> |
| @@ -7,9 +7,13 @@ SHORTSHA=${LIBGIT2SHA:0:7} | |||
| OS=`uname` | |||
| ARCH=`uname -m` | |||
| PACKAGEPATH="nuget.package/runtimes" | |||
| DCMAKEOSXARCHITECTURES="x86_64" | |||
There was a problem hiding this comment.
Let's name this OSXARCHITECTURE instead.
| @@ -25,15 +29,18 @@ if [[ $RID == *arm ]]; then | |||
| fi | |||
|
|
|||
| if [[ $RID == *arm64 ]]; then | |||
| export TOOLCHAIN_FILE=/nativebinaries/CMakeLists.arm64.txt | |||
| DCMAKEOSXARCHITECTURES="arm64" | |||
There was a problem hiding this comment.
Something seems off about this change. Setting DCMAKEOSXARCHITECTURES again seems redundant. We should be able to remove this and just keep the condition you added to make sure it's not set when on macOS.
| @@ -60,4 +67,4 @@ fi | |||
| rm -rf $PACKAGEPATH/$RID | |||
| mkdir -p $PACKAGEPATH/$RID/native | |||
|
|
|||
| cp libgit2/build/libgit2-$SHORTSHA.$LIBEXT $PACKAGEPATH/$RID/native | |||
| cp libgit2/build/libgit2-$SHORTSHA.$LIBEXT $PACKAGEPATH/$RID/native | |||
There was a problem hiding this comment.
Nitpick, but let's add the newline back to the file.
|
Unnecessary commits doesn't really matter when they get squashed anyway, but I can make a new one. |
|
New PR #132 |
This edits the scripts and yaml to include a
osx-arm64target. This should add the correct dylib to the Nuget package to be consumed by the main libgit2 library.Fixes #129