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

Android support #5544

Closed
wants to merge 2 commits into from
Closed

Conversation

robertchiras
Copy link
Contributor

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
Copy link
Member

saghul commented Mar 3, 2016

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.

@saghul saghul added the build Issues and PRs related to build files or the CI. label Mar 3, 2016
@robertchiras
Copy link
Contributor Author

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

@saghul
Copy link
Member

saghul commented Mar 3, 2016

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/

@robertchiras robertchiras force-pushed the android_support branch 2 times, most recently from 20bdb2e to 33b4c05 Compare March 3, 2016 15:20
*)
echo "Unsupported architecture provided: $ARCH"
exit 1
;;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@mscdex mscdex added the arm Issues and PRs related to the ARM platform. label Mar 3, 2016
@robertchiras
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Contributor Author

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

TARGET_ARCH="$ARCH"
fi

./configure \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@bnoordhuis
Copy link
Member

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 <[email protected]>
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 <[email protected]>
@robertchiras
Copy link
Contributor Author

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
Copy link
Member

rvagg commented Mar 22, 2016

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

@bnoordhuis bnoordhuis added the npm Issues and PRs related to the npm client dependency or the npm registry. label Mar 22, 2016
bnoordhuis pushed a commit that referenced this pull request Mar 22, 2016
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 <[email protected]>
PR-URL: #5544
Reviewed-By: Ben Noordhuis <[email protected]>
bnoordhuis pushed a commit that referenced this pull request Mar 22, 2016
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 <[email protected]>
PR-URL: #5544
Reviewed-By: Ben Noordhuis <[email protected]>
@bnoordhuis
Copy link
Member

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.

@bnoordhuis bnoordhuis closed this Mar 22, 2016
evanlucas pushed a commit that referenced this pull request Mar 30, 2016
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 <[email protected]>
PR-URL: #5544
Reviewed-By: Ben Noordhuis <[email protected]>
evanlucas pushed a commit that referenced this pull request Mar 30, 2016
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 <[email protected]>
PR-URL: #5544
Reviewed-By: Ben Noordhuis <[email protected]>
evanlucas pushed a commit that referenced this pull request Mar 31, 2016
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 <[email protected]>
PR-URL: #5544
Reviewed-By: Ben Noordhuis <[email protected]>
evanlucas pushed a commit that referenced this pull request Mar 31, 2016
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 <[email protected]>
PR-URL: #5544
Reviewed-By: Ben Noordhuis <[email protected]>
@MylesBorins
Copy link
Contributor

@bnoordhuis what are your thoughts regarding lts

@jasnell
Copy link
Member

jasnell commented Apr 9, 2016

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

@bnoordhuis
Copy link
Member

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

MylesBorins pushed a commit that referenced this pull request May 3, 2016
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 <[email protected]>
PR-URL: #5544
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 3, 2016
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 <[email protected]>
PR-URL: #5544
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 6, 2016
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 <[email protected]>
PR-URL: #5544
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 6, 2016
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 <[email protected]>
PR-URL: #5544
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 18, 2016
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 <[email protected]>
PR-URL: #5544
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 18, 2016
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 <[email protected]>
PR-URL: #5544
Reviewed-By: Ben Noordhuis <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arm Issues and PRs related to the ARM platform. build Issues and PRs related to build files or the CI. npm Issues and PRs related to the npm client dependency or the npm registry.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants