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

Build system complains about uncloned submodules for bundled dependencies even when using dependencies from system #891

Open
JayFoxRox opened this issue Aug 25, 2023 · 4 comments · May be fixed by #895
Open

Build system complains about uncloned submodules for bundled dependencies even when using dependencies from system #891

JayFoxRox opened this issue Aug 25, 2023 · 4 comments · May be fixed by #895
Labels
Build

Comments

@JayFoxRox
Copy link
Contributor

First system dependencies are searched (which is good):

apitrace/CMakeLists.txt

Lines 535 to 562 in 00706d1

if (NOT WIN32 AND NOT ENABLE_STATIC_EXE)
if (NOT ENABLE_STATIC_SNAPPY)
find_package (Snappy)
endif ()
# zlib 1.2.4-1.2.5 made it impossible to read the last block of incomplete
# gzip traces (e.g., apitrace-tests/traces/zlib-no-eof.trace).
find_package (ZLIB 1.2.6)
# FindPNG.cmake will search ZLIB internally (without requiring any particular
# version), adding its include dirs and libraries, and overwriting ZLIB_FOUND.
# So if the system's ZLIB was did not meet the our requirements, then there's
# no safe way to use the system's PNG library.
if (NOT APPLE AND ZLIB_FOUND)
find_package (PNG)
endif ()
find_package (PkgConfig)
if (PKG_CONFIG_FOUND)
pkg_check_modules (BROTLIDEC IMPORTED_TARGET libbrotlidec>=1.0.7)
pkg_check_modules (BROTLIENC IMPORTED_TARGET libbrotlienc>=1.0.7)
endif ()
find_package (GTest)
endif ()
add_subdirectory (thirdparty)

Then submodules must exist regardless (which is bad):

apitrace/thirdparty/CMakeLists.txt

Lines 19 to 36 in 00706d1

set (SUBMODULES_MISSING FALSE)
foreach (path IN ITEMS
brotli/LICENSE
gtest/LICENSE
libbacktrace/LICENSE
libpng/LICENSE
snappy/COPYING
zlib/README
directxmath/LICENSE
)
if (NOT EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/${path}")
message (SEND_ERROR "error: ${CMAKE_CURRENT_SOURCE_DIR}/${path} does not exist")
set (SUBMODULES_MISSING TRUE)
endif ()
endforeach ()
if (SUBMODULES_MISSING)
message (FATAL_ERROR "Update Git submodules by running\ngit submodule update --init --depth 1 --recursive")
endif ()

But then most of those folders can probably be skipped because they are only used conditionally (which is good):

apitrace/thirdparty/CMakeLists.txt

Lines 38 to 66 in 00706d1

if (ENABLE_STATIC_SNAPPY OR NOT Snappy_FOUND)
message (STATUS "Using bundled Snappy")
include_with_scope (snappy.cmake)
endif ()
if (NOT ZLIB_FOUND)
message (STATUS "Using bundled ZLIB")
include_with_scope (zlib.cmake)
endif ()
if (NOT PNG_FOUND)
message (STATUS "Using bundled PNG")
include_with_scope (libpng.cmake)
endif ()
if (NOT BROTLIDEC_FOUND OR NOT BROTLIENC_FOUND)
message (STATUS "Using bundled Brotli")
include_with_scope (brotli.cmake)
endif ()
if (CMAKE_EXECUTABLE_FORMAT STREQUAL "ELF")
include_with_scope (libbacktrace.cmake)
endif ()
# We use non-standard C++ flags, so we can't just use GTest's CMakeLists.txt
if (NOT GTEST_FOUND)
message (STATUS "Using bundled GTest")
include_with_scope (gtest.cmake)
endif ()

@spillner
Copy link

This is a bug: the CMakeLists are set up as though you're always building from git (hence the admonition to run git submodule update) and don't work if you're building from a release tarball. You can safely change the SEND_ERROR / FATAL_ERROR to WARNING, manually install the libbacktrace tree (git clone https://github.com/ianlancetaylor/libbacktrace.git from the thirdparty/ subdirectory), and then redo your cmake .. && make. cmake will find and use your system brotli, libpng, zlib, etc.

@jrfonseca
Copy link
Member

@JayFoxRox , @Spiller, so the ask here is to only throw an error if the submodule is necessary?

It would be nice if GitHub release tarballs recursively packed the submodules. GitHub doesn't provide such easy option, but it's possible to achieve the same via GitHub Actions, for example, https://github.com/Return-To-The-Roots/s25client/blob/a24429ffa9426557304144d523729013da3799f4/.github/workflows/release.yml

@JayFoxRox
Copy link
Contributor Author

so the ask here is to only throw an error if the submodule is necessary?

Yes. Precisely.
Sorry for not being clearer earlier.

@spillner spillner linked a pull request Sep 22, 2023 that will close this issue
Open
@spillner
Copy link

I took a slightly different tack: even if the submodule is used, allow CMake to search for the system version instead of insisting that a submodule be present under thirdparty/. If any of the thirdparty/ modules are missing, the user will receive a warning to git submodule update if any dependencies are not resolved.

I think this will make it much easier to build apitrace packages for a distribution that already ships separate packages for most or all of these dependencies, as well as to simply build from apitrace's point-release tarballs (on such a distribution) without having to sync with the git repos. In other words, it shifts the assumption that the default user is /not/ cloning apitrace from git.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Build
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Allow building using system versions instead of thirdparty submodules spillner/apitrace
3 participants
@jrfonseca @JayFoxRox @spillner