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

no longer build arm64 go builds #3838

Merged
merged 1 commit into from
Feb 21, 2023
Merged

no longer build arm64 go builds #3838

merged 1 commit into from
Feb 21, 2023

Conversation

arlyon
Copy link
Contributor

@arlyon arlyon commented Feb 16, 2023

🚨 note: I am assuming here that npm is intelligent enough to offer up the x86 package automatically, but there may be more we need to do here..

Rust's only toolchain on windows arm is msvc, which complicates linking now that we have the rust sandwich, as go uses gnu. Rather than attempt to get go building with msvc, we can instead just rely on windows' x86 emulation and publish one set of binaries for that platform until go is removed from our toolchain altogether.

@arlyon arlyon requested a review from a team as a code owner February 16, 2023 11:07
@arlyon arlyon force-pushed the arlyon/windows-arm branch from f8ac672 to 0162be7 Compare February 16, 2023 11:08
@arlyon arlyon requested a review from a team as a code owner February 16, 2023 11:08
@vercel
Copy link

vercel bot commented Feb 16, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
examples-kitchensink-blog 🔄 Building (Inspect) Feb 21, 2023 at 10:13PM (UTC)
turbo-site ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 21, 2023 at 10:13PM (UTC)
8 Ignored Deployments
Name Status Preview Comments Updated
examples-basic-web ⬜️ Ignored (Inspect) Feb 21, 2023 at 10:13PM (UTC)
examples-cra-web ⬜️ Ignored (Inspect) Feb 21, 2023 at 10:13PM (UTC)
examples-designsystem-docs ⬜️ Ignored (Inspect) Feb 21, 2023 at 10:13PM (UTC)
examples-native-web ⬜️ Ignored (Inspect) Feb 21, 2023 at 10:13PM (UTC)
examples-nonmonorepo ⬜️ Ignored (Inspect) Feb 21, 2023 at 10:13PM (UTC)
examples-svelte-web ⬜️ Ignored (Inspect) Feb 21, 2023 at 10:13PM (UTC)
examples-tailwind-web ⬜️ Ignored (Inspect) Feb 21, 2023 at 10:13PM (UTC)
examples-vite-web ⬜️ Ignored (Inspect) Feb 21, 2023 at 10:13PM (UTC)

@github-actions
Copy link
Contributor

github-actions bot commented Feb 16, 2023

🟢 CI successful 🟢

Thanks

@gsoltis gsoltis requested review from gsoltis and a team February 16, 2023 17:09
Copy link
Contributor

@mehulkar mehulkar left a comment

Choose a reason for hiding this comment

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

I don't have any experience in this area, but if it works, that seems good to me.

It looks like it's our postinstall script that chooses the right binary to download though. Maybe edit this to always serve the x86 build?
https://github.com/vercel/turbo/blob/564a0f0dab223fbaf2ad4a7a3a5332aa50e27eb0/packages/turbo/node-platform.js#L32-L36

Copy link
Member

@chris-olszewski chris-olszewski left a comment

Choose a reason for hiding this comment

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

I'm almost positive we need to change our postinstall logic that resides in packages/turbo/node-platform.js to now look for the turbo-windows-64 when on arm64 instead of turbo-windows-arm64 like it does now.

We should also confirm that goreleaser release --rm-dist -f combined-shim.yml --skip-publish produces a turbo-windows-64 where "cpu": ["x64", "arm64"]

@@ -144,9 +144,6 @@ jobs:
mv go-artifacts/turbo-go-cross-${{ inputs.release_branch }}/turbo_windows_amd64_v1/bin/* cli/dist-windows-amd64
chmod a+x cli/dist-windows-amd64/turbo.exe
chmod a+x cli/dist-windows-amd64/go-turbo.exe
mv go-artifacts/turbo-go-cross-${{ inputs.release_branch }}/turbo_windows_arm64/bin/* cli/dist-windows-arm64
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to continue shipping a windows/arm package, since we'll eventually be building it natively in Rust. So I think maybe this ought to be
mv go-artifacts/turbo-go-cross-${{ inputs.release_branch }}/turbo_windows_amd64_v1/bin/* cli/dist-windows-arm64

And keep the following lines?

@@ -16,6 +16,11 @@ builds:
- arm64
goamd64:
- v1
ignore:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we still want to build the windows/arm package, it just has x86 binaries in it.

@@ -22,7 +22,6 @@ builds:
targets:
- linux_arm64
- linux_amd64
- windows_arm64
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can delete the overrides entry below as well.

@gsoltis gsoltis added the pr: automerge Kodiak will merge these automatically after checks pass label Feb 21, 2023
@kodiakhq kodiakhq bot merged commit cda81f6 into main Feb 21, 2023
@kodiakhq kodiakhq bot deleted the arlyon/windows-arm branch February 21, 2023 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: ci pr: automerge Kodiak will merge these automatically after checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants