Nodejs build system incorrectly prepending zlib include path to small-icu includes.

[jameshilliard]

• Version: 12.14.1(bug appears to be in master as well)
• Platform: buildroot master
• Subsystem: small-icu

What steps will reproduce the bug?

attempt to build nodejs with small-icu and a shared zlib on a system that also contains a system icu with both zlib and icu headers in the /include directory

See here for buildroot build log failure.

How often does it reproduce? Is there a required condition?

always when building with small-icu and shared zlib on a system with zlib and icu headers in the system include path

What is the expected behavior?

nodejs will use icu headers from small-icu instead of system icu headers when small-icu is selected

What do you see instead?

nodejs attempts to use system-icu headers due to zlib include directory being prepended to small-icu include directories

Additional information

this bug is due to nodejs passing all include directories from config.gypi before the small-icu includes to the compiler which causes the compiler to try and use the incompatible system icu headers instead of the built in small-icu headers

[joyeecheung]

cc @nodejs/build-files

[mhdawson]

@srl295 FYI

[srl295]

What version of ICU is on the host? it would be evident in the file /home/naourr/work/instance-2/output-1/host/include/unicode/numfmt.h

also do you have the configure parameters handy?

[srl295]

This could be tricky, though, if it was something ICU depended on (as ICU doesn't depend on zlib). (which version of zlib was installed in the system?)

At least, ICU's own headers should come earlier in the search path.

[jameshilliard]

What version of ICU is on the host?

65-1, but it shouldn't matter as nodejs should not be using it at all

also do you have the configure parameters handy?

Yeah, it's these.

[jameshilliard]

This could be tricky, though, if it was something ICU depended on (as ICU doesn't depend on zlib). (which version of zlib was installed in the system?)

No, that's not the issue, it's just that the build system prepends the zlib include path(which also contains system ICU headers) before the small-icu built in headers include path that should be used.

At least, ICU's own headers should come earlier in the search path.

If this is fixed it should work fine, the problem is the zlib include path is prepended to the ICU path.

[srl295]

Oh, hi buildroot person! 👍

65-1, but it shouldn't matter as nodejs should not be using it at all

It shouldn't, but I want to be able to repro this.

Node v12.14.1 is on ICU 64.x per https://github.com/nodejs/node/blob/v12.14.1/deps/icu-small/source/common/unicode/uvernum.h#L63

[jameshilliard]

Right, and since buildroot is on 65.x the headers are incompatible. Easiest way to reproduce is clone buildroot master enable ICU and nodejs packages and then try building.

[srl295]

OK, so… can I get someone maybe from @nodejs/build-files to take a look here?

here's a hack-in-progress that changes configure_library() to use scoped variables. ( but I think I need someone else to pick this up.. )

./configure --shared-zlib --shared-zlib-includes=/src/II/include/ --shared-zlib-libpath=/src/II/lib/

yielding:

# Do not edit. Generated by the configure script.
{ 'target_defaults': { 'cflags': [],
                       'default_configuration': 'Release',
                       'defines': [],
                       'include_dirs': [],
                       'include_dirs_shared_zlib': ['/src/II/include/'],
                       'libraries': [],
                       'libraries_shared_zlib': ['-L/src/II/lib/', '-lz']},
}

diff:

diff --git a/configure.py b/configure.py
index 20cce214db..860aca3095 100755
--- a/configure.py
+++ b/configure.py
@@ -1146,10 +1146,14 @@ def configure_library(lib, output, pkgname=None):
         pkg_config(pkgname or lib))
 
     if options.__dict__[shared_lib + '_includes']:
-      output['include_dirs'] += [options.__dict__[shared_lib + '_includes']]
+      if 'include_dirs_' + shared_lib not in output:
+        output['include_dirs_' + shared_lib] = []
+      output['include_dirs_' + shared_lib] += [options.__dict__[shared_lib + '_includes']]
     elif pkg_cflags:
+      if 'include_dirs_' + shared_lib not in output:
+        output['include_dirs_' + shared_lib] = []
       stripped_flags = [flag.strip() for flag in pkg_cflags.split('-I')]
-      output['include_dirs'] += [flag for flag in stripped_flags if flag]
+      output['include_dirs_' + shared_lib] += [flag for flag in stripped_flags if flag]