build: make pkg-config optional for the system libunwind/breakpad options - #2007
Open
mertefesensoy wants to merge 3 commits into
Open
build: make pkg-config optional for the system libunwind/breakpad options#2007mertefesensoy wants to merge 3 commits into
mertefesensoy wants to merge 3 commits into
Conversation
…ions `SENTRY_LIBUNWIND_SYSTEM` and `SENTRY_BREAKPAD_SYSTEM` located their dependencies with `find_package(PkgConfig REQUIRED)` + `pkg_check_modules(... REQUIRED ...)`, and the exported `sentry-config.cmake` repeated that for consumers of a static build. Those are the only places in this project that ever touch pkg-config, and only on Linux, but because the requirement is unconditional and hard, packagers have to provide the tool on every platform they build sentry-native for -- including Windows and macOS, where it is never invoked. Add `cmake/sentry-find-system-library.cmake`, which prefers the pkg-config metadata when both the tool and the `.pc` module are present and otherwise resolves the library and its headers with `find_library()` / `find_path()`. Both branches define the same imported target (`sentry::libunwind`, `sentry::libunwind-ptrace`, `sentry::breakpad-client`), so the call sites and the installed config no longer care which lookup succeeded, and a missing dependency now reports what could not be found instead of failing inside FindPkgConfig. The module is installed next to `sentry-config.cmake` so consumers of a static build recreate the targets the same way. As a side effect the crash daemon's `libunwind-ptrace` lookup no longer depends on an earlier `find_package(PkgConfig)` call having run elsewhere in the file. Verified on Linux for the system-libunwind path (static and shared, `SENTRY_BACKEND=native`) and the system-breakpad path, each with pkg-config available, with pkg-config unavailable, and with pkg-config present but the `.pc` module missing; including install and a downstream `find_package(sentry)` build in each case.
Brings in fb337fb (getsentry#2004). The only conflict was CHANGELOG.md, where both getsentry#2004 and this branch appended an entry to the same Unreleased "Fixes" list; both entries are kept, with getsentry#2004 first since it is already on master.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2007 +/- ##
==========================================
- Coverage 74.69% 74.43% -0.27%
==========================================
Files 104 104
Lines 26182 26182
Branches 4740 4740
==========================================
- Hits 19557 19488 -69
- Misses 5295 5370 +75
+ Partials 1330 1324 -6 🚀 New features to boost your workflow:
|
Author
|
@jpnurmi @JoshuaMoelans |
Collaborator
|
The changes look promising, though I've only briefly glanced through them. It makes me wonder whether this is really best solved independently in each individual project, though. sentry-native is certainly not alone here, and the necessary discovery logic gets pretty noisy and error-prone. Just an idea: could this pattern be proposed upstream to CMake as a reusable abstraction? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
SENTRY_LIBUNWIND_SYSTEMandSENTRY_BREAKPAD_SYSTEMlocate their dependencies withfind_package(PkgConfig REQUIRED)+pkg_check_modules(... REQUIRED IMPORTED_TARGET ...), andsentry-config.cmake.inrepeats that for consumers of a static build:CMakeLists.txt:708—SENTRY_WITH_LIBUNWIND(Linux only) +SENTRY_LIBUNWIND_SYSTEMCMakeLists.txt:806—SENTRY_BACKEND_BREAKPAD+SENTRY_BREAKPAD_SYSTEMCMakeLists.txt:937— the crash daemon'slibunwind-ptracelookupsentry-config.cmake.in:19,24— the same two lookups, at consumer timeThose are the only places in this project that touch
pkg-config, they are all Linux-only, and both options default toOFF(the only other user is vendored crashpad, on Linux withCRASHPAD_ENABLE_STACKTRACE=ON). But because the requirement is unconditional and hard, packagers end up declaring the tool for every platform they build sentry-native for. vcpkg's port is the concrete example — it listspkgconfas a host dependency of the defaultbackendfeature for!android & !ios, sovcpkg install sentry-native:x64-windowsresolvespkgconf@3.0.3into the install plan even though nothing in the sentry-native build will ever run it.That is not free. Since
pkgconf2.9.90 the release tarball containstests/lib1/të😋st/lib/pkgconfig/utf8.pc(added in pkgconf commit 943a4497a95939c8b9c7aea442b417a193c51220, still present in 3.0.5; vcpkg is on 3.0.3). GitHub'sgit archivestores that name as raw UTF-8 in the 100-byteustarname field — the full path is 64 bytes, so nopaxpath=record is emitted, andgit archiveemits nohdrcharseteither. Windows'tar.exe(libarchive) therefore falls back toarchive_string_default_conversion_for_read()and decodes header names with the machine's legacy code page. Where that code page rejects the bytes,archive_mstring_copy_mbs_len_l()clears every string form of the entry name,archive_entry_pathname_w()returns NULL, and extraction aborts withInvalid empty pathname;tar.exeexits non-zero and vcpkg fails the port. The same failure mode has been reported for other ports whose sources contain non-ASCII paths, e.g. microsoft/vcpkg#43984None of that is sentry-native's bug — but sentry-native is what pulled the tool onto platforms that never needed it. Making the dependency honest is the part that belongs in this repo.
Change
Add
cmake/sentry-find-system-library.cmakewith asentry_find_system_library()helper thatpkg-configmetadata when both the tool and the requested.pcmodule are present (unchanged behaviour, including transitiveRequires:/Libs.private:), andfind_library()/find_path().Both branches define the same imported target —
sentry::libunwind,sentry::libunwind-ptrace,sentry::breakpad-client— so the call sites and the installed config no longer have to know which lookup succeeded. The module is installed next tosentry-config.cmakeand reused by it, so consumers of a static build recreate the targets the same way and no longer needpkg-configthemselves.Two incidental improvements fall out of this:
FindPkgConfigwithCould NOT find PkgConfig (missing: PKG_CONFIG_EXECUTABLE).libunwind-ptracelookup no longer depends on an earlierfind_package(PkgConfig)call having run ~230 lines above it in the same file.The vendored-libunwind default path, non-Linux builds, and builds that do have
pkg-configare untouched.Follow-up (not in this PR)
With this merged,
ports/sentry-native/vcpkg.jsoncan drop itspkgconfhost dependency outright. I verified with the real resolver that doing so removespkgconf(and its ownvcpkg-tool-mesonhost dependency) from the install plan on bothx64-windowsandx64-linux, and that the Linux build still configures, builds, installs and links downstream with nopkg-configon the machine at all. Happy to open that upstream. Worth noting the port's$commenton that dependency is inaccurate today: it attributespkgconfto the breakpad backend, but the port never enablesSENTRY_BREAKPAD_SYSTEM; the real user is the system-libunwind lookup on Linux.Verification
On Linux, for each of three states —
pkg-configavailable /pkg-configphysically removed from the machine /pkg-configpresent but the.pcmodule missing:SENTRY_BACKEND=native -DSENTRY_LIBUNWIND_SYSTEM=ON, static and shared: configure, build,cmake --install, thenfind_package(sentry)from a downstream project, build and run it.masterfails to configure in the "removed" state withCould NOT find PkgConfig; this branch succeeds. Confirmedsentry-crashlinkslibunwind,libunwind-ptraceandlibunwind-genericin every case, and thatsentry-targets.cmakecarriessentry::libunwindin the static link interface.SENTRY_BACKEND=breakpad -DSENTRY_BREAKPAD_SYSTEM=ONagainst a syntheticbreakpad-clientpackage: both branches resolve to the same include root (<prefix>/include/breakpad), matching theclient/<os>/handler/exception_handler.hincludes insrc/backends/sentry_backend_breakpad.cpp.make test-unitstill pass.Supersedes #2006, which closed automatically when its head branch was renamed.