-
Notifications
You must be signed in to change notification settings - Fork 30k
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
build: rework gyp files for zlib #33044
Conversation
Another reason I initially moved the gyp file was that I was hoping to reuse I'm not sure if this fixes #33019 -- I no longer see the reported errors in zlib when compiling with |
This comment has been minimized.
This comment has been minimized.
fwiw, I'd much rather we keep the relevant gyp files in the deps folder and consistently use a pattern that is more like what we do with openssl, nghttp2, etc... where the source files for the dependency are in a subdirectory (e.g. |
@jasnell Moved the gyp file back to |
CI: https://ci.nodejs.org/job/node-test-pull-request/30957/ (:yellow_heart:) |
Also fixes #32856. |
Sigh. I ran benchmarks over the weekend and there are notable regressions. AFAICT the build logs (see the second link below) show we're passing in the expected compiler flags/definitions for the optimized files based on the https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/576/console
|
8ae28ff
to
2935f72
Compare
Reportedly this fixes building on 32-bit x86 (#33019 (comment)) (which is an unofficial build platform) but I'm assuming the drop in benchmarks would prevent this landing. I don't have the time to investigate this further, so I've put a |
Another report that this fixes building on 32-bit Linux: nodejs/help#2943 (comment) |
I rebased against master and force-pushed in the hope that it will somehow cause the performance regression to go away. Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/670/ UPDATE: Bummer. Still shows the performance degredation.
|
Updated zlib to 8cd0fc1 hoping that removes the performance regression. Worth a shot. Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/671/ UPDATE: Performance regression is still there.
|
@@ -0,0 +1,28 @@ | |||
# Copyright (c) 2019 Refael Ackeramnn<refack@gmail.com>. All rights reserved. | |||
# Use of this source code is governed by an MIT-style license. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to update/run license-builder.sh for this? And might this be better living in tools
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
¯\_(ツ)_/¯. It's a copy of the one in tools/v8_gypfiles/GN-scraper.py
which we don't currently have in tools/license-builder.sh
.
If I remember correctly I tried to reference the existing script/put the script in tools but couldn't get the gyp file to call it successfully via <!@pymod_do_main(...
. Maybe someone else in @nodejs/gyp or @nodejs/python can advise.
I'm guessing there's a simple fix for the compilation failure with quic enabled on Windows. @nodejs/quic
|
Restructure the zlib.gyp file based on the upstream gn file, breaking out the files with optimizations that need additional compiler flags. Use a copy of the GN-scraper.py script to reduce the amount of hand editing when the zlib dependency is updated.
Did a quick rebase. |
FWIW it appears the
|
'type': 'static_library', | ||
'conditions': [ | ||
['OS!="win" or llvm_version!="0.0"', { | ||
'cflags': [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this getting used at all? The latest test-macOS / test-macOS (pull_request) logs don't seem to include the cflags
for this target in https://github.com/nodejs/node/pull/33044/checks?check_run_id=3008235129#step:5:1336:
cc -o /Users/runner/work/node/node/out/Release/obj.target/zlib_crc32_simd/deps/zlib/crc32_simd.o ../deps/zlib/crc32_simd.c '-DV8_DEPRECATION_WARNINGS' '-DV8_IMMINENT_DEPRECATION_WARNINGS' '-D_GLIBCXX_USE_CXX11_ABI=1' '-D_DARWIN_USE_64_BIT_INODE=1' '-DOPENSSL_NO_PINSHARED' '-DOPENSSL_THREADS' '-DCRC32_SIMD_SSE42_PCLMUL' '-DZLIB_IMPLEMENTATION' -I../deps/zlib -O3 -gdwarf-2 -mmacosx-version-min=10.13 -arch x86_64 -Wall -Wendif-labels -W -Wno-unused-parameter -fno-strict-aliasing -MMD -MF /Users/runner/work/node/node/out/Release/.deps//Users/runner/work/node/node/out/Release/obj.target/zlib_crc32_simd/deps/zlib/crc32_simd.o.d.raw -c
To follow this ticket :) |
Superseded by #45589. |
Restructure the gyp file based on the upstream gn file, breaking out
the files with optimizations that need additional compiler flags.
Use a copy of the GN-scraper.py script to reduce the amount of hand
editing when the zlib dependency is updated.
Refs: #32553
Refs: #33019
Fixes: #32856
cc @nodejs/build-files @nodejs/zlib
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes