Patches to help build node as a shared library for Android

[gcampax]

Hello! I maintain an Android app that uses nodejs as a library. These patches have been necessary to build.
The patches are tested on the 8.x branch, which we're using currently. They did not rebase cleanly on master because the corresponding core was moved around, but I think they are small enough they should be correct.

I hope the patches are generally useful. I would love to have them upstream, so I can move away from 8.x

[devnexen]

Hello and thanks for your interest !
The commit messages need to follow this guideline https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#commit-message-guidelines

[Trott]

@gcampax Thank you so much! Improving Android support has been a wish-list item for a long time.

@nodejs/build @nodejs/build-files @nodejs/gyp

[Trott]

@nodejs/python

[Trott]

Hello and thanks for your interest !
The commit messages need to follow this guideline https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#commit-message-guidelines

If you end up rebasing or something, fixing the commit message would be nice, but if not, whoever lands this will fix it at that time, so it's not critical. But if you're into saving someone else a few keystrokes, yeah, please follow those guidelines!

[gcampax]

Hello and thanks for your interest !
The commit messages need to follow this guideline https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#commit-message-guidelines

Of course! Thanks for the review.
I thought I did follow the guidelines though: the commit summary is prefixed with the subsystem name, is imperative, and is lowercase; the body is wrapped at 72 characters (approx.). Anything else I should fix?

[devnexen]

Hello and thanks for your interest !
The commit messages need to follow this guideline https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#commit-message-guidelines

Of course! Thanks for the review.
I thought I did follow the guidelines though: the commit summary is prefixed with the subsystem name, is imperative, and is lowercase; the body is wrapped at 72 characters (approx.). Anything else I should fix?

The first line should:...be prefixed with the name of the changed subsystem means nodejs project structure subsystem.

[bnoordhuis]

You only build the shared library, don't you? Does this also work when building the node executable? -fPIE means Position Independent Executable.

[gcampax]

It will work, because you can build an executable with PIC code, but it will not be as efficient as PIE code. Not sure about the -pie link flag though.

[bnoordhuis]

Can you check that it still produces a working binary? I have no way to test myself.

[Trott]

I thought I did follow the guidelines though: the commit summary is prefixed with the subsystem name, is imperative, and is lowercase; the body is wrapped at 72 characters (approx.). Anything else I should fix?

The subsystem should be build: rather than android: but that's a super minor thing. While there are some guidelines for how to determine the subsystem, a lot of it basically comes down to "This is how it's been done for the last several years."

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/25313/

[Trott]

@nodejs/collaborators @nodejs/hardware Pinging widely on this, sorry, but if anyone is invested in Node.js running on Android (and even if not), this could use some reviews. Even better, it could use some testing--that is, someone who can say "I tried to compile with this on Android and it worked!"

[devnexen]

Would it be handy (and useful) to update android-configure ?

[Trott]

@nodejs/members Anyone able to try compiling on Android with this patch and report results? It would be super-cool to get some confirmation that it works for people and inch Android support a little bit forward.

[gengjiawen]

@gcampax what's your step to build Node.js as a android library ?

[gcampax]

Hi all, sorry for the delayed reply...

@gcampax what's your step to build Node.js as a android library ?

As I mentioned at the openining of the issue, I build the 8.x branch, not master, and I forward-ported my patches without really testing them.
On 8.x, I set CC, CXX, LINK, etc. environment variables as well as CC.host, CXX.host, etc. and then I run:

export GYP_DEFINES="target_arch=${arch} v8_target_arch=${nodejs_cpu} android_target_arch=${arch} host_os=linux OS=android"
./configure --shared --dest-cpu=${nodejs_cpu} --dest-os=android --without-snapshot --openssl-no-asm --with-intl=small-icu --without-inspector

You can find the full build script at https://github.com/stanford-oval/almond-android/blob/master/rebuild-libraries.sh , including the appropriate settings of arch/android_arch/nodejs_cpu .

[gireeshpunathil]

just pinging folks in general to see where we stand. Did anyone have built / tested this successfully? I know our CI will not complain, as there is no flow that captures the android build and test.

[gireeshpunathil]

the Travis CI failure is because wrong sub-system. There seem to be an android sub-system, but the commit message validator seem to have no knowledge about it.

either we change the subsystem to shared_lib, or ignore error for now?

[richardlau]

the Travis CI failure is because wrong sub-system. There seem to be an android sub-system, but the commit message validator seem to have no knowledge about it.

either we change the subsystem to shared_lib, or ignore error for now?

https://github.com/nodejs/node/pull/29388/commits#issuecomment-526936711

[gireeshpunathil]

@richardlau - sorry, I did not follow that. I meant to say: android as a label exists, but not as a module sub-system, or at least the commit metadata validator does not recognize that; and hence the linter failure. Do you agree?

[richardlau]

@richardlau - sorry, I did not follow that. I meant to say: android as a label exists, but not as a module sub-system, or at least the commit metadata validator does not recognize that; and hence the linter failure. Do you agree?

Although there is some overlap, labels and subsystems are not quite the same thing. For example, currently the only platform subsystem that core-validate-commit recognises is win while the equivalent label is windows.

Of the currently recognised subsystems recognised by the validator build would seem to be the most fitting, as suggested by @Trott earlier.

[gireeshpunathil]

pinging @gcampax to see where do we stand and what it takes to move this forward.

[Trott]

pinging @gcampax to see where do we stand and what it takes to move this forward.

There are a few comments above for @gcampax but for the most part, the hold up here would seem to be a lack of Android folks among Collaborators who can test and/or approve this. Maybe one of our Google folks might be able to identify someone who might have an interest in Node.js on Android who can help out with testing and/or review? @fhinkel @MylesBorins @ofrobots @bcoe @bmeurer @eugeneo @psmarshall @hashseed

[gcampax]

Ok, I have force-pushed an update of this patchset. As I mentioned, I don't have a way to test master, because I am currently building 8.x and every version switch is a lot of work.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/27276/

[gireeshpunathil]

the CI is green; is there anything else that needs to be done before this can get in? @bcoe - was there any insights / comments that came out of the internal review you mentioned earlier? I would like to land this, but not being an SME on this area, would like to hear a green signal from one more expert!

[Trott]

Rubber-stamp LGTM. If you're the only Android dev weighing in, then let's at least make your life easier.

[Trott]

Landed in d8fc0ae...73df09e