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

fix: Allow x64 arch on Mac Rosetta #32266

Merged
merged 1 commit into from Jan 7, 2022
Merged

fix: Allow x64 arch on Mac Rosetta #32266

merged 1 commit into from Jan 7, 2022

Conversation

tmacwill
Copy link
Contributor

@tmacwill tmacwill commented Dec 21, 2021

Description of Change

Closes #32147.

If the npm_config_arch environment variable is set on Mac, then use the specified architecture rather than overriding it to x64.

Currently, npm/install.js overrides the architecture specified in npm_config_arch to be arm64 when running on Mac Rosetta. However, this can break workflows that rely on specific architectures for compatibility with native addons. Some native addons do not yet provide prebuilds for Mac arm64, requiring the use of Electron x64. By installing native addons in a Rosetta environment, the correct x64 prebuilds will be used for native addons, but npm/install.js as written will force an install of arm64 Electron, which is incompatible with the x64 addons.

Additionally, it's quite confusing that https://www.electronjs.org/docs/latest/tutorial/installation states the npm_config_arch flag can be used to specify an arch, but its value is ignored in Rosetta environments. It seems like the value of this flag should be respected if set, and a best guess can be made when it's not set. An alternative approach here would be to allow setting a third environment variable to prevent the override, but that seemed more confusing than respecting the value of the existing variable.

If I've misunderstood anything above, do let me know!

Checklist

Release Notes

Notes: Allowed specifying x64 arch on Mac Rosetta via npm_config_arch.

If the npm_config_arch environment variable is set on Mac, then use the
specified architecture rather than overriding it to x64.
@tmacwill tmacwill requested a review from a team as a code owner December 21, 2021 21:03
@welcome
Copy link

welcome bot commented Dec 21, 2021

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

  • fix: don't overwrite prevent_default if default wasn't prevented
  • feat: add app.isPackaged() method
  • docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Dec 21, 2021
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Dec 28, 2021
@beorn
Copy link

beorn commented Jan 5, 2022

This would be very useful to have. It seems like the appveyor builds/tests fail are due to time/quota issues.

@zcbenz zcbenz added semver/patch backwards-compatible bug fixes target/14-x-y labels Jan 6, 2022
@zcbenz zcbenz merged commit 824c909 into electron:main Jan 7, 2022
@welcome
Copy link

welcome bot commented Jan 7, 2022

Congrats on merging your first pull request! 🎉🎉🎉

@release-clerk
Copy link

release-clerk bot commented Jan 7, 2022

Release Notes Persisted

Allowed specifying x64 arch on Mac Rosetta via npm_config_arch.

@trop
Copy link
Contributor

trop bot commented Jan 7, 2022

I was unable to backport this PR to "14-x-y" cleanly;
you will need to perform this backport manually.

@trop
Copy link
Contributor

trop bot commented Jan 7, 2022

I have automatically backported this PR to "15-x-y", please check out #32380

@trop
Copy link
Contributor

trop bot commented Jan 7, 2022

I have automatically backported this PR to "16-x-y", please check out #32381

@trop trop bot added the in-flight/15-x-y label Jan 7, 2022
@trop
Copy link
Contributor

trop bot commented Jan 7, 2022

I have automatically backported this PR to "17-x-y", please check out #32382

@trop trop bot added the in-flight/16-x-y label Jan 7, 2022
t57ser pushed a commit to t57ser/electron that referenced this pull request Jan 25, 2022
If the npm_config_arch environment variable is set on Mac, then use the
specified architecture rather than overriding it to x64.
@knaos
Copy link

knaos commented Apr 6, 2022

This works but unfortunately the npm_config_arch field is interfering with other tools we use. For example, in our Electron project we use NativeAddons which we build with node-gyp. Setting arch=x64 in our .npmrc project breaks the node-gyp build. Would it be appropriate for a custom flag to be added which instructs only electron to pick up a different version of itself?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Ability to force the installation of x64 electron binaries on Apple Silicon
6 participants