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

node: Do not use shared abseil-cpp #25582

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vidplace7
Copy link
Contributor

Maintainer: @nxhack
Compile tested: OpenWRT One master/snapshot

Description:
Fix nodejs compile errors after the introduction of abseil-cpp in #25565

Extends the existing gyp_tools patch to force v8 to use the bundled (chromium) abseil-cpp.

Resolves #25483

Thanks to @vortexilation for pointing out this issue.

@nxhack
Copy link
Contributor

nxhack commented Dec 20, 2024

Hi @vidplace7

Question.

The source code distributed by node.js includes abseil-cpp.

. /deps/v8/third_party/abseil-cpp

It appears to be used in the current package.

. /out/Release/obj.host/v8_abseil
. /out/Release/obj.host/v8_abseil/deps/v8/third_party/abseil-cpp
. /out/Release/obj.host/tools/v8_gypfiles/libv8_abseil.a.ar-file-list
. /out/Release/obj.host/tools/v8_gypfiles/libv8_abseil.a
. /out/Release/obj.target/v8_abseil
. /out/Release/obj.target/v8_abseil/deps/v8/third_party/abseil-cpp
. /out/Release/obj.target/tools/v8_gypfiles/libv8_abseil.a.ar-file-list
. /out/Release/obj.target/tools/v8_gypfiles/libv8_abseil.a

Please tell us why this modification is necessary for this package.

@vidplace7
Copy link
Contributor Author

vidplace7 commented Dec 20, 2024

@nxhack a shared abseil-cpp package was introduced in #25565, for use in protobuf (and other packages that use the standard abseil-cpp LTS version / don't bundle it themselves).

Without this patch, if the shared abseil-cpp library package is installed to the staging_dir before node is compiled, node (v8) will attempt to use it, and fail spectacularly (as it's a different variant/version).

@nxhack
Copy link
Contributor

nxhack commented Dec 20, 2024

Understood.
I will now test to see if it builds correctly on all architectures. Please give me some time.

@nxhack
Copy link
Contributor

nxhack commented Dec 20, 2024

It seems that the host build of node.js fails when abseil-cpp is installed. We will consider a workaround.
It fails regardless of whether or not hostpkg abseil-cpp is installed.

@nxhack
Copy link
Contributor

nxhack commented Dec 20, 2024

It's a very dirty patch, but it worked!

diff --git a/lang/node/patches/204-v8_gyp.patch b/lang/node/patches/204-v8_gyp.patch
index 8496a7c71..0a12e67b8 100644
--- a/lang/node/patches/204-v8_gyp.patch
+++ b/lang/node/patches/204-v8_gyp.patch
@@ -28,7 +28,7 @@
        'sources': [
          '<(V8_ROOT)/src/init/setup-isolate-full.cc',
        ],
-+      'include_dirs': [ '<!@(echo "$STAGING_DIR"/usr/../usr/include)' ],
++      'include_dirs': [ '../../deps/v8/third_party/abseil-cpp', '<!@(echo "$STAGING_DIR"/usr/../usr/include)' ],
      },  # v8_init
      {
        # This target is used to work around a GCC issue that causes the
@@ -40,7 +40,7 @@
        'include_dirs': [
          '<(SHARED_INTERMEDIATE_DIR)',
          '<(generate_bytecode_output_root)',
-+        '<!@(echo "$STAGING_DIR"/usr/../usr/include)',
++        '../../deps/v8/third_party/abseil-cpp', '<!@(echo "$STAGING_DIR"/usr/../usr/include)',
        ],
        'sources': [
          '<!@pymod_do_main(GN-scraper "<(V8_ROOT)/BUILD.gn"  "\\"v8_initializers.*?sources = ")',
@@ -48,7 +48,7 @@
        'toolsets': ['host', 'target'],
        'direct_dependent_settings': {
          'sources': ['<!@pymod_do_main(GN-scraper "<(V8_ROOT)/BUILD.gn"  "v8_compiler_sources = ")'],
-+        'include_dirs': [ '<!@(echo "$STAGING_DIR"/usr/../usr/include)' ],
++        'include_dirs': [ '../../deps/v8/third_party/abseil-cpp', '<!@(echo "$STAGING_DIR"/usr/../usr/include)' ],
          'conditions': [
            ['v8_target_arch=="ia32"', {
              'sources': [
@@ -57,7 +57,7 @@
        'type': 'static_library',
        'toolsets': ['host', 'target'],
 +      'libraries!':[ '-lcrypto', '-lssl', '-lz', '-lhttp_parser', '-luv', '-lnghttp2', '-lcares' ],
-+      'include_dirs': [ '<!@(echo "$STAGING_DIR"/usr/../usr/include)' ],
++      'include_dirs': [ '../../deps/v8/third_party/abseil-cpp', '<!@(echo "$STAGING_DIR"/usr/../usr/include)' ],
        'dependencies': [
          'generate_bytecode_builtins_list',
          'run_torque',
@@ -65,7 +65,7 @@
          'run_torque',
          'v8_maybe_icu',
        ],
-+      'include_dirs': [ '<!@(echo "$STAGING_DIR"/usr/../usr/include)' ],
++      'include_dirs': [ '../../deps/v8/third_party/abseil-cpp', '<!@(echo "$STAGING_DIR"/usr/../usr/include)' ],
        'conditions': [
          ['(is_component_build and not v8_optimized_debug and v8_enable_fast_mksnapshot) or v8_enable_turbofan==0', {
            'dependencies': [
@@ -99,7 +99,7 @@
        'type': 'executable',
 +      'libraries!':[ '-lcrypto', '-lssl', '-lz', '-lhttp_parser', '-luv', '-lnghttp2', '-lcares' ],
 +      'library_dirs':[ '../../../../staging_dir/hostpkg/share/icu/current/lib' ],
-+      'include_dirs': [ '<!@(echo "$STAGING_DIR"/usr/../usr/include)' ],
++      'include_dirs': [ '../../deps/v8/third_party/abseil-cpp', '<!@(echo "$STAGING_DIR"/usr/../usr/include)' ],
        'dependencies': [
          'v8_base_without_compiler',
          'v8_compiler_for_mksnapshot',
@@ -144,7 +144,7 @@
          'include_dirs': [
            '<(V8_ROOT)/third_party/zlib',
            '<(V8_ROOT)/third_party/zlib/google',
-+          '<!@(echo "$STAGING_DIR"/usr/../usr/include)',
++          '../../deps/v8/third_party/abseil-cpp', '<!@(echo "$STAGING_DIR"/usr/../usr/include)',
          ],
        },
        'defines': [ 'ZLIB_IMPLEMENTATION' ],
@@ -152,7 +152,21 @@
        'include_dirs': [
          '<(V8_ROOT)/third_party/zlib',
          '<(V8_ROOT)/third_party/zlib/google',
-+        '<!@(echo "$STAGING_DIR"/usr/../usr/include)',
++        '../../deps/v8/third_party/abseil-cpp', '<!@(echo "$STAGING_DIR"/usr/../usr/include)',
        ],
        'sources': [
          '<(V8_ROOT)/third_party/zlib/adler32.c',
+@@ -2229,10 +2254,13 @@
+         'ABSEIL_ROOT': '../../deps/v8/third_party/abseil-cpp',
+       },
+       'direct_dependent_settings': {
++        'include_dirs!': [ '<!@(echo "$STAGING_DIR"/usr/include)' ],
+         'include_dirs': [
+           '<(ABSEIL_ROOT)',
++          '<!@(echo "$STAGING_DIR"/usr/../usr/include)',
+         ],
+       },
++      'include_dirs!': [ '<!@(echo "$STAGING_DIR"/usr/include)' ],
+       'include_dirs': [
+         '<(ABSEIL_ROOT)',
+       ],

I will resume testing.

@vortexilation
Copy link
Contributor

This PR seems doesn't work but patch above by @nxhack is working.

@vidplace7 vidplace7 marked this pull request as draft December 20, 2024 15:51
@nxhack
Copy link
Contributor

nxhack commented Dec 20, 2024

I have confirmed that it builds and runs on aarch64, arm, i386, x86_64 with my patch applied.

@vortexilation
Copy link
Contributor

Hmm, can you please try with glibc on x86_64?

@nxhack
Copy link
Contributor

nxhack commented Dec 21, 2024

@vortexilation

I am verifying a simplified version of the above patch. musl does not give me any problems at all, but glibc gives me the following error. I will consider a countermeasure.

../deps/v8/third_party/abseil-cpp/absl/container/internal/hashtablez_sampler.cc:75:6: error: no declaration matches 'void absl::lts_20240722::container_internal::HashtablezInfo::PrepareForSampling(int64_t, size_t)'
   75 | void HashtablezInfo::PrepareForSampling(int64_t stride,
      |      ^~~~~~~~~~~~~~
In file included from ../deps/v8/third_party/abseil-cpp/absl/container/internal/hashtablez_sampler.cc:15:
../../../../staging_dir/target-x86_64_glibc/usr/include/absl/container/internal/hashtablez_sampler.h:75:8: note: candidate is: 'void absl::lts_20240722::container_internal::HashtablezInfo::PrepareForSampling(int64_t, size_t, size_t, size_t, uint16_t '
   75 |   void PrepareForSampling(int64_t stride, size_t inline_element_size_value,
      |        ^~~~~~~~~~~~~~~~~~
../../../../staging_dir/target-x86_64_glibc/usr/include/absl/container/internal/hashtablez_sampler.h:66:8: note: 'struct absl::lts_20240722::container_internal::HashtablezInfo' defined here
   66 | struct HashtablezInfo : public profiling_internal::Sample<HashtablezInfo> {
      |        ^~~~~~~~~~~~~~
In file included from ../../../../staging_dir/target-x86_64_glibc/usr/include/absl/container/internal/hashtablez_sampler.h:5 :
../../../../staging_dir/target-x86_64_glibc/usr/include/absl/profiling/internal/sample_recorder.h: In instantiation of 'T* absl::lts_20240722::profiling_internal::SampleRecorder<T>::Register(Targs&& ...) [with Targs = {const long int&, long unsigned int&}; T = absl::lts_20240722::container_internal::HashtablezInfo]':
../deps/v8/third_party/abseil-cpp/absl/container/internal/hashtablez_sampler.cc:124:43:   required from here
../../../../staging_dir/target-x86_64_glibc/usr/include/absl/profiling/internal/sample_recorder.h:210:33: error: no matching function for call to 'absl::lts_20240722::container_internal::HashtablezInfo::PrepareForSampling(const long int&, long unsigned int&)'
  210 |       sample->PrepareForSampling(std::forward<Targs>(args)...);
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../../staging_dir/target-x86_64_glibc/usr/include/absl/container/internal/hashtablez_sampler.h:75:8: note: candidate:  void absl::lts_20240722::container_internal::HashtablezInfo::PrepareForSampling(int64_t, size_t, size_t, size_t, uint16_t)'
   75 |   void PrepareForSampling(int64_t stride, size_t inline_element_size_value,
      |        ^~~~~~~~~~~~~~~~~~
../../../../staging_dir/target-x86_64_glibc/usr/include/absl/container/internal/hashtablez_sampler.h:75:8: note:   candidate expects 5 arguments, 2 provided
../../../../staging_dir/target-x86_64_glibc/usr/include/absl/profiling/internal/sample_recorder.h: In instantiation of 'T* absl::lts_20240722::profiling_internal::SampleRecorder<T>::PopDead(Targs ...) [with Targs = {long int, long unsigned int}; T = absl::lts_20240722::container_internal::HashtablezInfo]':
../../../../staging_dir/target-x86_64_glibc/usr/include/absl/profiling/internal/sample_recorder.h:196:22:   required from 'T* absl::lts_20240722::profiling_internal::SampleRecorder<T>::Register(Targs&& ...) [with Targs = {const long int&, long unsigned int&}; T = absl::lts_20240722::container_internal::HashtablezInfo]'
../deps/v8/third_party/abseil-cpp/absl/container/internal/hashtablez_sampler.cc:124:43:   required from here
../../../../staging_dir/target-x86_64_glibc/usr/include/absl/profiling/internal/sample_recorder.h:182:29: error: no matching function for call to 'absl::lts_20240722::container_internal::HashtablezInfo::PrepareForSampling(long int, long unsigned int '
  182 |   sample->PrepareForSampling(std::forward<Targs>(args)...);
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../../staging_dir/target-x86_64_glibc/usr/include/absl/container/internal/hashtablez_sampler.h:75:8: note: candidate:  void absl::lts_20240722::container_internal::HashtablezInfo::PrepareForSampling(int64_t, size_t, size_t, size_t, uint16_t)'
   75 |   void PrepareForSampling(int64_t stride, size_t inline_element_size_value,
      |        ^~~~~~~~~~~~~~~~~~
../../../../staging_dir/target-x86_64_glibc/usr/include/absl/container/internal/hashtablez_sampler.h:75:8: note:   candidate expects 5 arguments, 2 provided
make[4]: *** [tools/v8_gypfiles/v8_abseil.host.mk:263: /mnt/glibc/openwrt/build_dir/target-x86_64_glibc/node-v22.11.0/out/Release/obj.host/v8_abseil/deps/v8/third_party/abseil-cpp/absl/container/internal/hashtablez_sampler.o] Error 1

@nxhack
Copy link
Contributor

nxhack commented Dec 21, 2024

With this patch, glibc seems to be ok.
I will test it again. It will take some time.

diff --git a/lang/node/patches/204-v8_gyp.patch b/lang/node/patches/204-v8_gyp.patch
index 8496a7c71..5dba48a4f 100644
--- a/lang/node/patches/204-v8_gyp.patch
+++ b/lang/node/patches/204-v8_gyp.patch
@@ -28,7 +28,7 @@
        'sources': [
          '<(V8_ROOT)/src/init/setup-isolate-full.cc',
        ],
-+      'include_dirs': [ '<!@(echo "$STAGING_DIR"/usr/../usr/include)' ],
++      'include_dirs': [ '../../deps/v8/third_party/abseil-cpp', '<!@(echo "$STAGING_DIR"/usr/../usr/include)' ],
      },  # v8_init
      {
        # This target is used to work around a GCC issue that causes the
@@ -40,7 +40,7 @@
        'include_dirs': [
          '<(SHARED_INTERMEDIATE_DIR)',
          '<(generate_bytecode_output_root)',
-+        '<!@(echo "$STAGING_DIR"/usr/../usr/include)',
++        '../../deps/v8/third_party/abseil-cpp', '<!@(echo "$STAGING_DIR"/usr/../usr/include)',
        ],
        'sources': [
          '<!@pymod_do_main(GN-scraper "<(V8_ROOT)/BUILD.gn"  "\\"v8_initializers.*?sources = ")',
@@ -48,7 +48,7 @@
        'toolsets': ['host', 'target'],
        'direct_dependent_settings': {
          'sources': ['<!@pymod_do_main(GN-scraper "<(V8_ROOT)/BUILD.gn"  "v8_compiler_sources = ")'],
-+        'include_dirs': [ '<!@(echo "$STAGING_DIR"/usr/../usr/include)' ],
++        'include_dirs': [ '../../deps/v8/third_party/abseil-cpp', '<!@(echo "$STAGING_DIR"/usr/../usr/include)' ],
          'conditions': [
            ['v8_target_arch=="ia32"', {
              'sources': [
@@ -57,7 +57,7 @@
        'type': 'static_library',
        'toolsets': ['host', 'target'],
 +      'libraries!':[ '-lcrypto', '-lssl', '-lz', '-lhttp_parser', '-luv', '-lnghttp2', '-lcares' ],
-+      'include_dirs': [ '<!@(echo "$STAGING_DIR"/usr/../usr/include)' ],
++      'include_dirs': [ '../../deps/v8/third_party/abseil-cpp', '<!@(echo "$STAGING_DIR"/usr/../usr/include)' ],
        'dependencies': [
          'generate_bytecode_builtins_list',
          'run_torque',
@@ -65,7 +65,7 @@
          'run_torque',
          'v8_maybe_icu',
        ],
-+      'include_dirs': [ '<!@(echo "$STAGING_DIR"/usr/../usr/include)' ],
++      'include_dirs': [ '../../deps/v8/third_party/abseil-cpp', '<!@(echo "$STAGING_DIR"/usr/../usr/include)' ],
        'conditions': [
          ['(is_component_build and not v8_optimized_debug and v8_enable_fast_mksnapshot) or v8_enable_turbofan==0', {
            'dependencies': [
@@ -99,7 +99,7 @@
        'type': 'executable',
 +      'libraries!':[ '-lcrypto', '-lssl', '-lz', '-lhttp_parser', '-luv', '-lnghttp2', '-lcares' ],
 +      'library_dirs':[ '../../../../staging_dir/hostpkg/share/icu/current/lib' ],
-+      'include_dirs': [ '<!@(echo "$STAGING_DIR"/usr/../usr/include)' ],
++      'include_dirs': [ '../../deps/v8/third_party/abseil-cpp', '<!@(echo "$STAGING_DIR"/usr/../usr/include)' ],
        'dependencies': [
          'v8_base_without_compiler',
          'v8_compiler_for_mksnapshot',
@@ -144,7 +144,7 @@
          'include_dirs': [
            '<(V8_ROOT)/third_party/zlib',
            '<(V8_ROOT)/third_party/zlib/google',
-+          '<!@(echo "$STAGING_DIR"/usr/../usr/include)',
++          '../../deps/v8/third_party/abseil-cpp', '<!@(echo "$STAGING_DIR"/usr/../usr/include)',
          ],
        },
        'defines': [ 'ZLIB_IMPLEMENTATION' ],
@@ -152,7 +152,15 @@
        'include_dirs': [
          '<(V8_ROOT)/third_party/zlib',
          '<(V8_ROOT)/third_party/zlib/google',
-+        '<!@(echo "$STAGING_DIR"/usr/../usr/include)',
++        '../../deps/v8/third_party/abseil-cpp', '<!@(echo "$STAGING_DIR"/usr/../usr/include)',
        ],
        'sources': [
          '<(V8_ROOT)/third_party/zlib/adler32.c',
+@@ -2228,6 +2253,7 @@
+       'variables': {
+         'ABSEIL_ROOT': '../../deps/v8/third_party/abseil-cpp',
+       },
++      'include_dirs!': [ '<!@(echo "$STAGING_DIR"/usr/include)' ],
+       'direct_dependent_settings': {
+         'include_dirs': [
+           '<(ABSEIL_ROOT)',

@vortexilation
Copy link
Contributor

vortexilation commented Dec 21, 2024

@nxhack
Your first patch is working for me, also the newest one (glibc). Thanks.

@nxhack
Copy link
Contributor

nxhack commented Dec 22, 2024

I have applied my recent patch and confirmed it works on aarch64, arm, i386, x86_64 and x86_64+glibc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

node: undefined reference to absl::lts_20230802
3 participants