Skip to content

build: allow unbundling of Node.js dependencies - #55903

Closed
codebytere wants to merge 2 commits into
nodejs:mainfrom
codebytere:upstream-node-bundle-defines
Closed

build: allow unbundling of Node.js dependencies#55903
codebytere wants to merge 2 commits into
nodejs:mainfrom
codebytere:upstream-node-bundle-defines

Conversation

@codebytere

@codebyterecodebytere commented Nov 18, 2024

Copy link
Copy Markdown
Member

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.

@codebyterecodebytere added the embedding Issues and PRs related to embedding Node.js in another project. label Nov 18, 2024
@codebyterecodebytere added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 18, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 18, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@lpincalpinca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

RSLGTM

Comment threadunofficial.gni
}

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

Copy link
Copy Markdown

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
Copy Markdown
MemberAuthor

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" ]
}

Copy link
Copy Markdown

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
Copy Markdown
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 threadunofficial.gni
Comment on lines +187 to +188
libs += [ "hdr_histogram" ]
include_dirs += [ "/usr/include/hdr" ]

Copy link
Copy Markdown

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

Comment threadunofficial.gni Outdated
Co-authored-by: LN Liberda <msgh@selfisekai.rocks>
@brjsp

brjsp commented Apr 19, 2025

Copy link
Copy Markdown
Contributor

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",- ]- }
}
}

Comment threadnode.gni
node_use_amaro = true

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What about Ada?

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
Contributor

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

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

@github-actionsgithub-actionsBot added the stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure. label Jul 28, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

embeddingIssues and PRs related to embedding Node.js in another project.staleIssues and PRs marked stale due to inactivity and scheduled for automatic closure.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@codebytere@nodejs-github-bot@brjsp@selfisekai@lpinca@anonrig@avivkeller