Skip to content

build: allow unbundling of Node.js dependencies #55903

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

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

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Nov 18, 2024

Refs electron/electron#44691

Linux distributions have guidelines on using distro-provided dependencies, rather than compiling them in statically - Electron already did this via electron/electron#34841 until we ingested Node.js' GN support, which didn't contain this.

This PR this enables downstream packagers to unbundle these dependencies. We don't need to do this for zlib, as the existing gn workflow uses the same //third_party/zlib, so unbundling zlib with Chromium tools in //build/linux/unbundle works already. This adds support for some of the others.

@codebytere codebytere added the embedding Issues and PRs related to embedding Node.js in another project. label Nov 18, 2024
@codebytere codebytere requested a review from zcbenz November 18, 2024 08:57
@codebytere codebytere added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 18, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 18, 2024
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

RSLGTM

@@ -179,7 +178,17 @@ template("node_gn_build") {
configs -= [ "//build/config/gcc:symbol_visibility_hidden" ]
configs += [ "//build/config/gcc:symbol_visibility_default" ]
}

if (use_system_llhttp) {
libs += [ "llhttp" ]

Choose a reason for hiding this comment

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

This passes -lllhttp to the linker, but does not add its header location to include_dirs - pkg-config can resolve it under the name libllhttp

Copy link
Member Author

Choose a reason for hiding this comment

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

selfisekai - to clarify, should this instead be:

if (use_system_llhttp) {
   if (is_linux) {
      pkg_config("llhttp") {
        packages = [ "libllhttp" ]
      }
   }
   libs += [ "llhttp" ]
}

Choose a reason for hiding this comment

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

libs += [ ":llhttp" ], LGTM otherwise

Copy link
Contributor

Choose a reason for hiding this comment

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

should be pkg_config on libllhttp

Comment on lines +187 to +188
libs += [ "hdr_histogram" ]
include_dirs += [ "/usr/include/hdr" ]

Choose a reason for hiding this comment

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

This also should be pkg_config on hdr_histogram

Co-authored-by: LN Liberda <msgh@selfisekai.rocks>
@brjsp
Copy link
Contributor

brjsp commented Apr 19, 2025

You should also add ada-url, simdutf, simdjson, and sqlite3. Note that sqlite should be added in a subdir to keep the defines, like this:

Unbundle Node's sqlite copy. Note that this is NOT the sqlite used by Chromium, which is a fork.
(Yes, there are two instances of sqlite in Electron)

--- src/third_party/electron_node/deps/sqlite/unofficial.gni    2025-04-16 14:39:35.699035282 +0200
+++ /var/tmp/build-root/openSUSE_Tumbleweed-x86_64/home/abuild/rpmbuild/BUILD/nodejs-electron-35.1.5-build/src/third_party/electron_node/deps/sqlite/unofficial.gni       2025-04-18 15:46:13.417442111 +0200
@@ -1,3 +1,4 @@
+import("//build/config/linux/pkg_config.gni")
 # This file is used by GN for building, which is NOT the build system used for
 # building official binaries.
 # Please edit the gyp files if you are making changes to build system.
@@ -5,8 +6,8 @@
 # The actual configurations are put inside a template in unofficial.gni to
 # prevent accidental edits from contributors.
 template("sqlite_gn_build") {
-  config("sqlite_config") {
-    include_dirs = [ "." ]
+  pkg_config("sqlite_config") {
+    packages = ["sqlite3"]
     defines = [
       "SQLITE_ENABLE_MATH_FUNCTIONS",
       "SQLITE_ENABLE_SESSION",
@@ -14,28 +15,9 @@ template("sqlite_gn_build") {
     ]
   }

-  gypi_values = exec_script("../../tools/gypi_to_gn.py",
-                            [ rebase_path("sqlite.gyp") ],
-                            "scope",
-                            [ "sqlite.gyp" ])

   source_set(target_name) {
     forward_variables_from(invoker, "*")
     public_configs = [ ":sqlite_config" ]
-    sources = gypi_values.sqlite_sources
-    cflags_c = [
-      "-Wno-implicit-fallthrough",
-      "-Wno-unreachable-code-return",
-      "-Wno-unreachable-code-break",
-      "-Wno-unreachable-code",
-    ]
-    if (is_win) {
-      cflags_c += [
-        "-Wno-sign-compare",
-        "-Wno-unused-but-set-variable",
-        "-Wno-unused-function",
-        "-Wno-unused-variable",
-      ]
-    }
   }
 }

@@ -62,6 +62,12 @@ declare_args() {

# Build with Amaro (TypeScript utils).
node_use_amaro = true

# Allows downstream packagers (eg. Linux distributions) to build against system shared libraries.
use_system_cares = false
Copy link
Member

Choose a reason for hiding this comment

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

What about Ada?

Choose a reason for hiding this comment

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

I'd say it's a small user group because Ada uses C++ types that are not ABI-compatible between LLVM libc++ (linked statically by Chromium as default) and libstdc++ (used as the system one by most distros). use_custom_libcxx=false is very broken, and pretty much the only distros I know of using it currently are Chimera Linux (which uses libc++ as system, and patches libc++ to keep Chromium working), Debian, and openSUSE

Copy link
Member

Choose a reason for hiding this comment

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

That's educational. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify: openSUSE uses use_custom_libcxx=false for Electron (which i maintain), but not for Chromium (which i don't, and don't have resources to).

It's much more important that it stays usable for Electron, since requiring a custom C++ implementation would also require changes in packaging every Electron application we ship that has native modules.

@selfisekai
Copy link

For simdutf, I think the one in Chromium //third_party/simdutf should be reused in Electron, similar to zlib. That'd provide unbundling as well (https://chromium-review.googlesource.com/c/chromium/src/+/6333513). But that sounds like another PR to me

@brjsp
Copy link
Contributor

brjsp commented Apr 20, 2025

the one in Chromium //third_party/simdutf should be reused in Electron
@selfisekai as of Electron 35 this seems to be already done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
embedding Issues and PRs related to embedding Node.js in another project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants