deps: delete deps/zlib #47157
Closed
deps: delete deps/zlib #47157
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We currently have two copies of Chromium's zlib: one in deps/zlib and
another in deps/v8/third_party/zlib. This has a couple of disadvantages:
1. There is an additional cost to keeping both dependencies up-to-date,
and in fact they were already out-of-sync (see the refs).
2. People who compile with --shared-zlib (i.e. distro maintainers) will
probably not be thrilled to learn that there is still a copy of zlib
inside.
3. It's aesthetically unpleasing.
Centralize on the version in V8 rather than the one in deps, and delete
the one in deps. Basically I just copied deps/zlib/zlib.gyp to
tools/v8_gypfiles/zlib.gyp, since the former had a better build
configuration (see the refs). This approach seemed better than the
opposite approach of centralizing on deps/zlib because:
1. We would need to occasionally bump deps/zlib manually after bumping
deps/v8, which seemed annoying.
2. The maintenance steps for bumping zlib seemed more onerous than the
maintenance steps for bumping V8.
(If we would prefer the opposite approach, I actually have another patch
locally.)
One discrepancy was that V8's version of zlib had all symbols to be
prefixed with `Cr_z_` per deps/v8/third_party/zlib/chromeconf.h, which
seemed undesirable, so I added define `CHROMIUM_ZLIB_NO_CHROMECONF`
to the build to remove it. (deps/zlib handled this by just commenting
out the relevant include, but floating a patch seemed less desirable.)
I tested this on Linux with the default build and a --shared-zlib build.
I checked that the shared-zlib build correctly linked zlib according to
ldd. I would appreciate if the reviewers could suggest some other build
configurations to try.
Refs: nodejs#47145
Refs: nodejs#47151
nodejs-github-bot
added
dependencies
|
I wasn't sure what to do with |
|
We could cherry-pick the update patch like we do for adhoc V8 patches. In practice the changes to (Also, how do we deal with zlib security fixes today? My assumption is that we're currently just updating the copy in deps/zlib and not updating the one in deps/v8/third_party/zlib. If we already have a process in place for updating deps/v8/third_party/zlib, then we can just use that going forward, it's strictly less work than before.) |
|
The remaining failing test is caused by node/test/addons/zlib-binding/binding.gyp Line 14 in 142d6af |
|
I'd do it the other way around. deps/zlib is what backs the built-in The flip side - V8 now depending on deps/zlib - should be a lot less problematic. V8 doesn't really do anything out of the ordinary with zlib. (I may be wrong about that but I don't think so.) |
OK, I'll put up the PR which does the opposite of this PR later today. FWIW deps/zlib and V8's zlib are the same codebase -- they're both Chromium's fork of zlib. This is more of a question of maintenance & upgrades rather than what code we're running. |
This was referenced Mar 20, 2023
19 tasks
kvakil
added a commit
to kvakil/node
that referenced
this pull request
Apr 10, 2023
We currently have two copies of Chromium's zlib: one in deps/zlib and another in deps/v8/third_party/zlib. This has a couple of disadvantages: 1. There is an additional cost to keeping both dependencies up-to-date, and in fact they were already out-of-sync (see the refs). 2. People who compile with --shared-zlib (i.e. distro maintainers) will probably not be thrilled to learn that there is still a copy of zlib inside. 3. It's aesthetically unpleasing. This diff (per discussion in the refs) centralizes on deps/zlib and deletes deps/v8/third_party/zlib. That requires changing the include headers in two files in deps/v8/src, which was pretty straightforward. When the user requests compiling with a shared zlib build, we still need to compile the contents of deps/zlib/google. This is not ideal but it's also not a change in behavior: prior to this diff those files were being compiled in the deps/v8/third_party/zlib version. I tested this on Linux with the default build and a --shared-zlib build. I checked that the shared-zlib build dynamically linked zlib according to ldd, and that the regular build did not. I would appreciate if the reviewers could suggest some other build configurations to try. Refs: nodejs#47145 Refs: nodejs#47157
kvakil
added a commit
to kvakil/node
that referenced
this pull request
Apr 10, 2023
We currently have two copies of Chromium's zlib: one in deps/zlib and another in deps/v8/third_party/zlib. This has a couple of disadvantages: 1. There is an additional cost to keeping both dependencies up-to-date, and in fact they were already out-of-sync (see the refs). 2. People who compile with --shared-zlib (i.e. distro maintainers) will probably not be thrilled to learn that there is still a copy of zlib inside. 3. It's aesthetically unpleasing. This diff (per discussion in the refs) centralizes on deps/zlib and deletes deps/v8/third_party/zlib. That requires changing the include headers in two files in deps/v8/src, which was pretty straightforward. When the user requests compiling with a shared zlib build, we still need to compile the contents of deps/zlib/google. This is not ideal but it's also not a change in behavior: prior to this diff those files were being compiled in the deps/v8/third_party/zlib version. I tested this on Linux with the default build and a --shared-zlib build. I checked that the shared-zlib build dynamically linked zlib according to ldd, and that the regular build did not. I would appreciate if the reviewers could suggest some other build configurations to try. Refs: nodejs#47145 Refs: nodejs#47157
kvakil
added a commit
to kvakil/node
that referenced
this pull request
Apr 10, 2023
We currently have two copies of Chromium's zlib: one in deps/zlib and another in deps/v8/third_party/zlib. This has a couple of disadvantages: 1. There is an additional cost to keeping both dependencies up-to-date, and in fact they were already out-of-sync (see the refs). 2. People who compile with --shared-zlib (i.e. distro maintainers) will probably not be thrilled to learn that there is still a copy of zlib inside. 3. It's aesthetically unpleasing. This diff (per discussion in the refs) centralizes on deps/zlib and deletes deps/v8/third_party/zlib. That requires changing the include headers in two files in deps/v8/src, which was pretty straightforward. When the user requests compiling with a shared zlib build, we still need to compile the contents of deps/zlib/google. This is not ideal but it's also not a change in behavior: prior to this diff those files were being compiled in the deps/v8/third_party/zlib version. I tested this on Linux with the default build and a --shared-zlib build. I checked that the shared-zlib build dynamically linked zlib according to ldd, and that the regular build did not. I would appreciate if the reviewers could suggest some other build configurations to try. Refs: nodejs#47145 Refs: nodejs#47157
kvakil
added a commit
to kvakil/node
that referenced
this pull request
Jun 22, 2023
We currently have two copies of Chromium's zlib: one in deps/zlib and another in deps/v8/third_party/zlib. This has a couple of disadvantages: 1. There is an additional cost to keeping both dependencies up-to-date, and in fact they were already out-of-sync (see the refs). 2. People who compile with --shared-zlib (i.e. distro maintainers) will probably not be thrilled to learn that there is still a copy of zlib inside. 3. It's aesthetically unpleasing. This diff (per discussion in the refs) centralizes on deps/zlib and deletes deps/v8/third_party/zlib. That requires changing the include headers in two files in deps/v8/src, which was pretty straightforward. When the user requests compiling with a shared zlib build, we still need to compile the contents of deps/zlib/google. This is not ideal but it's also not a change in behavior: prior to this diff those files were being compiled in the deps/v8/third_party/zlib version. I tested this on Linux with the default build and a --shared-zlib build. I checked that the shared-zlib build dynamically linked zlib according to ldd, and that the regular build did not. I would appreciate if the reviewers could suggest some other build configurations to try. Refs: nodejs#47145 Refs: nodejs#47157
kvakil
added a commit
to kvakil/node
that referenced
this pull request
Jun 22, 2023
This is the first of a series of patches. This patch is contains changes to the existing zlib.gyp file to allow it to be used by our v8.gyp. --- We currently have two copies of Chromium's zlib: one in deps/zlib and another in deps/v8/third_party/zlib. This has a couple of disadvantages: 1. There is an additional cost to keeping both dependencies up-to-date, and in fact they were already out-of-sync (see the refs). 2. People who compile with --shared-zlib (i.e. distro maintainers) will probably not be thrilled to learn that there is still a copy of zlib inside. 3. It's aesthetically unpleasing. This diff (per discussion in the refs) centralizes on deps/zlib and deletes deps/v8/third_party/zlib. When the user requests compiling with a shared zlib build, we still need to compile the contents of deps/zlib/google. This is not ideal but it's also not a change in behavior: prior to this diff those files were being compiled in the deps/v8/third_party/zlib version. I tested this on Linux with the default build and a --shared-zlib build. I checked that the shared-zlib build dynamically linked zlib according to ldd, and that the regular build did not. I would appreciate if the reviewers could suggest some other build configurations to try. Refs: nodejs#47145 Refs: nodejs#47157
|
Closing for #47493. In addition this issue is sort of already fixed on the V8 side by https://chromium-review.googlesource.com/c/v8/v8/+/4571989. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
We currently have two copies of Chromium's zlib: one in deps/zlib and another in deps/v8/third_party/zlib. This has a couple of disadvantages:
Centralize on the version in V8 rather than the one in deps, and delete the one in deps. Basically I just copied deps/zlib/zlib.gyp to tools/v8_gypfiles/zlib.gyp, since the former had a better build configuration (see the refs). This approach seemed better than the opposite approach of centralizing on deps/zlib because:
(If we would prefer the opposite approach, I actually have another patch locally.)
One discrepancy was that V8's version of zlib had all symbols to be prefixed with
Cr_z_per deps/v8/third_party/zlib/chromeconf.h, which seemed undesirable, so I added defineCHROMIUM_ZLIB_NO_CHROMECONFto the build to remove it. (deps/zlib handled this by just commenting out the relevant include, but floating a patch seemed less desirable.)I tested this on Linux with the default build and a --shared-zlib build. I checked that the shared-zlib build correctly linked zlib according to ldd. I would appreciate if the reviewers could suggest some other build configurations to try.
Refs: #47145
Refs: #47151