fix: remove the SENTRY_CRASHPAD_SYSTEM build option

[supervacuus]

While fixing the breakpad dependency specs in our CMake build here: #926, I saw that we still have the SENTRY_CRASHPAD_SYSTEM option in our build, which was a community contribution.

This option cannot work because we

• added a FirstChanceHandler for Windows some time ago and
• implemented HTTP proxy support to the crashpad_handler back in the spring of this year

Both of these would lead to compile errors (vs. previously, the added support for attachments would only differ at runtime). Since no one complained about these breaking changes, I guess this option is no longer in use.

Of course, we could also keep the option, add a compile-def and would have to keep compatible build-time guarded interfaces in the backend code. I don't know if that is worth it.

This also fixes install dependencies (zlib) for static crashpad builds when adding the Native SDK in a CMake project via find_package().

[supervacuus]

@MartinDelille and @AenBleidd, since you maintain source-level packages of the Native SDK, are you aware of any usage or patches that refer to SENTRY_CRASHPAD_SYSTEM? Or did someone talk to you about any breakage with this option?

It seems the vcpkg one does not interact with the option and leaves this up to the user.

The conan package previously defaulted to the system one (referring to a package installed from our fork, right?), but in your recent change, you defaulted to the vendored version (because sentry's forks of breakpad and crashpad are no longer separate packages). Did I get this right?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (164da79) 82.65% compared to head (80fb17c) 82.64%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #928      +/-   ##
==========================================
- Coverage   82.65%   82.64%   -0.02%     
==========================================
  Files          53       53              
  Lines        7376     7376              
  Branches     1186     1186              
==========================================
- Hits         6097     6096       -1     
- Misses       1171     1173       +2     
+ Partials      108      107       -1     

[MartinDelille]

@supervacuus Thanks for pinging me here! SENTRY_CRASHPAD_SYSTEM is indeed used here and unconditionnaly true: https://github.com/conan-io/conan-center-index/blob/master/recipes/sentry-native/all/conanfile.py#L149

This might be wrong since (if I understand correctly it shall be set to True if we are using other crashpad than the one from sentry) so I might fix it here: conan-io/conan-center-index#20762

[AenBleidd]

Hello @supervacuus, I am not aware about any usage of the SENTRY_CRASHPAD_SYSTEM in the vcpkg, and it's not explicitly set to true. However, we have a small patch here that refers to this flag:
https://github.com/microsoft/vcpkg/blob/master/ports/sentry-native/fix-config-cmake.patch#L45

[supervacuus]

Hmm, I thought I wrote a response on this issue, sorry.

Thanks, @AenBleidd and @MartinDelille, for your responses. As you can see, I removed the build parameter in the (potentially build-breaking) 0.7.0 release. This means the patches for vcpkg will have to be adapted slightly. For the conan package, I would wait to release 0.7.0 until conan-io/conan-center-index#20762 is merged.

If this turns out to be a considerable hassle for your package builds or users, I will revert the removal and add a compile def that limits the interface dependencies to the available upstream APIs in a follow-up release (the actual build-breaker in 0.7.0 is the switch to crashpad as the default backend on Linux).

[MartinDelille]

Hi @supervacuus ! Now that conan-io/conan-center-index#20762 is merged. I could update the recipe.

Shall I stop defining SENTRY_CRASHPAD_SYSTEM from version 0.7.0 or in all case ?

[supervacuus]

Hi @MartinDelille. I would leave SENTRY_CRASHPAD_SYSTEM in previous versions because things might get more complicated than necessary. I cannot imagine anyone using Conan relying on it beyond the separate crashpad/breakpad packages, but the flag still exists and it is only the build that would fail. With 0.7.0 CMake would complain already during the configuration step.

[MartinDelille]

Ok thanks! I implemented this check here: https://github.com/conan-io/conan-center-index/pull/22422/files#diff-95df6adf18543155d3f51239ff9678848b33c8922e0e652daa20175d2d2fc816R158-R160