deps: delete deps/v8/third_party/zlib

[kvakil]

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.

Note to reviewers: the first commit does the changes, the second commit actually deletes the directory.

Refs: https://github.com/nodejs/node/pull/47145 Refs: https://github.com/nodejs/node/pull/47157

[nodejs-github-bot]

Review requested:

• [ ] @nodejs/gyp
• [ ] @nodejs/v8-update

[Trott]

It appears that this breaks the build on Windows.

[bnoordhuis]

D:\a\node\node\deps\zlib\google\compression_utils_portable.h(19,10): fatal error C1083: Cannot open include file: 'zlib.h': No such file or directory [D:\a\node\node\deps\zlib\zlib_google.vcxproj]

That happens because you changed includes in deps/v8/src from:

#include "third_party/zlib/google/compression_utils_portable.h"

To:

#include "compression_utils_portable.h"

You'll need to add deps/zlib to the right include_dirs directives in tools/v8_gypfiles/v8.gyp and possibly other build files in said directory.

As a rule we don't accept V8 patches that haven't been merged upstream but deleting 45,000+ lines of code in exchange for a two-line diff seems like a worthwhile tradeoff.

If Windows weren't a concern, I'd suggest symlinking deps/v8/third_party/zlib to deps/zlib but yeah, Windows...

[targos]

Can you please make a standalone commit for the changes in deps/v8 and common.gypi? We must be able to cherry-pick it easily during V8 updates.

As a rule we don't accept V8 patches that haven't been merged upstream but deleting 45,000+ lines of code in exchange for a two-line diff seems like a worthwhile tradeoff.

+1. I'll adapt the V8 update script to stop adding zlib after this lands.

[targos]

@kvakil is there a chance you'll continue this PR? I think it may fix a problem introduced with V8 11.5: https://github.com/nodejs/node/pull/48456#issuecomment-1591050515

[kvakil]

You'll need to add deps/zlib to the right include_dirs directives in tools/v8_gypfiles/v8.gyp and possibly other build files in said directory.

thanks. Fixed in the latest iteration.

If Windows weren't a concern, I'd suggest symlinking deps/v8/third_party/zlib to deps/zlib but yeah, Windows...

If we really want to avoid the V8 patch, it seems possible to do this via a gyp action to "generate" the correct directory structure (via symlink on POSIX systems and just copying on Windows systems).

---

Can you please make a standalone commit for the changes in deps/v8 and common.gypi? We must be able to cherry-pick it easily during V8 updates.

I did my best here, but I'm not really sure how to achieve this. If every commit needs to be independently buildable, then I think I need to make the changes to deps/v8 and tools/v8_gypfiles/v8.gyp at the same time. But I tried my best to make it smaller: the second commit in this patch series is now just the changes to V8-related files.

Also happy to give the gyp idea mentioned above a try if floating the V8 patch is harder.

---

Updated the current patch series. Main changes:

• should build on windows now
• should work with cross-compilation now

[nodejs-github-bot]

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

[nodejs-github-bot]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/5408/

[nodejs-github-bot]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/5408/

[nodejs-github-bot]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/5408/

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[targos]

The problem is that the V8 changes break V8 CI

[nodejs-github-bot]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/5412/

[nodejs-github-bot]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/5412/

[kvakil]

Hm. Perhaps we can cp -r deps/zlib deps/v8/third_party/zlib (or symlink) in tools/make-v8.sh, and that might be enough? Will give it a go tomorrow.

[nodejs-github-bot]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/5412/

[targos]

I suppose we also need to adapt BUILD.gn (for the include dir)

[kloczek]

It appears that this breaks the build on Windows.

So what is the problem on Windows with installing system zlib + its devel resources before start building node? 🤔

[aduh95]

This needs a rebase.

[github-actions[bot]]

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

[github-actions[bot]]

Closing this because it has stalled. Feel free to reopen if this issue/PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.