-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
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
base: main
Are you sure you want to change the base?
Conversation
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.
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" ] |
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.
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
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.
selfisekai - to clarify, should this instead be:
if (use_system_llhttp) {
if (is_linux) {
pkg_config("llhttp") {
packages = [ "libllhttp" ]
}
}
libs += [ "llhttp" ]
}
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.
libs += [ ":llhttp" ]
, LGTM otherwise
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.
should be pkg_config on libllhttp
libs += [ "hdr_histogram" ] | ||
include_dirs += [ "/usr/include/hdr" ] |
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.
This also should be pkg_config on hdr_histogram
Co-authored-by: LN Liberda <msgh@selfisekai.rocks>
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 |
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.
What about Ada?
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'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
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.
That's educational. Thanks!
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.
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.
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 |
|
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.