Skip to content

build: make pkg-config optional for the system libunwind/breakpad options - #2007

Open
mertefesensoy wants to merge 4 commits into
getsentry:masterfrom
mertefesensoy:pkg-config-optional/libunwind/breakpad
Open

build: make pkg-config optional for the system libunwind/breakpad options#2007
mertefesensoy wants to merge 4 commits into
getsentry:masterfrom
mertefesensoy:pkg-config-optional/libunwind/breakpad

Conversation

@mertefesensoy

@mertefesensoy mertefesensoy commented Aug 20, 2026

Copy link
Copy Markdown

Problem

SENTRY_LIBUNWIND_SYSTEM and SENTRY_BREAKPAD_SYSTEM locate their dependencies with find_package(PkgConfig REQUIRED) + pkg_check_modules(... REQUIRED IMPORTED_TARGET ...), and sentry-config.cmake.in repeats that for consumers of a static build:

  • CMakeLists.txt:708SENTRY_WITH_LIBUNWIND (Linux only) + SENTRY_LIBUNWIND_SYSTEM
  • CMakeLists.txt:806SENTRY_BACKEND_BREAKPAD + SENTRY_BREAKPAD_SYSTEM
  • CMakeLists.txt:937 — the crash daemon's libunwind-ptrace lookup
  • sentry-config.cmake.in:19,24 — the same two lookups, at consumer time

Those are the only places in this project that touch pkg-config, they are all Linux-only, and both options default to OFF (the only other user is vendored crashpad, on Linux with CRASHPAD_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 lists pkgconf as a host dependency of the default backend feature for !android & !ios, so vcpkg install sentry-native:x64-windows resolves pkgconf@3.0.3 into the install plan even though nothing in the sentry-native build will ever run it.

That is not free. Since pkgconf 2.9.90 the release tarball contains tests/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's git archive stores that name as raw UTF-8 in the 100-byte ustar name field — the full path is 64 bytes, so no pax path= record is emitted, and git archive emits no hdrcharset either. Windows' tar.exe (libarchive) therefore falls back to archive_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 with Invalid empty pathname; tar.exe exits 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#43984

None 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.cmake with a sentry_find_system_library() helper that

  • prefers the pkg-config metadata when both the tool and the requested .pc module are present (unchanged behaviour, including transitive Requires: / Libs.private:), 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 have to know which lookup succeeded. The module is installed next to sentry-config.cmake and reused by it, so consumers of a static build recreate the targets the same way and no longer need pkg-config themselves.

Two incidental improvements fall out of this:

  • A missing dependency now reports what was not found and how to fix it, instead of failing inside FindPkgConfig with Could NOT find PkgConfig (missing: PKG_CONFIG_EXECUTABLE).
  • The crash daemon's libunwind-ptrace lookup no longer depends on an earlier find_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-config are untouched.

Follow-up (not in this PR)

With this merged, ports/sentry-native/vcpkg.json can drop its pkgconf host dependency outright. I verified with the real resolver that doing so removes pkgconf (and its own vcpkg-tool-meson host dependency) from the install plan on both x64-windows and x64-linux, and that the Linux build still configures, builds, installs and links downstream with no pkg-config on the machine at all. Happy to open that upstream. Worth noting the port's $comment on that dependency is inaccurate today: it attributes pkgconf to the breakpad backend, but the port never enables SENTRY_BREAKPAD_SYSTEM; the real user is the system-libunwind lookup on Linux.

Verification

On Linux, for each of three states — pkg-config available / pkg-config physically removed from the machine / pkg-config present but the .pc module missing:

  • SENTRY_BACKEND=native -DSENTRY_LIBUNWIND_SYSTEM=ON, static and shared: configure, build, cmake --install, then find_package(sentry) from a downstream project, build and run it. master fails to configure in the "removed" state with Could NOT find PkgConfig; this branch succeeds. Confirmed sentry-crash links libunwind, libunwind-ptrace and libunwind-generic in every case, and that sentry-targets.cmake carries sentry::libunwind in the static link interface.
  • SENTRY_BACKEND=breakpad -DSENTRY_BREAKPAD_SYSTEM=ON against a synthetic breakpad-client package: both branches resolve to the same include root (<prefix>/include/breakpad), matching the client/<os>/handler/exception_handler.h includes in src/backends/sentry_backend_breakpad.cpp.
  • The not-found path produces the intended diagnostic.
  • Default (vendored libunwind) build and make test-unit still pass.

Supersedes #2006, which closed automatically when its head branch was renamed.

…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.
Copilot AI lite review requested due to automatic review settings August 20, 2026 10:33

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@codecov

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.43%. Comparing base (69639a0) to head (2a6bac0).

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:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mertefesensoy

Copy link
Copy Markdown
Author

@jpnurmi @JoshuaMoelans
Could one of you take a look, or point me at the right reviewer? Build-system only (CMake + the exported config), CI green on all platforms, branch up to date with master.

@jpnurmi

jpnurmi commented Aug 21, 2026

Copy link
Copy Markdown
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?

Reviewer feedback on the previous version: the discovery logic was
bespoke and noisier than it needed to be. It is -- CMake already treats
this as a solved pattern, and 14 of the 162 find modules it ships follow
it. `FindLibinput` is the closest match.

Restructure `sentry_find_system_library()` the same way:

* `pkg-config` supplies `HINTS` for `find_library()`/`find_path()`
  instead of being a second code path that returns a `PkgConfig::`
  target. One lookup now, not two, so there is no branch to get wrong.
* `find_package_handle_standard_args()` replaces the hand-rolled
  `message(FATAL_ERROR)`, which brings the standard "Could NOT find X
  (missing: ...)" diagnostic and the usual found/required handling.
* Carry `CFLAGS_OTHER` from the `.pc` onto the imported target when
  pkg-config did resolve, which the previous version dropped.

The `pkg_check_modules()` call stays guarded: the command is defined by
`FindPkgConfig` itself, so it does not exist when that module was never
loaded (for instance under `CMAKE_DISABLE_FIND_PACKAGE_PkgConfig`).

One behavioural consequence: with a single lookup path there is no
transitive `Requires:` resolution, so `libunwind-ptrace` names
`libunwind-generic` itself. The previous fallback already did this, and
the daemon's `readelf -d` output is unchanged either way. `pthread` from
`breakpad-client.pc` is likewise no longer picked up implicitly, but
sentry links `Threads::Threads` independently via `SENTRY_LINK_PTHREAD`.

Re-verified on Linux for system libunwind (static and shared,
`SENTRY_BACKEND=native`) and system breakpad, in each of three states --
pkg-config installed, pkg-config physically removed from the machine,
and pkg-config present with the `.pc` module missing -- including
install and a downstream `find_package(sentry)` build and run, plus the
not-found diagnostic and `make test-unit`.
@mertefesensoy

Copy link
Copy Markdown
Author

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?

Fair point. CMake already has a convention for this: FindLibinput.cmake and 13 others it ships use pkg_check_modules(... QUIET) for hints, then find_path/find_library plus find_package_handle_standard_args. Switched the helper to that shape, so it’s a single lookup path now.

@mertefesensoy

Copy link
Copy Markdown
Author

On upstreaming, I think FPHSA plus that convention is the abstraction, since each library still needs its own names and deps. CMake doesn’t ship a FindLibunwind though, which could be worth proposing separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants