refactor: build bundled zlib via CMake instead of CONFIGURE_COMMAND - #3020
refactor: build bundled zlib via CMake instead of CONFIGURE_COMMAND#3020mvanhorn wants to merge 4 commits into
Conversation
|
Welcome @mvanhorn! It looks like this is your first PR to falcosecurity/libs 🎉 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3020 +/- ##
=======================================
Coverage 76.56% 76.56%
=======================================
Files 301 301
Lines 33693 33693
Branches 5038 5038
=======================================
Hits 25797 25797
Misses 7896 7896
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Perf diff from master - unit testsHeap diff from master - unit testsHeap diff from master - scap fileBenchmarks diff from master |
|
Ping @irozzo-1A ... do I remember correctly that you did |
| -DCMAKE_INSTALL_LIBDIR=lib | ||
| CMAKE_CACHE_ARGS ${ZLIB_EXTERNAL_PROJECT_CACHE_ARGS} | ||
| # Build only the target we need so the unused library type is never compiled. | ||
| BUILD_COMMAND ${CMAKE_COMMAND} --build <BINARY_DIR> --config $<CONFIG> --target |
There was a problem hiding this comment.
--config $<CONFIG> looks risky here because this branch also supports single-config generators. If $<CONFIG> expands to an empty value, the build command may still be invoked with a bare --config, which can break the external project build.
Could we make --config conditional on GENERATOR_IS_MULTI_CONFIG, similar to how ZLIB_BUILD_CONFIG_SUBDIR is handled above?
| set(ZLIB_HEADERS "") | ||
| list( | ||
| APPEND | ||
| ZLIB_HEADERS | ||
| "${ZLIB_INCLUDE}/crc32.h" | ||
| "${ZLIB_INCLUDE}/deflate.h" | ||
| "${ZLIB_INCLUDE}/gzguts.h" | ||
| "${ZLIB_INCLUDE}/inffast.h" | ||
| "${ZLIB_INCLUDE}/inffixed.h" | ||
| "${ZLIB_INCLUDE}/inflate.h" | ||
| "${ZLIB_INCLUDE}/inftrees.h" | ||
| "${ZLIB_INCLUDE}/trees.h" | ||
| "${ZLIB_SOURCE_DIR}/crc32.h" | ||
| "${ZLIB_SOURCE_DIR}/deflate.h" | ||
| "${ZLIB_SOURCE_DIR}/gzguts.h" | ||
| "${ZLIB_SOURCE_DIR}/inffast.h" | ||
| "${ZLIB_SOURCE_DIR}/inffixed.h" | ||
| "${ZLIB_SOURCE_DIR}/inflate.h" | ||
| "${ZLIB_SOURCE_DIR}/inftrees.h" | ||
| "${ZLIB_SOURCE_DIR}/trees.h" | ||
| "${ZLIB_INCLUDE}/zconf.h" | ||
| "${ZLIB_INCLUDE}/zlib.h" | ||
| "${ZLIB_INCLUDE}/zutil.h" | ||
| "${ZLIB_SOURCE_DIR}/zlib.h" | ||
| "${ZLIB_SOURCE_DIR}/zutil.h" |
There was a problem hiding this comment.
nit: this seems to change the semantic of ZLIB_HEADERS, it does not seem to be a problem,
Would it make sense to rename it to something like _zlib_headers to avoid confusion ? In such case we should apply the same convention to other internal variables for consistency e.g.
- ZLIB_SOURCE_DIR
- ZLIB_BUILD_DIR
- ZLIB_BUILD_TARGET
- ZLIB_LIBRARY_INSTALL_TYPE
- ZLIB_BUILD_CONFIG_SUBDIR
- ZLIB_BUILT_LIB
- ZLIB_INSTALL_SCRIPT
- ZLIB_EXTERNAL_PROJECT_CACHE_ARGS
- ZLIB_IS_MULTI_CONFIG
I don't consider this as a blocker for this PR though.
|
Addressed both points in ca60fa1: the external zlib build now passes |
ca60fa1 to
dc1e7d9
Compare
| set(ZLIB_SRC "${PROJECT_BINARY_DIR}/zlib-prefix/src/zlib") | ||
| set(ZLIB_INCLUDE "${ZLIB_SRC}") | ||
| set(ZLIB_HEADERS "") | ||
| set(ZLIB_SRC "${PROJECT_BINARY_DIR}/zlib-prefix/src") |
There was a problem hiding this comment.
Aligning ZLIB_SRC with the jsoncpp convention SGTM. However, ZLIB_SRC is not private to this module, so re-pointing it has two side effects.
First, cmake/modules/libbpf.cmake:71 passes -L${ZLIB_SRC} to libbpf's make. That directory used to hold libz.a directly, now the library lives in ${ZLIB_SRC}/lib, so the flag points at a dir with no zlib in it. It is inert atm, since EXTRA_LDFLAGS only feeds the shared-object link rule that libbpf skips under BUILD_STATIC_ONLY=y. Still, may you fix it to -L${ZLIB_SRC}/lib here, before it bites us later? 🙏
Second, Falco reuses this very module and passes --with-zlib=${ZLIB_SRC} in its own cmake/modules/curl.cmake. curl's configure expands that to -I$OPT_ZLIB/include -L$OPT_ZLIB/lib 👉 https://github.com/curl/curl/blob/curl-8_7_1/configure.ac#L1320-L1322
Pre-PR neither of those dirs existed (the old build was in-source with INSTALL_COMMAND ""), so Falco's bundled curl was silently falling back to the system zlib. With this PR both exist, so curl will finally pick the bundled one. That is a fix rather than a regression, but it is still a behavior change downstream, so I'd prefer to see a Falco build green before we merge this.
| ExternalProject_Add( | ||
| zlib | ||
| PREFIX "${PROJECT_BINARY_DIR}/zlib-prefix" | ||
| SOURCE_DIR "${_zlib_source_dir}" |
There was a problem hiding this comment.
| SOURCE_DIR "${_zlib_source_dir}" | |
| SOURCE_DIR "${_zlib_source_dir}" | |
| BINARY_DIR "${_zlib_build_dir}" |
Nit. _zlib_build_dir (line 69) hardcodes ExternalProject's default binary dir, and the install script turns any mismatch into a FATAL_ERROR. Declaring BINARY_DIR explicitly drops that implicit coupling, and it is what re2.cmake already does.
Also, the file(MAKE_DIRECTORY ...) at line 83 can go, since file(WRITE ...) creates the parent dirs on its own.
| -DCMAKE_INSTALL_PREFIX=${ZLIB_SRC} | ||
| -DCMAKE_INSTALL_LIBDIR=lib |
There was a problem hiding this comment.
| -DCMAKE_INSTALL_PREFIX=${ZLIB_SRC} | |
| -DCMAKE_INSTALL_LIBDIR=lib | |
| -DCMAKE_INSTALL_PREFIX=${ZLIB_SRC} |
Nit. zlib 1.3.1 does not use GNUInstallDirs, it hardcodes INSTALL_LIB_DIR "${CMAKE_INSTALL_PREFIX}/lib", so CMAKE_INSTALL_LIBDIR is ignored and every configure of the sub-build warns about it:
CMake Warning (unused-cli):
Manually-specified variables were not used by the project:
CMAKE_INSTALL_LIBDIR
The layout is already lib/, so dropping it changes nothing. Same for line 172 in the Windows branch.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mvanhorn The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
All three applied in f0ed401.
Dropped
On your downstream point: I can't produce a green Falco build from here. Agreed it should gate the merge given curl will now pick up the bundled zlib where it previously fell back to the system one. Happy to open the Falco-side PR to test it if that's useful, or leave it to you. |
fdd9309 to
92e72b4
Compare
…alcosecurity#2584) zlib v1.3.1 ships a first-class CMakeLists.txt, so build the bundled dependency on non-Windows with its own CMake build (via CMAKE_ARGS and the shared falcosecurity_external_project_cache_args toolchain mechanism) instead of invoking the hand-written ./configure script through CONFIGURE_COMMAND. Only the library type actually consumed is built (zlibstatic for static, zlib for shared), preserving the previous './configure --static' behavior and keeping static-only / cross toolchains working. The install step is config-aware (handles multi-config generators) and preserves the shared library SOVERSION symlink chain. ZLIB_INCLUDE / ZLIB_LIB / ZLIB_HEADERS are unchanged for downstream consumers. Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> (cherry picked from commit 3cc133a) Signed-off-by: Matt Van Horn <mvanhorn@gmail.com>
…internal vars Per review from @irozzo-1A: pass --config $<CONFIG> to the external zlib build only on multi-config generators (a single-config generator would expand to a bare --config and break the build), and rename the internal helper variables to the _zlib_* convention to avoid clashing with the public ZLIB_* cache variables. Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> (cherry picked from commit dc1e7d9) Signed-off-by: Matt Van Horn <mvanhorn@gmail.com>
…, fix libbpf -L path
Per review:
- Declare BINARY_DIR explicitly so ExternalProject's default is no longer
implicitly coupled to _zlib_build_dir, and drop the now-redundant
file(MAKE_DIRECTORY): file(WRITE ...) creates its parent dirs.
- zlib 1.3.1 hardcodes INSTALL_LIB_DIR from CMAKE_INSTALL_PREFIX and ignores
GNUInstallDirs, so CMAKE_INSTALL_LIBDIR was unused and warned on every
sub-build configure.
- libbpf's EXTRA_LDFLAGS pointed at ${ZLIB_SRC}, which no longer holds libz.a
now that the build installs under ${ZLIB_SRC}/lib.
Signed-off-by: Matt Van Horn <mvanhorn@gmail.com>
(cherry picked from commit deaaba6)
Signed-off-by: Matt Van Horn <mvanhorn@gmail.com>
56ad528 to
f5ddc15
Compare
poiana
added
release-note-none
and removed
do-not-merge/contains-merge-commits
do-not-merge/release-note-label-needed
labels
| "LDFLAGS=-Wl,-Bstatic" "EXTRA_LDFLAGS=-L${LIBELF_SRC}/libelf/libelf -L${ZLIB_SRC}/lib" -C | ||
| ${LIBBPF_SRC}/libbpf/src install install_uapi_headers |
There was a problem hiding this comment.
| "LDFLAGS=-Wl,-Bstatic" "EXTRA_LDFLAGS=-L${LIBELF_SRC}/libelf/libelf -L${ZLIB_SRC}/lib" -C | |
| ${LIBBPF_SRC}/libbpf/src install install_uapi_headers | |
| "LDFLAGS=-Wl,-Bstatic" "EXTRA_LDFLAGS=-L${LIBELF_SRC}/libelf/libelf -L${ZLIB_SRC}/lib" | |
| -C ${LIBBPF_SRC}/libbpf/src install install_uapi_headers |
This is what format code is failing on. Adding /lib took the line to 101 columns, one over the line_width: 100 in .cmake-format.json, so cmake-format wants -C wrapped down.
May you run cmake-format -i cmake/modules/libbpf.cmake (0.6.13, with the repo config) just to be safe? The patch is downloadable from the failing job too 👉 https://github.com/falcosecurity/libs/actions/runs/30755533927
Thanks 🙏
Adding the /lib suffix pushed the EXTRA_LDFLAGS line to 101 columns, one over line_width in .cmake-format.json, so cmake-format 0.6.13 wraps -C down. Both file(INSTALL DESTINATION ...) calls create their own destination, so the MAKE_DIRECTORY ahead of them is redundant. Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
|
Thanks for the thorough pass, and for pinpointing the format failure.
On the Falco side effect: agreed that curl picking up the bundled zlib rather than silently falling back to the system one is the point, but it is still a downstream behaviour change. I cannot run a Falco build myself, so I will leave that gate to you. |
5 participants
Summary
Build the bundled
zlibdependency on non-Windows via zlib's own CMake build instead ofinvoking its hand-written
./configurescript throughExternalProject_Add'sCONFIGURE_COMMAND. zlib v1.3.1 ships a first-classCMakeLists.txt, so the bundled dep is nowconfigured and built with CMake (mirroring the existing Windows branch and the
jsoncpp/re2/tbbmodules), with build settings passed viaCMAKE_ARGSand the toolchainforwarded through the shared
falcosecurity_external_project_cache_argsmechanism.Why this matters
Issue #2584 tracks this as a build-system maintainability cleanup. Using zlib's own CMake build
drops the bespoke
./configure+make+CFLAGSplumbing (-O3/-g,-fPIC, and toolchainflag accumulation) in favor of standard CMake cache vars (
CMAKE_BUILD_TYPE,CMAKE_POSITION_INDEPENDENT_CODE,CMAKE_C_FLAGS), and unifies how the bundled dependencytoolchain is propagated with the rest of the bundled deps.
Changes
cmake/modules/zlib.cmake(non-Windows branch): replaceCONFIGURE_COMMAND ./configure/BUILD_COMMAND make/BUILD_IN_SOURCE 1with a CMake configure+build viaCMAKE_ARGSandCMAKE_CACHE_ARGS ${ZLIB_EXTERNAL_PROJECT_CACHE_ARGS}.zlibstaticfor static builds,zlibfor shared) so the unused target is never compiled — preserving the old
./configure --staticbehavior and keeping static-only / cross toolchains working.
-DZLIB_BUILD_EXAMPLES=OFFso the bundled sub-build skips zlib's example/test executables.ZLIB_INCLUDE/ZLIB_LIB/ZLIB_HEADERScontinue to point at the produced library andheaders, so downstream consumers (
libscap,libsinsp,libpman,libbpf) see no interfacechange.
Testing
The
cmake/modules/zlib.cmakemodule is syntactically valid and theExternalProject_Addarguments are internally consistent (verified by configuring the module in isolation and by
building the bundled zlib sub-project directly for static and shared, single- and multi-config
generators). A full
falcosecurity/libsbuild was not run locally as it requires the completedependency set; CI exercises the end-to-end bundled build.
Fixes #2584
AI was used for assistance.
Does this PR introduce a user-facing change?: