Buildroot Archive on lore.kernel.org
 help / color / mirror / Atom feed
From: "Yann E. MORIN" <yann.morin.1998@free.fr>
To: Giulio Benetti <giulio.benetti@benettiengineering.com>
Cc: Martin Bark <martin@barkynet.com>,
	Daniel Price <daniel.price@gmail.com>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	buildroot@buildroot.org
Subject: Re: [Buildroot] [PATCH] package/nodejs/nodejs-src: remove .node files with different architecture
Date: Wed, 1 Nov 2023 16:21:14 +0100	[thread overview]
Message-ID: <20231101152114.GE114892@scaer> (raw)
In-Reply-To: <20231031120713.678783-1-giulio.benetti@benettiengineering.com>

Giulio, All,

On 2023-10-31 13:07 +0100, Giulio Benetti spake thusly:
> Actually nodejs-src fails to build when additional modules with prebuilt
> .node files are added. This is due to Buildroot's check-bin-arch that
> checks for executable files and libraries that have different architecture
> from the one we're building for. So let's go through all .node files in
> $(TARGET_DIR)/usr/lib/node_modules and check if the architecture is
> different or not found and in case delete the .node file.

Unfortunately, this is not going to work correctly.

For example, the module failing in #15823 is reported to be serialport,
but really the failing one is the scoped @serialport/bindings-cpp
module.

That module has "prebuilds" binaries for various systems, here's the
list:

    $ find prebuilds/ -type f
    prebuilds/android-arm/node.napi.armv7.node
    prebuilds/darwin-x64+arm64/node.napi.node
    prebuilds/android-arm64/node.napi.armv8.node
    prebuilds/win32-ia32/node.napi.node
    prebuilds/linux-arm/node.napi.armv7.node
    prebuilds/linux-arm/node.napi.armv6.node
    prebuilds/linux-x64/node.napi.musl.node
    prebuilds/linux-x64/node.napi.glibc.node
    prebuilds/linux-arm64/node.napi.armv8.node
    prebuilds/win32-x64/node.napi.node

and running readelf -h on each will report many modules actually matching
the current machine. For example, those two will both match AArch64,
while obviously we only want to keep the latter:

    prebuilds/android-arm64/node.napi.armv8.node
    prebuilds/linux-arm64/node.napi.armv8.node

Same goes for arm, and similar goes for x64 musl vs. glibc.

Those last two are also very interesting: it means that modules need to
be built properly against the correct C library, not just the target
CPU.

Which means that, even a module matching the current arch is not
necessarily applicable to said arch if it was built for another C
library...

Which leads to the question: what happens for such a module when there
is no prebuilt binary for the current arch? Is it going to be compiled
by npm when it installs the module? Thanks to Adam, I could inspect the
tarball of @serial/bindings-cpp@12.0.1, and I could spot .cpp and h
files in the src/ subidr:

    $ wget 'https://registry.npmjs.org/@serialport/bindings-cpp/-/bindings-cpp-12.0.1.tgz'
    $ tar tzf bindings-cpp-12.0.1.tgz |grep src/
    package/src/darwin_list.cpp
    package/src/poller.cpp
    package/src/serialport_linux.cpp
    package/src/serialport_unix.cpp
    package/src/serialport_win.cpp
    package/src/serialport.cpp
    package/src/darwin_list.h
    package/src/poller.h
    package/src/serialport_linux.h
    package/src/serialport_unix.h
    package/src/serialport_win.h
    package/src/serialport.h

There's also binding.gyp that looks like it lists what to build under
which condition.

In which case, how does npm decide that it can use one such prebuild, or
whether it should compile from the source files? Can we force it to
never use prebuilds and always compile?

I'm afraid the proposed patch just covers up a bigger issue by sweeping
the dust under the rug... It will indeed make check-bin-arch happy
because no binary will indeed match another arch, indeed, but that does
not mean the module will be actually correctly installed...

So yes, npm is such a mess that maybe sweeping the dust under the rug is
the only solution we can implement, but it is disappointing...

Maybe we can just also exclude the node modules from check-bin-arch
altogether and be done with it:

    NODEJS_SRC_BIN_ARCH_EXCLUDE = /usr/lib/node_modules/

Regards,
Yann E. MORIN.

> Fixes:
> https://bugs.busybox.net/show_bug.cgi?id=15823
> 
> Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.com>
> ---
>  package/nodejs/nodejs-src/nodejs-src.mk | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/package/nodejs/nodejs-src/nodejs-src.mk b/package/nodejs/nodejs-src/nodejs-src.mk
> index 3452c93728..bdbf0709c6 100644
> --- a/package/nodejs/nodejs-src/nodejs-src.mk
> +++ b/package/nodejs/nodejs-src/nodejs-src.mk
> @@ -247,6 +247,18 @@ define NODEJS_SRC_INSTALL_MODULES
>  	# npm install call below and setting npm_config_rollback=false can both
>  	# help in diagnosing the problem.
>  	$(NPM) install -g $(NODEJS_SRC_MODULES_LIST)
> +
> +	# Remove prebuilt files which are not compatible with the architecture
> +	# and OS(Linux) we're building for. NOTE: .node files that don't have a
> +	# readelf output have different ABI(i.e. Windows, Darwin etc.)
> +	for f in $$(find $(TARGET_DIR)/usr/lib/node_modules -type f -name "*.node"); do \
> +		echo $$f; \
> +		arch=`$(TARGET_READELF) -h "$$f" 2>&1 | \
> +			sed -r -e '/^  Machine: +(.+)/!d; s//\1/;' | head -1`; \
> +		if [ "$$arch" != "$(BR2_READELF_ARCH_NAME)" ]; then \
> +			rm -f $$f; \
> +		fi \
> +	done
>  endef
>  endif
>  
> -- 
> 2.34.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

  reply	other threads:[~2023-11-01 15:21 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-10-31 12:07 [Buildroot] [PATCH] package/nodejs/nodejs-src: remove .node files with different architecture Giulio Benetti
2023-11-01 15:21 ` Yann E. MORIN [this message]
2023-11-02 23:16   ` Giulio Benetti
2023-11-02 23:43     ` Giulio Benetti
2023-11-03 21:21     ` Yann E. MORIN
2023-11-05 14:27       ` Giulio Benetti
2023-11-07 16:15         ` Yann E. MORIN
2023-11-07 20:08           ` Arnout Vandecappelle via buildroot
2023-11-07 20:40             ` Yann E. MORIN

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20231101152114.GE114892@scaer \
    --to=yann.morin.1998@free.fr \
    --cc=buildroot@buildroot.org \
    --cc=daniel.price@gmail.com \
    --cc=giulio.benetti@benettiengineering.com \
    --cc=martin@barkynet.com \
    --cc=thomas.petazzoni@bootlin.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox