deps: delete deps/v8/third_party/zlib #47493
Conversation
|
Review requested:
|
nodejs-github-bot
added
build
kvakil
force-pushed
the
unifyZlibApproachOne
branch
from
6ba7074 to
f3d3302
Compare
|
It appears that this breaks the build on Windows. |
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 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... |
|
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.
+1. I'll adapt the V8 update script to stop adding zlib after this lands. |
|
@kvakil is there a chance you'll continue this PR? I think it may fix a problem introduced with V8 11.5: #48456 (comment) |
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
This is the second of a series of patches. This patch is contains changes to V8 and its build system to switch to our other copy of zlib.
Previous patches in the series make it such that this directory is no longer needed. This commit actually does the deletion.
kvakil
force-pushed
the
unifyZlibApproachOne
branch
from
f3d3302 to
8dc576e
Compare
thanks. Fixed in the latest iteration.
If we really want to avoid the V8 patch, it seems possible to do this via a
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:
|
github-actions
bot
removed
the
request-ci
|
The problem is that the V8 changes break V8 CI |
|
Hm. Perhaps we can |
Symlink the actual directory in.
|
I suppose we also need to adapt BUILD.gn (for the include dir) |
So what is the problem on Windows with installing system zlib + its devel resources before start building node? 🤔 |
|
This needs a rebase. |
|
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. |
|
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. |
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:
There is an additional cost to keeping both dependencies up-to-date, and in fact they were already out-of-sync (see the refs).
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.
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: #47145
Refs: #47157