fix(cmake): fix ext_zlib build type switching#35316
Conversation
Add zlib BUILD_BYPRODUCTS and use configure-time CMAKE_BUILD_TYPE so Make generators rebuild ext_zlib when switching build types. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the ext_zlib external project configuration in cmake/external.cmake to use ${CMAKE_BUILD_TYPE} for the build type and adds a BUILD_BYPRODUCTS entry. A review comment points out a potential path mismatch in the BUILD_BYPRODUCTS definition when ${CMAKE_BUILD_TYPE} is empty, suggesting the use of the ${ext_zlib_install} variable to ensure consistency with the installation prefix.
| CMAKE_ARGS -DCMAKE_POSITION_INDEPENDENT_CODE:BOOL=ON # linking consistent | ||
| CMAKE_ARGS -DZLIB_BUILD_SHARED:BOOL=OFF | ||
| CMAKE_ARGS -DZLIB_BUILD_TESTING:BOOL=OFF | ||
| BUILD_BYPRODUCTS "${TD_EXTERNALS_BASE_DIR}/install/ext_zlib/${CMAKE_BUILD_TYPE}/lib/${ext_zlib_static}" |
There was a problem hiding this comment.
The BUILD_BYPRODUCTS path uses ${CMAKE_BUILD_TYPE}, which can be empty if not explicitly set (e.g., on multi-config generators or when relying on defaults). However, the installation prefix (_ins) passed to the external project in line 197 is defined using TD_CONFIG_NAME, which defaults to Debug when CMAKE_BUILD_TYPE is unset. This mismatch means the byproduct path will not match the actual installation location, causing build errors as CMake won't find the expected file. It is safer and more consistent to use the ${ext_zlib_install} variable which is already set up by the INIT_EXT macro.
BUILD_BYPRODUCTS "${ext_zlib_install}/lib/${ext_zlib_static}"
There was a problem hiding this comment.
Pull request overview
Backports the ext_zlib ExternalProject improvements so that switching build types and building with Makefile generators behaves correctly (especially around stamp detection and dependency tracking for the produced static zlib library).
Changes:
- Pass a build-type value into
ext_zlib’sCMAKE_ARGSintended to make stamp files reflect build-type changes. - Add
BUILD_BYPRODUCTSfor the produced zlib static library to fix Makefile-generator dependency tracking. - Minor whitespace cleanup around
GIT_TAG.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| CMAKE_ARGS -DCMAKE_BUILD_TYPE:STRING=${CMAKE_BUILD_TYPE} # use configure-time value so stamp files detect build type changes | ||
| CMAKE_ARGS -DCMAKE_INSTALL_PREFIX:STRING=${_ins} # let default INSTALL step use | ||
| CMAKE_ARGS -DCMAKE_POSITION_INDEPENDENT_CODE:BOOL=ON # linking consistent | ||
| CMAKE_ARGS -DZLIB_BUILD_SHARED:BOOL=OFF | ||
| CMAKE_ARGS -DZLIB_BUILD_TESTING:BOOL=OFF | ||
| BUILD_BYPRODUCTS "${TD_EXTERNALS_BASE_DIR}/install/ext_zlib/${CMAKE_BUILD_TYPE}/lib/${ext_zlib_static}" |
Summary
Testing