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

relay-compiler v13: Linux-ARM platform support #3799

Closed
atatarenko opened this issue Feb 17, 2022 · 8 comments
Closed

relay-compiler v13: Linux-ARM platform support #3799

atatarenko opened this issue Feb 17, 2022 · 8 comments

Comments

@atatarenko
Copy link

Hello relay team! Thank you for all the effort you've been putting into the relay!

I have a question (or even a feature request) but let me quickly describe the context.

My laptop is on based apple m1 platform, at the same my team and I prefer to develop in docker containers.
It means that my usual platform is linux-arm64 which is not supported among other platforms listed below:

if (process.platform === 'darwin' && process.arch === 'x64') {
binary = path.join(__dirname, 'macos-x64', 'relay');
} else if (process.platform === 'darwin' && process.arch === 'arm64') {
binary = path.join(__dirname, 'macos-arm64', 'relay');
} else if (process.platform === 'linux' && process.arch === 'x64') {
binary = path.join(__dirname, 'linux-x64', 'relay');
} else if (process.platform === 'win32' && process.arch === 'x64') {
binary = path.join(__dirname, 'win-x64', 'relay.exe');
} else {
binary = null;
}

Now I have to run the new relay-compiler in a docker container forcing the platform for it this way platform: linux/x86_64 which doesn't bring any benefits of node -> rust migration when it comes to speed. And actually brings another layer of virtualization to the whole app. Not that bad but not perfect too.

Do you have any plans regarding linux-arm64 support for the new compiler?

@alunyov
Copy link
Contributor

alunyov commented Feb 17, 2022

Thanks for suggestion, I think we could add support for this here:

target:
- target: x86_64-unknown-linux-musl
os: ubuntu-latest
build-name: relay
artifact-name: relay-compiler-linux-x64
packages: musl-tools
features: vendored
- target: x86_64-apple-darwin
os: macos-latest
build-name: relay
artifact-name: relay-compiler-macos-x64
- target: aarch64-apple-darwin
os: macos-latest
build-name: relay
artifact-name: relay-compiler-macos-arm64
- target: x86_64-pc-windows-msvc
os: windows-latest
build-name: relay.exe
artifact-name: relay-compiler-win-x64

The downsides:

  • it will decrease the CI time (probably fine)
  • it will increase the npm package size, as we're uploading all artifacts for all platforms there.

There is a PR to add platform-specific compilers as optional dependencies, but this approach also has downsides (as optional dependencies may be disabled in some cases).

I suggest keeping this issue open for now, if more people will want this, we can add support for this.

@ro-savage
Copy link

@alunyov - This is certainly very important - every new macbook is going to be arm-based. Given a large percentage of developers use macs - this issue is only going to increase.

As our organisation about 80% of developers are now on M1 Macbooks and would be unable to do builds using Relay v13.

@wasd171
Copy link
Contributor

wasd171 commented Apr 15, 2022

@alunyov you have added a "help wanted" label. How exactly could the community help? This issue is slowing down our development, I'd be willing to invest my time to fix it

@alunyov
Copy link
Contributor

alunyov commented Apr 18, 2022

@wasd171 something similar to this PR: #3667 - where we need to update CI to include a new target, also update the JS part of the compiler to resolve the correct binary.

@wasd171
Copy link
Contributor

wasd171 commented Apr 19, 2022

@alunyov I have created a PR #3882 that addresses this issue

facebook-github-bot pushed a commit that referenced this issue Apr 20, 2022
Summary:
Following discussions in #3799 this PR adds support for a Linux ARM64 target for `relay-compiler`

Pull Request resolved: #3882

Test Plan:
Imported from GitHub, without a `Test Plan:` line.

**Static Docs Preview: relay**
|[Full Site](https://our.intern.facebook.com/intern/staticdocs/eph/D35760366/V2/relay/)|

|**Modified Pages**|

Reviewed By: josephsavona

Differential Revision: D35760366

Pulled By: alunyov

fbshipit-source-id: 4e79ce2ccbe91a70f2d91d80667fd6960210c7f1
@TrySound
Copy link
Contributor

TrySound commented Apr 20, 2022

Does it mean relay-compiler package size will be ~10-20MB bigger?

@wasd171
Copy link
Contributor

wasd171 commented Apr 21, 2022

@TrySound additional binary is ~5 MB

@bencoveneyttl
Copy link

Looks like the fix was shipped in https://github.com/facebook/relay/releases/tag/v14.0.0 and this can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants