-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
src: fix dynamically linked zlib version #51007
Conversation
Report the version of the dynamically linked zlib if built with `configure --shared-zlib` instead of the hardcoded version that corresponds to the bundled version of zlib in `deps`.
Review requested:
|
So this fixes the immediate bug -- prior to this PR One interesting observation is that, after building with iojs@bd40e8f13f4d:/node$ LD_LIBRARY_PATH=/opt/zlib_1.2.12/lib/ ./node -p process.config.target_defaults
{
cflags: [],
default_configuration: 'Release',
defines: [
'NODE_OPENSSL_CONF_NAME=nodejs_conf',
'NODE_OPENSSL_HAS_QUIC',
'ICU_NO_USER_DATA_OVERRIDE'
],
include_dirs: [ '/opt/zlib_1.2.12/include/' ],
libraries: [ '-L/opt/zlib_1.2.12/lib/', '-lz' ]
}
iojs@bd40e8f13f4d:/node$ LD_LIBRARY_PATH=/opt/zlib_1.2.12/lib/ ./node -p process.versions.zlib
1.2.12
iojs@bd40e8f13f4d:/node$ LD_LIBRARY_PATH=/opt/zlib_1.2.12/lib/ ./node -p "process.report.getReport().header.componentVersions.zlib"
1.2.12
iojs@bd40e8f13f4d:/node$ LD_LIBRARY_PATH=/opt/zlib_1.2.12/lib/ ./node -p "process.report.getReport().sharedObjects"
[
'linux-vdso.so.1',
'/opt/zlib_1.2.12/lib/libz.so.1',
'/lib/x86_64-linux-gnu/libdl.so.2',
'/lib/x86_64-linux-gnu/libstdc++.so.6',
'/lib/x86_64-linux-gnu/libm.so.6',
'/lib/x86_64-linux-gnu/libgcc_s.so.1',
'/lib/x86_64-linux-gnu/libpthread.so.0',
'/lib/x86_64-linux-gnu/libc.so.6',
'/lib64/ld-linux-x86-64.so.2'
]
iojs@bd40e8f13f4d:/node$ which so far is consistent, but when changing iojs@bd40e8f13f4d:/node$ LD_LIBRARY_PATH=/lib/x86_64-linux-gnu/ ./node -p process.versions.zlib
1.2.12
iojs@bd40e8f13f4d:/node$ LD_LIBRARY_PATH=/lib/x86_64-linux-gnu/ ./node -p "process.report.getReport().header.componentVersions.zlib"
1.2.11
iojs@bd40e8f13f4d:/node$ LD_LIBRARY_PATH=/lib/x86_64-linux-gnu/ ./node -p "process.report.getReport().sharedObjects"
[
'linux-vdso.so.1',
'/lib/x86_64-linux-gnu/libz.so.1',
'/lib/x86_64-linux-gnu/libdl.so.2',
'/lib/x86_64-linux-gnu/libstdc++.so.6',
'/lib/x86_64-linux-gnu/libm.so.6',
'/lib/x86_64-linux-gnu/libgcc_s.so.1',
'/lib/x86_64-linux-gnu/libpthread.so.0',
'/lib/x86_64-linux-gnu/libc.so.6',
'/lib64/ld-linux-x86-64.so.2'
]
iojs@bd40e8f13f4d:/node$ i.e. |
@richardlau serendipitously, I have a similar problem, as a distributor, when embedding builtins from system source at build time (the ones officially supported + the ones I added manually, acorn &al). |
Yes I think process.versions is captured in the snapshot at build time. If this needs to be dynamic (or any other) I think we can just reset the versions at pre-execution (something like if the process.config indicates that some library is dynamically linked, reset the version retrieved from C++ land) |
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.
LGTM
Landed in 7b3eb31 |
Report the version of the dynamically linked zlib if built with `configure --shared-zlib` instead of the hardcoded version that corresponds to the bundled version of zlib in `deps`. PR-URL: #51007 Refs: #50910 Refs: #50158 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Report the version of the dynamically linked zlib if built with `configure --shared-zlib` instead of the hardcoded version that corresponds to the bundled version of zlib in `deps`. PR-URL: #51007 Refs: #50910 Refs: #50158 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Report the version of the dynamically linked zlib if built with
configure --shared-zlib
instead of the hardcoded version that corresponds to the bundled version of zlib indeps
.Refs: #50910 (comment)
Refs: #50158
This should unblock #50910 by fixing the regression caused by #50158.