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

Patches to help build node as a shared library for Android #29388

Closed
wants to merge 2 commits into from

Conversation

gcampax
Copy link
Contributor

@gcampax gcampax commented Aug 31, 2019

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

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. labels Aug 31, 2019
@devnexen
Copy link
Contributor

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 Trott added the android Issues and PRs related to the android platform. label Sep 1, 2019
@Trott
Copy link
Member

Trott commented Sep 1, 2019

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

Trott commented Sep 1, 2019

@nodejs/python

@Trott
Copy link
Member

Trott commented Sep 1, 2019

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

gcampax commented Sep 1, 2019

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?

configure.py Show resolved Hide resolved
@devnexen
Copy link
Contributor

devnexen commented Sep 1, 2019

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.

'cflags': [ '-fPIE' ],
'ldflags': [ '-fPIE', '-pie' ]
'cflags': [ '-fPIC' ],
'ldflags': [ '-fPIC' ]
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

configure.py Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented Sep 1, 2019

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

@Trott
Copy link
Member

Trott commented Sep 7, 2019

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

devnexen commented Sep 9, 2019

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

@Trott
Copy link
Member

Trott commented Sep 26, 2019

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

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

@gcampax
Copy link
Contributor Author

gcampax commented Sep 26, 2019

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

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

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

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

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

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

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

@Trott
Copy link
Member

Trott commented Oct 22, 2019

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

Compiling a library with -fPIE won't do, and on Android libraries
are not versioned.
@gcampax
Copy link
Contributor Author

gcampax commented Oct 22, 2019

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

@gireeshpunathil
Copy link
Member

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!

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

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

@Trott
Copy link
Member

Trott commented Dec 12, 2019

Landed in d8fc0ae...73df09e

@Trott Trott closed this Dec 12, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Dec 12, 2019
Compiling a library with -fPIE won't do, and on Android libraries
are not versioned.

PR-URL: nodejs#29388
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Trott pushed a commit to Trott/io.js that referenced this pull request Dec 12, 2019
And other errors like lost promises

PR-URL: nodejs#29388
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 13, 2019
Compiling a library with -fPIE won't do, and on Android libraries
are not versioned.

PR-URL: #29388
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 13, 2019
And other errors like lost promises

PR-URL: #29388
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 13, 2019
targos pushed a commit that referenced this pull request Jan 14, 2020
Compiling a library with -fPIE won't do, and on Android libraries
are not versioned.

PR-URL: #29388
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Jan 14, 2020
And other errors like lost promises

PR-URL: #29388
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
Compiling a library with -fPIE won't do, and on Android libraries
are not versioned.

PR-URL: #29388
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
And other errors like lost promises

PR-URL: #29388
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 12, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 12, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 15, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 18, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 21, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 21, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 24, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 24, 2020
* chore: bump node in DEPS to v12.16.0

* Fixup asar support setup patch

nodejs/node#30862

* Fixup InternalCallbackScope patch

nodejs/node#30236

* Fixup GN buildfiles patch

nodejs/node#30755

* Fixup low-level hooks patch

nodejs/node#30466

* Fixup globals require patch

nodejs/node#31643

* Fixup process stream patch

nodejs/node#30862

* Fixup js2c modification patch

nodejs/node#30755

* Fixup internal fs override patch

nodejs/node#30610

* Fixup context-aware warn patch

nodejs/node#30336

* Fixup Node.js with ltcg config

nodejs/node#29388

* Fixup oaepLabel patch

nodejs/node#30917

* Remove redundant ESM test patch

nodejs/node#30997

* Remove redundant cli flag patch

nodejs/node#30466

* Update filenames.json

* Remove macro generation in GN build files

nodejs/node#30755

* Fix some compilation errors upstream

* Add uvwasi to deps

nodejs/node#30258

* Fix BoringSSL incompatibilities

* Fixup linked module patch

nodejs/node#30274

* Add missing sources to GN uv build

libuv/libuv#2347

* Patch some uvwasi incompatibilities

* chore: bump Node.js to v12.6.1

* Remove mark_arraybuffer_as_untransferable.patch

nodejs/node#30549

* Fix uvwasi build failure on win

* Fixup --perf-prof cli option error

* Fixup early cjs module loading

* fix: initialize diagnostics properly

nodejs/node#30025

* Disable new esm syntax specs

nodejs/node#30219

* Fixup v8 weakref hook spec

nodejs/node#29874

* Fix async context timer issue

* Disable monkey-patch-main spec

It relies on nodejs/node#29777, and we don't
override prepareStackTrace.

* Disable new tls specs

nodejs/node#23188

We don't support much of TLS owing to schisms between BoringSSL and
OpenSSL.

Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
android Issues and PRs related to the android platform. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet