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.

Sign up for GitHub

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

Jump to bottom

build: support android build on ndk version equal or above 23(Android… #31521

Closed
wants to merge 7 commits into from
Closed

build: support android build on ndk version equal or above 23(Android… #31521

wants to merge 7 commits into from

Conversation

forfun414
Copy link
Contributor

… 6.0)

change scripts and sources for android build, don't need standalone toolchain a
fter ndk 19, and use clang as default android target compiler.

Checklist
  • [x ] make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • [x ] commit message follows commit guidelines

@forfun414
build: support android build on ndk version equal or above 23(Android…
2981fb4 
… 6.0)

change scripts and sources for android build, don't need standalone toolchain a\
fter ndk 19, and use clang as default android target compiler.
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. libuv Issues and PRs related to the libuv dependency or the uv binding. openssl Issues and PRs related to the OpenSSL dependency. tools Issues and PRs related to the tools directory. labels Jan 26, 2020
devnexen
devnexen previously approved these changes Jan 27, 2020
View reviewed changes
deps/openssl/openssl-cl_no_asm.gypi Outdated Show resolved Hide resolved
@Trott Trott added the android Issues and PRs related to the android platform. label Jan 28, 2020
@Trott
Copy link
Member

Trott commented Jan 28, 2020

/ping @gcampax Any chance you would be able to verify these changes?

@forfun414
build: support android build on ndk version equal or above 23(Android…
93d6de1 
… 6.0)

merge linux and android into one conditon
@forfun414
build: support android build on ndk version equal or above 23(Android…
0dac234 
… 6.0)

merge linux and android into one conditon and support arch aarch64 compile
richardlau
richardlau reviewed Jan 29, 2020
View reviewed changes
android-configure Outdated Show resolved Hide resolved
@forfun414
build: support android build on ndk version equal or above 23(Android…
fdd4cfc 
… 6.0)

fix gcc version checking failed conditons, like gcc 8.1
@gcampax
Copy link
Contributor

gcampax commented Feb 14, 2020

I build-tested this locally and it seems to work. One caveat is that I don't use the android-configure script so I didn't test that.
Also, I realized I had the same patch for c-ares locally, so that's good for sure.

@Trott
Copy link
Member

Trott commented Feb 18, 2020

@nodejs/collaborators Any folks with sufficient Android familiarity to approve this? Maybe some folks to at least weigh in on the GYP changes? I'd hate to have to chose between rubber-stamping this or not including it.....

cclauss
cclauss requested changes Feb 18, 2020
View reviewed changes
deps/openssl/openssl_no_asm.gypi Outdated Show resolved Hide resolved
@forfun414
build: support android build on ndk version equal or above 23(Android…
a4aedd4 
… 6.0)

change syntax for android and linux case
@nodejs-github-bot
Copy link
Collaborator

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

@Trott
Copy link
Member

Trott commented Mar 10, 2020

Would any of these files in deps need to be upstreamed first or are they all Node.js additions? Not sure who to ping for that question but let's go with @nodejs/build-files

@richardlau
Copy link
Member

Would any of these files in deps need to be upstreamed first or are they all Node.js additions? Not sure who to ping for that question but let's go with @nodejs/build-files

With the exception of the deps/uv files those are all Node.js additions. libuv is in the process of dropping gyp support (libuv/libuv#2682) so we'll end up owning the gyp files here in nodejs/node (like we have done with the gyp files for V8).

cc @nodejs/libuv

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 11, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@Trott
Copy link
Member

Trott commented Mar 11, 2020

This has one approval, has been open for more than seven days, and has a passing CI. This can land. Would be nice to get some more eyes on it. While it's a change for android, it touches a few files that are used by everyone.....

addaleax pushed a commit that referenced this pull request Mar 11, 2020
@forfun414 @addaleax
build: support android build on ndk version equal or above 23
50317c3 
change scripts and sources for android build, don't need standalone
toolchain after ndk 19, and use clang as default android target
compiler.

PR-URL: #31521
Reviewed-By: Christian Clauss <[email protected]>
@addaleax
Copy link
Member

Landed in 50317c3

@addaleax addaleax closed this Mar 11, 2020
MylesBorins pushed a commit that referenced this pull request Mar 11, 2020
@forfun414 @MylesBorins
build: support android build on ndk version equal or above 23
d66daa5 
change scripts and sources for android build, don't need standalone
toolchain after ndk 19, and use clang as default android target
compiler.

PR-URL: #31521
Reviewed-By: Christian Clauss <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 12, 2020
Merged
gcampax pushed a commit to stanford-oval/node that referenced this pull request Apr 5, 2020
@forfun414 @gcampax
build: support android build on ndk version equal or above 23
09b1f46 
change scripts and sources for android build, don't need standalone
toolchain after ndk 19, and use clang as default android target
compiler.

PR-URL: nodejs#31521
Reviewed-By: Christian Clauss <[email protected]>
targos pushed a commit that referenced this pull request Apr 22, 2020
@forfun414 @targos
build: support android build on ndk version equal or above 23
e655869 
change scripts and sources for android build, don't need standalone
toolchain after ndk 19, and use clang as default android target
compiler.

PR-URL: #31521
Reviewed-By: Christian Clauss <[email protected]>
@targos targos mentioned this pull request Apr 22, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau left review comments

@cclauss cclauss cclauss approved these changes

@mscdex mscdex mscdex left review comments

@devnexen devnexen devnexen left review comments

Assignees
No one assigned
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. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. libuv Issues and PRs related to the libuv dependency or the uv binding. openssl Issues and PRs related to the OpenSSL dependency. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@forfun414 @Trott @gcampax @nodejs-github-bot @richardlau @addaleax @mscdex @cclauss @devnexen