Android support #5544

robertchiras · 2016-03-03T13:12:19Z

Pull Request check-list

Please make sure to review and check all of these items:

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
I have one test failing (test-http-curl-chunk-problem), but this test also fails on a clean master branch, so it's not because of my patches
Is the commit message formatted according to [CONTRIBUTING.md][0]?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

Affected core subsystem(s)

build

Description of change

The provided patches add support for building node for Android systems (Brillo is also supported). This include building node with both NDK (building node outside of Android build system) and Bionic (building node inside Android build system).
Building node with Bionic, inside an Android/Brillo build system is useful mostly for vendors who wants to include node in their products.
In order to successfully build node with Bionic, I have another patch inside libuv, but I will submit that patch directly to libuv.

saghul · 2016-03-03T14:32:00Z

The changes to gyp need upstreaming, and if they need to be floated, they would need floating in node-gyp, which npm would then bump, not here. As a general rule, no patch should modify deps/ unless it updates a given dependency.

robertchiras · 2016-03-03T14:35:15Z

Yes, you are right. I will remove the change to gyp and upstream it to npm.
Thanks!

saghul · 2016-03-03T14:42:39Z

Not npm, sorry if I wasn't clear before. It should go GYP, which would then be picked up by node-gyp, which would then be picked up by npm. Yep, as messy as it sounds.

In case the patch has to be floated by node-gyp (which I don't know), it should go here: https://github.com/nodejs/node-gyp. GYP upstream is here: https://gyp.gsrc.io/

bnoordhuis · 2016-03-03T15:25:08Z

android-configure

+	*)
+		echo "Unsupported architecture provided: $ARCH"
+		exit 1
+		;;


Style issue: can you use two space indent instead of tabs?

robertchiras · 2016-03-04T08:45:46Z

@saghul Regarding the change to GYP: this is weird, because there is no "android.py" in pylib/gyp/generator from GYP upstream (https://chromium.googlesource.com/external/gyp), but in node-gyp I can see this file present.
Is it OK to submit my change to node-gyp only?
I don't know where they got that version of GYP which comes with an android generator, since GYP project from Chromium doesn't have it.

bnoordhuis · 2016-03-04T09:00:12Z

The android generator was removed last year in commit https://chromium.googlesource.com/external/gyp/+/f34b9aa7c9d6dc368aa01a82f9f847eca15b9e18. I think it's an oversight on our part that we didn't remove it from the copy that is bundled with node-gyp.

Is it OK to submit my change to node-gyp only?

I guess so but you'd be making changes to a component that is unmaintained. Expect bitrot.

robertchiras · 2016-03-14T12:54:42Z

Hi,
Is there anything I should do for this pull request to be accepted?
Thanks,
Robert

bnoordhuis · 2016-03-14T13:05:11Z

tools/create_android_makefiles

+  TARGET_ARCH="$ARCH"
+fi
+
+./configure \


You need to update the paths now that you've moved the file. Alternatively (and preferably), add a cd $(dirname "$0/..") at the top, that makes it independent of the current working directory.

Tiny style nit: can you indent the line continuations with four instead of two spaces? Thanks.

bnoordhuis · 2016-03-14T13:06:30Z

This also depends on an npm update before it's fully functional, IIRC?

Modified android-configure script to support also x86 arch. Currently added support only for ia32 target arch. Also, compile openssl without asm, since using the asm sources will make node fail to run on Android, because it adds text relocations. Signed-off-by: Robert Chiras <robert.chiras@intel.com>

The create_android_makefiles script will create .mk files for node and all of its dependencies ready to be build using Android build system. Signed-off-by: Robert Chiras <robert.chiras@intel.com>

robertchiras · 2016-03-14T14:48:32Z

Regarding dependency of the npm update. Yes, the patch "Add script to create Android .mk files" is kind of dependent of that npm update, by means that it will create Android .mk files (the script won't fail), but the resulting files won't be usable in an Android build environment.

rvagg · 2016-03-22T01:21:45Z

I'm seeking some feedback about how Node.js is used on Android if anyone watching this thread would like to tell us about it: nodejs/build#359

Modified android-configure script to support also x86 arch. Currently added support only for ia32 target arch. Also, compile openssl without asm, since using the asm sources will make node fail to run on Android, because it adds text relocations. Signed-off-by: Robert Chiras <robert.chiras@intel.com> PR-URL: #5544 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

The create_android_makefiles script will create .mk files for node and all of its dependencies ready to be build using Android build system. Signed-off-by: Robert Chiras <robert.chiras@intel.com> PR-URL: #5544 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

bnoordhuis · 2016-03-22T18:18:51Z

I decided to land the commits in 271201f and cd9f54b. There's been no npm upgrade so far but I figure it won't make matters worse to have these commits in. Thanks, Robert.

The create_android_makefiles script will create .mk files for node and all of its dependencies ready to be build using Android build system. Signed-off-by: Robert Chiras <robert.chiras@intel.com> PR-URL: #5544 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Modified android-configure script to support also x86 arch. Currently added support only for ia32 target arch. Also, compile openssl without asm, since using the asm sources will make node fail to run on Android, because it adds text relocations. Signed-off-by: Robert Chiras <robert.chiras@intel.com> PR-URL: #5544 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

The create_android_makefiles script will create .mk files for node and all of its dependencies ready to be build using Android build system. Signed-off-by: Robert Chiras <robert.chiras@intel.com> PR-URL: #5544 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Modified android-configure script to support also x86 arch. Currently added support only for ia32 target arch. Also, compile openssl without asm, since using the asm sources will make node fail to run on Android, because it adds text relocations. Signed-off-by: Robert Chiras <robert.chiras@intel.com> PR-URL: #5544 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins · 2016-04-08T20:53:07Z

@bnoordhuis what are your thoughts regarding lts

jasnell · 2016-04-09T00:03:13Z

Not sure if this needs to be in v4 at all.

bnoordhuis · 2016-04-09T09:13:50Z

I don't think cherry-picking the commits would hurt. They should apply cleanly and they're pretty standalone.

Modified android-configure script to support also x86 arch. Currently added support only for ia32 target arch. Also, compile openssl without asm, since using the asm sources will make node fail to run on Android, because it adds text relocations. Signed-off-by: Robert Chiras <robert.chiras@intel.com> PR-URL: #5544 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

The create_android_makefiles script will create .mk files for node and all of its dependencies ready to be build using Android build system. Signed-off-by: Robert Chiras <robert.chiras@intel.com> PR-URL: #5544 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Modified android-configure script to support also x86 arch. Currently added support only for ia32 target arch. Also, compile openssl without asm, since using the asm sources will make node fail to run on Android, because it adds text relocations. Signed-off-by: Robert Chiras <robert.chiras@intel.com> PR-URL: #5544 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

The create_android_makefiles script will create .mk files for node and all of its dependencies ready to be build using Android build system. Signed-off-by: Robert Chiras <robert.chiras@intel.com> PR-URL: #5544 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Modified android-configure script to support also x86 arch. Currently added support only for ia32 target arch. Also, compile openssl without asm, since using the asm sources will make node fail to run on Android, because it adds text relocations. Signed-off-by: Robert Chiras <robert.chiras@intel.com> PR-URL: #5544 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

The create_android_makefiles script will create .mk files for node and all of its dependencies ready to be build using Android build system. Signed-off-by: Robert Chiras <robert.chiras@intel.com> PR-URL: #5544 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

saghul added the build Issues and PRs related to build files or the CI. label Mar 3, 2016

robertchiras force-pushed the android_support branch 2 times, most recently from 20bdb2e to 33b4c05 Compare March 3, 2016 15:20

bnoordhuis reviewed Mar 3, 2016
View reviewed changes

mscdex added the arm Issues and PRs related to the ARM platform. label Mar 3, 2016

robertchiras force-pushed the android_support branch from 33b4c05 to c3b9451 Compare March 4, 2016 07:36

bnoordhuis reviewed Mar 14, 2016
View reviewed changes

robertchiras added 2 commits March 14, 2016 16:44

build: Add script to create Android .mk files

289ea4b

The create_android_makefiles script will create .mk files for node and all of its dependencies ready to be build using Android build system. Signed-off-by: Robert Chiras <robert.chiras@intel.com>

robertchiras force-pushed the android_support branch from c3b9451 to 289ea4b Compare March 14, 2016 14:44

bnoordhuis added the npm Issues and PRs related to the npm client dependency or the npm registry. label Mar 22, 2016

bnoordhuis closed this Mar 22, 2016

bnoordhuis added the lts-watch-v4.x label Mar 22, 2016

evanlucas mentioned this pull request Mar 31, 2016

v5.10.0 Release Proposal #5970

Closed

bnoordhuis mentioned this pull request May 2, 2016

build,android: Added option to compile for Android with -fPIE flag #5783

Closed

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels May 3, 2016

MylesBorins mentioned this pull request May 18, 2016

v4.4.5 proposal #6824

Merged

bnoordhuis mentioned this pull request Nov 17, 2017

Fix: module compilation within termux-app on android nodejs/node-gyp#1340

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Android support #5544

Android support #5544

robertchiras commented Mar 3, 2016

saghul commented Mar 3, 2016

robertchiras commented Mar 3, 2016

saghul commented Mar 3, 2016

bnoordhuis Mar 3, 2016

robertchiras Mar 4, 2016

robertchiras commented Mar 4, 2016

bnoordhuis commented Mar 4, 2016

robertchiras commented Mar 14, 2016

bnoordhuis Mar 14, 2016

robertchiras Mar 14, 2016

bnoordhuis commented Mar 14, 2016

robertchiras commented Mar 14, 2016

rvagg commented Mar 22, 2016

bnoordhuis commented Mar 22, 2016

MylesBorins commented Apr 8, 2016

jasnell commented Apr 9, 2016

bnoordhuis commented Apr 9, 2016

Android support #5544

Android support #5544

Conversation

robertchiras commented Mar 3, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

saghul commented Mar 3, 2016

robertchiras commented Mar 3, 2016

saghul commented Mar 3, 2016

bnoordhuis Mar 3, 2016

Choose a reason for hiding this comment

robertchiras Mar 4, 2016

Choose a reason for hiding this comment

robertchiras commented Mar 4, 2016

bnoordhuis commented Mar 4, 2016

robertchiras commented Mar 14, 2016

bnoordhuis Mar 14, 2016

Choose a reason for hiding this comment

robertchiras Mar 14, 2016

Choose a reason for hiding this comment

bnoordhuis commented Mar 14, 2016

robertchiras commented Mar 14, 2016

rvagg commented Mar 22, 2016

bnoordhuis commented Mar 22, 2016

MylesBorins commented Apr 8, 2016

jasnell commented Apr 9, 2016

bnoordhuis commented Apr 9, 2016