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.

Sign up for GitHub

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

Jump to bottom

deps: delete deps/zlib #47157

Closed
wants to merge 3 commits into from
Closed

deps: delete deps/zlib #47157

wants to merge 3 commits into from

Commits on Mar 19, 2023

  1. deps: delete deps/zlib 

    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
    @kvakil
    kvakil committed Mar 19, 2023
    Configuration menu
    Copy the full SHA
    73d92c3 View commit details
    Browse the repository at this point in the history

  2. fixup! deps: delete deps/zlib

    @kvakil
    kvakil committed Mar 19, 2023
    Configuration menu
    Copy the full SHA
    d1f16a9 View commit details
    Browse the repository at this point in the history

  3. fixup! deps: delete deps/zlib

    @kvakil
    kvakil committed Mar 19, 2023
    Configuration menu
    Copy the full SHA
    61db049 View commit details
    Browse the repository at this point in the history