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

Meta-issue to track cross-compiling progress #575

Closed
18 tasks done
vweevers opened this issue Dec 27, 2018 · 8 comments
Closed
18 tasks done

Meta-issue to track cross-compiling progress #575

vweevers opened this issue Dec 27, 2018 · 8 comments
Labels
discussion Discussion
Milestone

Comments

@vweevers
Copy link
Member

vweevers commented Dec 27, 2018

As this effort spans multiple projects (both in tooling and target audience). In mostly chronological order:

Feel free to edit if I missed anything.

cc @ralphtheninja @ahdinosaur @staltz @mafintosh

@vweevers vweevers added this to Backlog in Level (old board) via automation Dec 27, 2018
@ralphtheninja
Copy link
Member

ralphtheninja commented Dec 27, 2018

How do we pass ARM version suffix for output folder (e.g. -v7) to prebuildify?

The ARM_VERSION environment variable is passed on the docker container here https://github.com/ahdinosaur/prebuildify-cross/blob/master/build#L237 , so it's available to prebuildify when invoked here https://github.com/ahdinosaur/prebuildify-cross/blob/master/build-in-docker#L16

So we need to modify prebuildify to create this suffix for the arm case here https://github.com/mafintosh/prebuildify/blob/master/index.js#L33

Same question for libc flavor. Or should prebuildify always include it in output folder names?

I guess this should go into prebuildify in the same sense. If you think about it, prebuildify is used to make the binaries and node-gyp-build tries to resolve/load them, so it makes sense that they have similar/mirrored behavior (i.e. producer/consumer logic)

@vweevers
Copy link
Member Author

Making our desired changes the default behavior (rather than optional features) of prebuildify and node-gyp-build would be the cleanest solution, yea. It probably means both will need a major bump.

ahdinosaur added a commit to prebuild/prebuildify-cross that referenced this issue Dec 28, 2018
@ahdinosaur
Copy link

  • Don't pass -t electron@3.0.0 to prebuildify when cross-compiling

oh okay good to know. should i change the api of prebuildify-cross so that it takes arguments to pass to prebuildify? at the moment it just runs npm run prebuild, without any way to change arguments specifically for the prebuild.

i could change the prebuildify-cross api to something like:

prebuildify-cross --args="-t 8.14.0 --napi --strip" --platform=linux --arch=arm --arm-version=7

would that be helpful? if so, here's a branch for that functionality: https://github.com/ahdinosaur/prebuildify-cross/tree/prebuildify-args 🌟

(by the way, i will be away from my computer for next 5 days, off to party in the bush 🏞️ ☀️ 💃 🎶 )

@ralphtheninja
Copy link
Member

Don't pass -t electron@3.0.0 to prebuildify when cross-compiling

@vweevers At first I was wondering why we shouldn't do this, but I guess it doesn't make any sense to do this for e.g. android, but maybe we want this for other flavors? Would it make sense to have prebuilts made for electron on say Raspberry PI?

would that be helpful?

@ahdinosaur Yes, I think this would be helpful, since we need to have different behavior for npm run prebuild depending on which targets we are building for.

@vweevers vweevers added the discussion Discussion label Dec 31, 2018
@vweevers vweevers moved this from Backlog to In progress in Level (old board) Feb 1, 2019
@vweevers
Copy link
Member Author

Status update: it has taken quite a few iterations (of PRs, test releases and discussions), but I think we finally have a complete plan now. Note that some of the discussion above is outdated as we decided to replace prebuildify-cross with our (prebuild's) own Docker images.

I've opened prebuild/prebuildify#26 which consolidates recent ad-hoc changes (for overriding strip, platform, arch) into a clean and documented set of options. Once that is merged, we'll have to update the Docker images accordingly. Then we can implement the new naming scheme for prebuilds (prebuild/prebuildify#25), on which the discussion has now settled as well.

The only thing that's unclear to me is Android support. Specifically, whether to use and target the nodejs-mobile forks of node and node-gyp. I'm not convinced that's the way to go, because I'm afraid we'll be stuck with it. Node core is open to improving Android support - so that's what I prefer to aim for. I don't see an easy way to support both.

@staltz
Copy link
Contributor

staltz commented Mar 11, 2019

@vweevers nodejs-mobile uses node chakracore because (for reasons I can't remember now but Janea systems documents it well) that makes it possible to run in iOS. Having leveldown supported in iOS is as important as Android support in my opinion. Node core isn't yet as developer friendly as nodejs-mobile is when targeting mobile.

@vweevers
Copy link
Member Author

Would it be an option to use node core for Android, and nodejs-mobile for iOS? The former is already working. For the latter, I'm hoping that:

  1. Telling prebuildify to use nodejs-mobile-gyp instead of node-gyp is enough to get prebuilds working, running in a MacOS job in Travis
  2. os.platform() returns a distinct value (e.g. ios?) so that node-gyp-build can select the correct prebuild.

@vweevers
Copy link
Member Author

nodejs-mobile uses node chakracore because (for reasons I can't remember now [..]) that makes it possible to run in iOS

Thanks to this tweet I learned it's because iOS disallows write-access to executable memory. V8 7.4 addresses this by introducing a interpreter-only mode. Some time in the future node core should be able to take advantage of that!

cc @mcollina Does node have any official plans yet?

@vweevers vweevers added this to the 5.1.0 milestone Mar 25, 2019
@vweevers vweevers pinned this issue Mar 31, 2019
This was referenced Apr 27, 2019
Level (old board) automation moved this from In progress to Done Apr 28, 2019
@vweevers vweevers unpinned this issue Apr 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Discussion
Projects
No open projects
Development

No branches or pull requests

4 participants