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

Bundled zlib is compiled despite the --shared-zlib configure-flag #10649

Closed
UnitedMarsupials-zz opened this issue Jan 6, 2017 · 2 comments
Closed
Labels
build Issues and PRs related to build files or the CI. zlib Issues and PRs related to the zlib subsystem.

Comments

@UnitedMarsupials-zz
Copy link

All operating systems out there -- except, perhaps, Windows -- already include libz. However, when building node with the --shared-zlib flag, the bundled version of the library is compiled anyway -- even if the node-executable duly uses the OS-provded library (/lib/libz.so.6 in my case).

This is both wasteful and dangerous, because some day it may happen, that a bundled header-file will be #include-ed, that will somehow disagree with the implementation provided by the OS. It should be possible to build node with deps/zlib completely absent -- for safety.

Other bundled libraries (cares, uv) do not seem to be mishandled this way -- only zlib.

@addaleax addaleax added build Issues and PRs related to build files or the CI. zlib Issues and PRs related to the zlib subsystem. labels Jan 6, 2017
@bnoordhuis
Copy link
Member

The bundled zlib is used by the C++ test suite, that is why. The fix is probably this:

diff --git a/node.gyp b/node.gyp
index abba4c3..5075c51 100644
--- a/node.gyp
+++ b/node.gyp
@@ -895,29 +895,33 @@
       ],
 
       'conditions': [
         ['v8_inspector=="true"', {
           'defines': [
             'HAVE_INSPECTOR=1',
           ],
           'dependencies': [
-            'deps/zlib/zlib.gyp:zlib',
             'v8_inspector_compress_protocol_json#host'
           ],
           'include_dirs': [
             '<(SHARED_INTERMEDIATE_DIR)'
           ],
           'sources': [
             'src/inspector_socket.cc',
             'src/inspector_socket_server.cc',
             'test/cctest/test_inspector_socket.cc',
             'test/cctest/test_inspector_socket_server.cc'
           ],
           'conditions': [
+            [ 'node_shared_zlib=="false"', {
+              'dependencies': [
+                'deps/zlib/zlib.gyp:zlib',
+              ]
+            }],
             [ 'node_shared_openssl=="false"', {
               'dependencies': [
                 'deps/openssl/openssl.gyp:openssl'
               ]
             }],
             [ 'node_shared_http_parser=="false"', {
               'dependencies': [
                 'deps/http_parser/http_parser.gyp:http_parser'

Contributors, feel free to steal. I'm not going to PR it, I'm on holiday!

@gibfahn
Copy link
Member

gibfahn commented Jan 6, 2017

Will test/steal ^

gibfahn added a commit to gibfahn/node that referenced this issue Jan 29, 2017
Even if the --shared-zlib flag was used, the bundled deps/zlib was still
being compiled into the binary as it was required by the C++ test suite.

PR-URL: nodejs#10657
Fixes: nodejs#10649
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
evanlucas pushed a commit that referenced this issue Jan 31, 2017
Even if the --shared-zlib flag was used, the bundled deps/zlib was still
being compiled into the binary as it was required by the C++ test suite.

PR-URL: #10657
Fixes: #10649
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants