-
Notifications
You must be signed in to change notification settings - Fork 31.2k
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
build: commit some android build and test fixtures #57748
base: main
Are you sure you want to change the base?
Conversation
Review requested:
|
5d4758e
to
3087b6f
Compare
17a21ac
to
961976e
Compare
patches = [ | ||
'./android-patches/common.gypi.patch', | ||
'./android-patches/trap-handler.h.patch', | ||
'./android-patches/v8.gyp.patch', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe both v8.gyp.patch
and common.gypi.patch
should be actually committed rather than being left as patch files. The reason why I made them as patch files is because I find them quite hacky and smelly. Would like to know what approach do you guys prefer.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #57748 +/- ##
==========================================
- Coverage 90.24% 90.24% -0.01%
==========================================
Files 630 630
Lines 184990 184990
Branches 36216 36215 -1
==========================================
- Hits 166948 166936 -12
+ Misses 11003 11001 -2
- Partials 7039 7053 +14 🚀 New features to boost your workflow:
|
Interesting, MacOS and FreeBSD do not have procfs, but have |
This commit contains some test fixes as well as build fixes for Android which I have been maintaining for Termux since a few years now. The full list of patches can be found at https://github.com/termux/termux-packages/tree/master/packages/nodejs and https://github.com/termux/termux-packages/tree/master/packages/nodejs-lts Not all the patches from the above sources are being committed over here as a some off them are specific to Termux. This does not fix the builds for Android completely, I was not able to successfully build Node locally according to the build instructions for Android, but I was able to build with the patches as in the above two URLs, in a separate environment. The build scripts used to build Node can be found in build.sh in the above directories linked. Here's a summary of what's changed: - Older Android versions doesn't have /dev/stdin, /dev/stdout, or /dev/stderr. Although this seems to have changed now, I can see that they exist on my new phone. Anyways these are specifically listed as not being the part of the POSIX standard. https://pubs.opengroup.org/onlinepubs/9799919799/ (See 2.1.1 Requirements, the last bullet point of 4th point specifically states that these three character special files are non- standard extension). The tests making use of /dev/stdin have been modified to use /proc/self/fd/0 which is guaranteed to exist on all POSIX platforms - Some other tests where Linux kernel behavior is different from other platforms (buffer size tests) have been patched to treat Android as Linux since Android makes use of the Linux kernel under the hood. - tools/v8_gypfiles/toolchain.gypi adds -m32 to host build flags when compiling for 32 bit target architecture. But in common.gypi adding -m64 conflicts with it which may cause compilation errors. Having both -m32 and -m64 may raise errors that are hard to debug. This patch probably needs to be improved but works good enough for Android builds. This probably hasn't been noticed yet because cross-compilations aren't that common for other platforms, Android has no other option but to cross compile. android-patches/common.gypi.patch does fix the builds for 32-bit architectures on 64-bit host, by not not adding -m64 to host build when cross compiling for 32-bit. - android-patches/v8.gyp.patch fixes assembly errors during cross compilation for 32-bit on 64-bit host. This patch is needed for the same reason as for the above patch - android-patches/trap-handler.h.patch has been updated so that it applies cleanly in the new source tree. - libuv does not implement uv_udp_set_source_membership for Android, OpenBSD and FreeBSD. So for these platforms it returns ENOSYS instead of EINVAL in tests. Tests have been patched to reflect this
FreeBSD and MacOS do not have procfs, so use /dev/fd/0 This is still not the POSIX way of doing things. Apparently there exist no POSIX way of doing this. /dev/stdin, /dev/stdout, and /dev/stderr are POSIX extensions, so is procfs. Also there is no mention of /dev/fd/{fd} anywhere at all in the POSIX specification over at https://doi.org/10.1109/IEEESTD.2024.10555529
961976e
to
e03ad8c
Compare
Hey, it's been 3 days since I created this PR and haven't received any reviews. Also I think @nodejs/build should also review this PR (someone from the nodejs org needs to mention this, as I am not part of the org, this doesn't actually notify them) |
This commit contains some test fixes as well as build fixes for Android which I have been maintaining for Termux since a few years now. The full list of patches can be found at https://github.com/termux/termux-packages/tree/master/packages/nodejs and https://github.com/termux/termux-packages/tree/master/packages/nodejs-lts Not all the patches from the above sources are being committed over here as a some off them are specific to Termux. This does not fix the builds for Android completely, I was not able to successfully build Node locally according to the build instructions for Android, but I was able to build with the patches as in the above two URLs, in a separate environment. The build scripts used to build Node can be found in build.sh in the above directories linked.
Here's a summary of what's changed:
Older Android versions doesn't have /dev/stdin, /dev/stdout, or /dev/stderr. Although this seems to have changed now, I can see that they exist on my new phone. Anyways these are specifically listed as not being the part of the POSIX standard. https://pubs.opengroup.org/onlinepubs/9799919799/ (See 2.1.1 Requirements, the last bullet point of 4th point specifically states that these three character special files are non- standard extension). The tests making use of /dev/stdin have been modified to use /proc/self/fd/0 which is guaranteed to exist on all POSIX platforms
Some other tests where Linux kernel behavior is different from other platforms (buffer size tests) have been patched to treat Android as Linux since Android makes use of the Linux kernel under the hood.
tools/v8_gypfiles/toolchain.gypi adds -m32 to host build flags when compiling for 32 bit target architecture. But in common.gypi adding -m64 conflicts with it which may cause compilation errors. Having both -m32 and -m64 may raise errors that are hard to debug. This patch probably needs to be improved but works good enough for Android builds. This probably hasn't been noticed yet because cross-compilations aren't that common for other platforms, Android has no other option but to cross compile. android-patches/common.gypi.patch does fix the builds for 32-bit architectures on 64-bit host, by not not adding -m64 to host build when cross compiling for 32-bit.
android-patches/v8.gyp.patch fixes assembly errors during cross compilation for 32-bit on 64-bit host. This patch is needed for the same reason as for the above patch
android-patches/trap-handler.h.patch has been updated so that it applies cleanly in the new source tree.
libuv does not implement uv_udp_set_source_membership for Android, OpenBSD and FreeBSD. So for these platforms it returns ENOSYS instead of EINVAL in tests. Tests have been patched to reflect this
Additionally, I also tried building for FreeBSD in a VM, but the builds failed because I didn't apply the patches used by FreeBSD https://github.com/freebsd/freebsd-ports/tree/main/www/node23