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

build: commit some android build and test fixtures #57748

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

thunder-coding
Copy link
Contributor

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

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency. labels Apr 4, 2025
@thunder-coding thunder-coding changed the title android, build: commit some android build and test fixtures build: commit some android build and test fixtures Apr 4, 2025
@thunder-coding thunder-coding force-pushed the android-fixes branch 2 times, most recently from 17a21ac to 961976e Compare April 4, 2025 17:05
patches = [
'./android-patches/common.gypi.patch',
'./android-patches/trap-handler.h.patch',
'./android-patches/v8.gyp.patch',
Copy link
Contributor Author

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.

Copy link

codecov bot commented Apr 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.24%. Comparing base (9aa57bf) to head (e03ad8c).
Report is 1 commits behind head on main.

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     

see 22 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@thunder-coding
Copy link
Contributor Author

Interesting, MacOS and FreeBSD do not have procfs, but have /dev/fd/{fd}, which is also supported on Linux and Android. The interesting thing is that latest edition of the POSIX specifications have no mention of /dev/fd/{fd} or /proc. The only mention of /proc is in https://pubs.opengroup.org/onlinepubs/9799919799/ (page 15 of https://doi.org/10.1109/IEEESTD.2024.10555529 if you have access to it). So apparently there is no way to do this the POSIX way (i.e. with a guarantee that it'll work on every next POSIX OS).

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

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants