-
-
Notifications
You must be signed in to change notification settings - Fork 220
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
Build v8 for Apple Silicon #202
Conversation
// #cgo darwin,amd64 LDFLAGS: -L${SRCDIR}/deps/darwin_x86_64 | ||
// #cgo darwin,arm64 LDFLAGS: -L${SRCDIR}/deps/darwin_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.
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.
Thanks for the PR; great that we can cross-compile for Darwin arm64 🎉
How do we run the unit tests on arm64 🤔
I've just ordered a M1 Mac, so will be able to functional test soon.
@@ -1,3 +1,3 @@ | |||
module rogchap.com/v8go | |||
|
|||
go 1.12 | |||
go 1.16 |
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.
Likely a good idea to update to minimum of 1.16; but just making sure this will be ok for others? @dylanahsmith
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 are on 1.17. I think we should support what Go itself supports, so 👍 to this change.
@@ -12,10 +12,10 @@ jobs: | |||
name: Tests on ${{ matrix.go-version }} ${{ matrix.platform }} | |||
strategy: | |||
matrix: |
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.
Are we missing the arch
values for this pipeline?
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 can't test against darwin/arm64
without access to a M1 action runner
Run CGO_ENABLED=1 GOOS=darwin GOARCH=arm64 SDKROOT=$(xcrun --sdk macosx --show-sdk-path) go test -v -coverprofile c.out ./...
6
fork/exec /var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/go-build300881077/b001/v8go.test: bad CPU type in executable
7
FAIL rogchap.com/v8go 0.005s
8
? rogchap.com/v8go/deps/darwin_arm64 [no test files]
9
? rogchap.com/v8go/deps/darwin_x86_64 [no test files]
10
? rogchap.com/v8go/deps/include [no test files]
11
? rogchap.com/v8go/deps/include/cppgc [no test files]
12
? rogchap.com/v8go/deps/include/libplatform [no test files]
13
? rogchap.com/v8go/deps/linux_x86_64 [no test files]
14
? rogchap.com/v8go/deps/windows_x86_64 [no test files]
15
FAIL
Tested with a private repo importing this branch on a M1 Mac.
Sadly we won't be able to run tests against darwin/arm64 without access to a M1 runner. However we can test against |
🎩 with latest changes: https://github.com/epk/v8go/actions/runs/1391408059 |
This reverts commit de01fba.
Co-authored-by: dylanahsmith <dylanahsmith@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## master #202 +/- ##
=======================================
Coverage 95.59% 95.59%
=======================================
Files 16 16
Lines 545 545
=======================================
Hits 521 521
Misses 15 15
Partials 9 9 Continue to review full report at Codecov.
|
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.
go-version: [1.12.17, 1.16.4] | ||
platform: [ubuntu-latest, macos-latest, windows-latest] | ||
go-version: [1.16.8, 1.17.1] |
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.
Github is showing "Some checks haven’t completed yet", but then lists the status for Go versions that were updated here to no longer run:
Tests on 1.12.17 ubuntu-latest Expected — Waiting for status to be reported (Required)
Tests on 1.12.17 windows-latest Expected — Waiting for status to be reported (Required)
Tests on 1.16.4 macos-latest Expected — Waiting for status to be reported (Required)
Tests on 1.16.4 ubuntu-latest Expected — Waiting for status to be reported (Required)
Tests on 1.16.4 windows-latest Expected — Waiting for status to be reported (Required)
This is blocking me from merging. @rogchap you could do the merge to workaround this.
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.
Oh, it looks like the github branch protection rules for the repo need to be updated, since the name of these CI jobs has changed.
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 can fix this.
When building v8 on macOS, v8 tooling falls back to Xcode to provide the build toolchain, which supports cross compilation between amd64 and arm64. This PR adds a
darwin_arm64
v8 build and changes the CI workflow to automatically build fordarwin_arm64
lipo:
Build Logs from my intel Mac building the arm target:
To build an arm64 binary importing this module from an intel Mac:
See golang/go#44112 for details
closes #171 #54