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

Codegen not working on Apple Silicon / M1 without Rosetta #1611

Closed
LinusU opened this issue Jan 6, 2021 · 16 comments
Closed

Codegen not working on Apple Silicon / M1 without Rosetta #1611

LinusU opened this issue Jan 6, 2021 · 16 comments

Comments

@LinusU
Copy link

LinusU commented Jan 6, 2021

Bug report

The run-bundled-codegen.sh script uses a bundled copy of node which is an Intel binary. This means that it cannot run on the new M1 Macs (without Rosetta), and fails with the following message:

env: node: Bad CPU type in executable

I believe that an easy fix is to upgrade the bundled Node.js version to a universal binary.

Versions

Please fill in the versions you're currently using:

  • apollo-ios SDK version: 0.39.0
  • Xcode version: 12.3
  • Swift version: 5.3.2
  • Package manager: 5.3.0

Steps to reproduce

Run the run-bundled-codegen.sh script on an M1 Mac.

@LinusU
Copy link
Author

LinusU commented Jan 6, 2021

If anyone needs a quick workaround, it can be solved by simply replacing the bundled Node with your system binary (e.g. the one installed via Brew):

cp $(which node) ~/Library/Developer/Xcode/DerivedData/PROJECT-xxxxxxxxxxxxxxxxxxxxxxxxxxxx/SourcePackages/checkouts/apollo-ios/scripts/apollo/bin/node

@designatednerd
Copy link
Contributor

I'd respectfully disagree with "not working" as it's working when using rosetta.

I'll take a look at what we can do about getting a silicon binary to run natively though.

@LinusU LinusU changed the title Codegen not working on Apple Silicon / M1 Codegen not working on Apple Silicon / M1 without Rosetta Jan 7, 2021
@LinusU
Copy link
Author

LinusU commented Jan 7, 2021

I'd respectfully disagree with "not working" as it's working when using rosetta.

Sure thing 👍 I've updated the title

The reason I said not working is because I didn't have Rosetta installed, and the only information I got was a failed build with "node: Bad CPU type in executable" buried in the logs.

Let me know if there is anything I can do to help!

@designatednerd
Copy link
Contributor

Yeah I think the overwhelming majority of folks are going to have rosetta installed, at least to start with.

Unfortunately most of what I'd need to take a look at is on our end around the packaging, but thank you for the offer!

@LinusU
Copy link
Author

LinusU commented Apr 27, 2021

@designatednerd any update on this? Ran into this today again 😅

Node.js 14 is getting Apple Silicon support soon (nodejs/node#38051) and the newly released 16 version already have it 🚀

edit: For some reason the above posted workaround doesn't work anymore 🤔

Instead, open ~/Library/Developer/Xcode/DerivedData/PROJECT-xxxxxxxxxxxxxxxxxxxxxxxxxxxx/SourcePackages/checkouts/apollo-ios/scripts/run-bundled-codegen.sh and change the path to include your Homebrew Node first:

  # Add the binary directory to the beginning of PATH so included binary verson of node is used.
- PATH="${SCRIPT_DIR}/apollo/bin:${PATH}" 
+ PATH="/opt/homebrew/bin:${SCRIPT_DIR}/apollo/bin:${PATH}" 

@designatednerd
Copy link
Contributor

Talked this through with some colleagues - we'll take a look at updating once the Node 14 universal binary gets released. Right now the version bundled for iOS use is 12 since that was the LTS version when we first started putting this workaround together.

@sigmadeltasoftware
Copy link

@designatednerd any update on this? Ran into this today again 😅

Node.js 14 is getting Apple Silicon support soon (nodejs/node#38051) and the newly released 16 version already have it 🚀

edit: For some reason the above posted workaround doesn't work anymore 🤔

Instead, open ~/Library/Developer/Xcode/DerivedData/PROJECT-xxxxxxxxxxxxxxxxxxxxxxxxxxxx/SourcePackages/checkouts/apollo-ios/scripts/run-bundled-codegen.sh and change the path to include your Homebrew Node first:

  # Add the binary directory to the beginning of PATH so included binary verson of node is used.
- PATH="${SCRIPT_DIR}/apollo/bin:${PATH}" 
+ PATH="/opt/homebrew/bin:${SCRIPT_DIR}/apollo/bin:${PATH}" 

Ran into this same issue today as well, script change still worked but the file itself was now located inside of the Pods directory:

$PROJECT_ROOT/Pods/Apollo/scripts/run-bundled-codegen.sh

@aravasio
Copy link

aravasio commented Nov 17, 2021

I just want to confirm @sigmadeltasoftware answer. It works, but it's also a bad long-term solution, since something such as removing the Podfile.lock or deintegrating / installing cocoapods will bring this issue back, since the script that's inside /Pods/Apollo/Scripts will be the invalid one.

I think the actual problem is that the script located in:
Pods/Apollo/scripts/run-bundled-codegen.sh
states that there's a bundled apollo-cli binary within the Pod. And that binary is probably built for x86, which means that it will always fail without Rosetta.

PATH="${SCRIPT_DIR}/apollo/bin:${PATH}"

# Use the bundled executable of the Apollo CLI to generate code
APOLLO_CLI="${SCRIPT_DIR}/apollo/bin/run" <---- this is the in-bundle reference

Maybe it should check if it's intel-based or arm-based?

I suppose using npm instead of homebrew might fix this, but I'm not 100% sure, since I'm no NPM expert.

@designatednerd
Copy link
Contributor

This is actually using neither homebrew nor NPM, it's downloading a big old pile of code (including basically all of node) from a bundle on our website. Which is basically a workaround to using NPM because the issues here used to be about 2/3 people having issues with NPM and node versioning because it's not an ecosystem most mobile developers are super-familiar with. 🙃

The good news is that with the system we're moving to with Apollo 1.0 (see RFC here), we will be using the JavaScriptCore SDK embedded with macOS rather than dealing with Node and all its attendant...fun.

This also does mean that the workaround is what you should use if you're vehemently against using Rosetta - we won't be updating the old versions to use a node binary built for it, since we're planning on dropping the whole charade and we're choosing to focus our efforts on getting that out as soon as possible.

Hope that helps!

@sigmadeltasoftware
Copy link

@designatednerd Thank you for the explanation & zooming in on the current priorities. In context of the workaround suggestion, I've opened a pull request above which at least incorporates it into the existing codebase until the move to the JavaScriptCore SDK has been completed.

This way current (non-Rosetta) Apple Silicon users can at least keep enjoying apollo-ios without first having to fumble through a series of vague XCode error logs until they end up in this thread.

@designatednerd
Copy link
Contributor

The issues only occur if the user does not have Rosetta installed. A huge number of apps still require Rosetta, so the overwhelming majority of users with M1 laptops should not run into this problem.

We're fine with people using this as a workaround if they really, really can't install Rosetta for whatever reason, but I don't think adding it to the main script is a good idea as it opens a pandora's box of possible errors (what if the user doesn't have homebrew installed? what if the user hasn't installed the version of the apollo-tooling repo that corresponds to the version of apollo-ios that they're currently using) that we'd have an awful time hunting down, which was the reason for bundling everything locally in the first place.

Does that make sense?

@aravasio
Copy link

aravasio commented Nov 18, 2021

@designatednerd It does make sense, but truth be told, I'm not entirely sold on why is adding a second binary (for arm64) and checking for architecture on-script such a huge deal.

It is my understanding that you already have a binary bundled in the pod, why can't you have both? That would remove dependency hell and allow for multiplatform support out of the box, even if it would make for a bigger pod, considering there's a whole other binary around now.

PATH="${SCRIPT_DIR}/apollo/bin:${PATH}
if [[ `uname -m` == 'arm64' ]]; then
  PATH=${SCRIPT_DIR}/apollo/bin-arm64:${PATH}
fi

or something akin to that, seems like a reasonable approach, although I'm admittedly working on the assumption that bundling node for arm64 would be a straightforward task in itself (considering you can install it with homebrew, such a binary ought to exist somewhere), which I'm not able to evaluate on my own.

Am I missing something? I think that's a far better option than, as you pointed out, working on the assumption node is installed god-knows-where and hoping for the best.

@designatednerd
Copy link
Contributor

@aravasio I went into quite a bit more detail here in the PR, but yeah, unfortunately, it's not quite as straightforward as it sounds.

@pedrommcarrasco
Copy link

What's the current status on this @designatednerd? 🤔

@AnthonyMDev
Copy link
Contributor

For the 1.0 version, we are rewriting the entire code generation engine in Swift. So this won't be a problem anymore. There is currently no plan to resolve this issue for the existing (soon to be "legacy") codegen. The workarounds here aren't perfect, but they work well enough to keep people moving forward until 1.0 is officially released.

@pedrommcarrasco
Copy link

Ok, thank you for replying so quickly @AnthonyMDev - good luck on the 1.0! 🚀

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

6 participants