GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — src: fix dynamically linked zlib version by richardlau · Pull Request #51007 · nodejs/node

Conversation

richardlau commented Dec 1, 2023

Copy link

Copy Markdown

Member

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.

Refs: #50910 (comment)
Refs: #50158

This should unblock #50910 by fixing the regression caused by #50158.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          src: fix dynamically linked zlib version

6ad7778

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`.

nodejs-github-bot commented Dec 1, 2023

Copy link

Copy Markdown

Collaborator

Review requested:

@nodejs/gyp

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot added build

Issues and PRs related to Node.js builds or CI infrastructure.

c++

Issues and PRs that require attention from people who are familiar with C++.

needs-ci PRs that need a full CI run. labels

Dec 1, 2023

richardlau added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Dec 1, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Dec 1, 2023

nodejs-github-bot commented Dec 1, 2023

Copy link

Copy Markdown

Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/56041/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

richardlau commented Dec 1, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member Author

So this fixes the immediate bug -- prior to this PR process.versions.zlib would incorrectly report 1.2.13.1-motley-5daffc7 which is the value in src/zlib_version.h that was set the last time deps/zlib was updated.

One interesting observation is that, after building with --shared-zlib if I run with a different (but ABI compatible) version of zlib, process.versions still reports the version linked against but process.report.getReport().header.componentVersions shows the expected version.
e.g.

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 LD_LIBRARY_PATH to pick up a different version of zlib (1.2.11):

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. process.report seems to be correct but process.versions is not. Are the versions in process.versions coming from the snapshot? cc @joyeecheung

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

richardlau mentioned this pull request

Dec 1, 2023

deps: update zlib to 1.3-22124f5 #50910

Merged

kapouer commented Dec 1, 2023

Copy link

Copy Markdown

Contributor

@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).
For example at build time I do this ugly stuff.

richardlau reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

joyeecheung commented Dec 2, 2023

Copy link

Copy Markdown

Member

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)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lpinca approved these changes

Dec 2, 2023

View reviewed changes

richardlau added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label

Dec 3, 2023

mhdawson approved these changes

Dec 4, 2023

View reviewed changes

mhdawson left a comment

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Dec 4, 2023

richardlau added commit-queue PRs queued for automated landing through the Commit Queue. and removed request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels

Dec 4, 2023

nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label

Dec 4, 2023

nodejs-github-bot merged commit 7b3eb31 into nodejs:main

Dec 4, 2023

nodejs-github-bot commented Dec 4, 2023

Copy link

Copy Markdown

Collaborator

Landed in 7b3eb31

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

richardlau deleted the zlibver branch

December 5, 2023 17:33

RafaelGSS pushed a commit that referenced this pull request

Dec 15, 2023


          src: fix dynamically linked zlib version

f90fc83

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]>

RafaelGSS mentioned this pull request

Dec 15, 2023

v21.5.0 proposal #51166

Merged

richardlau added a commit that referenced this pull request

Mar 25, 2024


          src: fix dynamically linked zlib version

97aa67f

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]>

richardlau mentioned this pull request

Mar 25, 2024

v20.12.0 proposal #52212

Merged

This was referenced Jun 12, 2024

test: update TLS tests for OpenSSL 3.2 #53384

Merged

src: reset process.versions during pre-execution #53444

Merged

Reviewers

lpinca

lpinca approved these changes

+1 more reviewer

mhdawson

mhdawson approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready

PRs with CI started, the required approvals, and no outstanding review comments.

build

Issues and PRs related to Node.js builds or CI infrastructure.

c++

Issues and PRs that require attention from people who are familiar with C++.

needs-ci

PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

richardlau commented Dec 1, 2023

Uh oh!

nodejs-github-bot commented Dec 1, 2023

Uh oh!

nodejs-github-bot commented Dec 1, 2023

Uh oh!

richardlau commented Dec 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kapouer commented Dec 1, 2023

Uh oh!

joyeecheung commented Dec 2, 2023

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 4, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

richardlau commented Dec 1, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.