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

process.versions.zlib does not include the commit hash #50139

Closed
panva opened this issue Oct 11, 2023 · 6 comments · Fixed by #50158
Closed

process.versions.zlib does not include the commit hash #50139

panva opened this issue Oct 11, 2023 · 6 comments · Fixed by #50158
Labels
dependencies Pull requests that update a dependency file. zlib Issues and PRs related to the zlib subsystem.

Comments

@panva
Copy link
Member

panva commented Oct 11, 2023

Despite upgrade to the bundled zlib library in #50085 the process.versions.zlib output remains the same.

node -p 'process.versions.zlib'
1.2.13.1-motley
node -v
v18.18.1
./node -p 'process.versions.zlib'
1.2.13.1-motley
./node -v
v21.0.0-pre

The expected output of process.versions.zlib would be e.g. 1.2.13.1-motley-f5fd0ad, that is including the commit hash from upstream.

@panva panva added zlib Issues and PRs related to the zlib subsystem. dependencies Pull requests that update a dependency file. labels Oct 11, 2023
@MrJithil
Copy link
Member

MrJithil commented Oct 11, 2023

Seems like ZLIB_VERSION not updated in the zlib.h header file. I can see same version info after running the tools/dep_updaters/update-zlib.sh

@MrJithil
Copy link
Member

The version info we are using https://chromium.googlesource.com/chromium/src/third_party/zlib.git/+/refs/heads/main/zlib.h#40 from here. Which is 1.2.13.1-motley

So, seems its working as designed.

@panva
Copy link
Member Author

panva commented Oct 11, 2023

It's clearly insufficient given the updates don't change this constant. It should include the current hash.

@MrJithil
Copy link
Member

Okay. Could you please suggest which variable should map here? Or do we need to take the latest commit hash shorthand?

@panva
Copy link
Member Author

panva commented Oct 11, 2023

@MrJithil

NEW_VERSION="$VERSION_NUMBER-$LATEST_COMMIT"

@MrJithil
Copy link
Member

MrJithil commented Oct 11, 2023

Got it.
Please suggest the approach.

To accomodate the commit shorthand, we need to create a new header file inside the deps/zlib with a new definition, ZLIB_VERSION or some suitable names.

Then inside the src/node_metadata.cc , we can import the new definition.

Is this is fine?

nodejs-github-bot pushed a commit that referenced this issue Oct 23, 2023
src: add commit hash shorthand in zlib version
PR-URL: #50158
Fixes: #50139
Reviewed-By: Marco Ippolito <[email protected]>
alexfernandez pushed a commit to alexfernandez/node that referenced this issue Nov 1, 2023
src: add commit hash shorthand in zlib version
PR-URL: nodejs#50158
Fixes: nodejs#50139
Reviewed-By: Marco Ippolito <[email protected]>
targos pushed a commit that referenced this issue Nov 11, 2023
src: add commit hash shorthand in zlib version
PR-URL: #50158
Fixes: #50139
Reviewed-By: Marco Ippolito <[email protected]>
UlisesGascon pushed a commit that referenced this issue Dec 11, 2023
src: add commit hash shorthand in zlib version
PR-URL: #50158
Fixes: #50139
Reviewed-By: Marco Ippolito <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants
@panva @MrJithil and others