sentry-breakpad: add recipe #5737

madebr · 2021-06-02T20:48:59Z

Specify library name and version: sentry-breakpad/all

Add sentry's fork of breakpad
Versions are the same as those of sentry-native.
See [package] sentry-native should use breakpad as a backend on Linux. #5067
After merging of this pr and Add breakpad recipe #5639, sentry-native will have the possibility to switch between google's original and sentry's fork (same approach as in sentry-native: add option to switch crashpad dependency #5717)

I've read the guidelines for contributing.
I've followed the PEP8 style guides for Python code in the recipes.
I've used the latest Conan client version.
I've tried at least one configuration locally with the
conan-center hook activated.

SSE4 · 2021-06-03T08:34:16Z

recipes/sentry-breakpad/all/conandata.yml

+sources:
+  "0.4.9":
+    url: "https://github.com/getsentry/sentry-native/releases/download/0.4.9/sentry-native.zip"
+    sha256: "559344bad846b7868c182b22df0cd9869c31ebf46a222ac7a6588aab0211af7d"


do you need all these minor versions?

They're the same versions the sentry-native recipe provides.

We can create them once, but they might be removed from the repository in the future (like patch versions from cmake, meson,...). IMO It can be good to have them available in the remote for at least one revision.

ericriff · 2021-06-03T17:01:03Z

recipes/sentry-breakpad/all/conanfile.py

+        # FIXME: convert to patches
+        import textwrap
+
+        files_to_patch = [


You can take the patch from my PR, it should work.
https://github.com/conan-io/conan-center-index/pull/5639/files#diff-46f93bc4248cb2a5f042278e731395de3ffdbdc16f1c3e78935b713a652d28f7

ericriff · 2021-06-03T17:04:24Z

recipes/sentry-breakpad/all/conanfile.py

+        cmake.install()
+
+    def package_info(self):
+        self.cpp_info.names["pkg_config"] = "breakpad-client"


Suggested change

self.cpp_info.names["pkg_config"] = "breakpad-client"

self.cpp_info.names["pkg_config"] = "breakpad-client"

self.cpp_info.names["cmake_find_package"] = "breakpad"

self.cpp_info.names["cmake_find_package_multi"] = "breakpad"

So the package is a drop in replacement to google/breakpad #5639

Is that the provides attribute?

I mean from CMake's perspective. With this change you can have have find_package(breakpad) regardless of the breakpad package you're using.

I think the provides only prevents two packages with the same libraries/APIs from colliding.

@prince-chrismc
This is done to switch between breakpad and sentry-breakpad seamlessly.
sentry-breakpad is a forked version that, so far I know, don't has any intention to become standalone.

I think it doesn't hurt to have it even if we're not planning on someone using it as a standalone version.
I can totally see someone trying to do it since the official version only builds on Linux.

ericriff · 2021-06-03T17:05:13Z

recipes/sentry-breakpad/all/conanfile.py

+
+class SentryBreakpadConan(ConanFile):
+    name = "sentry-breakpad"
+    description = "A set of client and server components which implement a crash-reporting system."


Only client in this case?

Yes indeed.
I don't think it matters much, but I'll change it.

ericriff · 2021-06-03T17:06:54Z

recipes/sentry-breakpad/all/test_package/CMakeLists.txt

+
+add_executable(${PROJECT_NAME} test_package.cpp)
+target_link_libraries(${PROJECT_NAME} PRIVATE PkgConfig::BREAKPAD)
+set_property(TARGET ${PROJECT_NAME} PROPERTY CXX_STANDARD 11)


I guess this is OK but upstream also sets C_STANDARD to 11
https://github.com/getsentry/sentry-native/blob/master/CMakeLists.txt#L28-L34

I added this.

prince-chrismc · 2021-06-04T13:59:19Z

recipes/sentry-breakpad/all/CMakeLists.txt

+if(CMAKE_SYSTEM_NAME MATCHES "Linux")
+    set(LINUX ON)
+endif()


This looks a little odd

This is logic from the main sentry-native cmake script that is repeated in the breakpad cmake script.
It's really crucial to have the LINUX variable defined.

For the next guy?

Suggested change

if(CMAKE_SYSTEM_NAME MATCHES "Linux")

set(LINUX ON)

endif()

if(CMAKE_SYSTEM_NAME MATCHES "Linux")

set(LINUX ON) # See https://github.com/conan-io/conan-center-index/pull/5737#discussion_r646140190

endif()

I like that comment.

prince-chrismc · 2021-06-04T14:01:03Z

recipes/sentry-breakpad/all/conanfile.py

+            tools.check_min_cppstd(self, 11)
+
+    def source(self):
+        tools.get(**self.conan_data["sources"][self.version], destination=self._source_subfolder)


strip_root?

sentry-native's releases don't have a root folder.
All files are in the root folder.
See https://github.com/getsentry/sentry-native/releases/tag/0.4.9 (the manually uploaded sentry-native.zip)

prince-chrismc · 2021-06-04T14:01:14Z

recipes/sentry-breakpad/all/conanfile.py

+        if self._cmake:
+            return self._cmake
+        self._cmake = CMake(self)
+        self._cmake.configure()


Out of source build?

Totally possible, not that it will change anything 😄

Reverted since I don't know a built-in way to refer to the generated conanbuildinfo.cmake

prince-chrismc · 2021-06-04T14:02:17Z

recipes/sentry-breakpad/all/conanfile.py

+        cmake.install()
+
+    def package_info(self):
+        self.cpp_info.names["pkg_config"] = "breakpad-client"


Is that the provides attribute?

conan-center-bot · 2021-06-06T21:22:44Z

All green in build 9 (6ebf26c5be5107399f1d00fd01c22a8237af2e5a):

sentry-breakpad/0.4.9@:
All packages built successfully! (All logs)
sentry-breakpad/0.4.8@:
All packages built successfully! (All logs)
sentry-breakpad/0.4.7@:
All packages built successfully! (All logs)
sentry-breakpad/0.4.1@:
All packages built successfully! (All logs)
sentry-breakpad/0.2.6@:
All packages built successfully! (All logs)

ericriff · 2021-06-07T00:16:43Z

recipes/sentry-breakpad/all/conanfile.py

+        for patch in self.conan_data.get("patches", {}).get(self.version, []):
+            tools.patch(**patch)
+
+        # FIXME: convert to patches


Is this FIXME on the scope of this PR? Or it will be addressed in the future?

I waited to convert them until I got it building successfully on c3i.
I will convert them asap.

uilianries · 2021-06-10T20:20:12Z

recipes/sentry-breakpad/all/CMakeLists.txt

+if(CMAKE_SYSTEM_NAME MATCHES "Linux")
+    set(LINUX ON)
+endif()


I like that comment.

* sentry-breakpad: add recipe * sentry-breakpad: needs c++11 * sentry-breakpad: bump minimum required cmake version * sentry-breakpad: implement feedback + MSVC support * sentry-breakpad: no build_subfolder * sentry-breakpad: add macos support to test_package * sentry-breakpad: don't do global use namespace * sentry-breakpad: older versions don't support apple or android * sentry-breakpad: old versions do not support Windows as well

madebr mentioned this pull request Jun 2, 2021

[package] sentry-native should use breakpad as a backend on Linux. #5067

Closed

This comment has been minimized.

Sign in to view

conan-center-bot added the Failed label Jun 2, 2021

madebr added 2 commits June 2, 2021 23:26

sentry-breakpad: add recipe

4f6e367

sentry-breakpad: needs c++11

55b4e65

madebr force-pushed the sentry_breakpad branch from 4c6cd3d to 55b4e65 Compare June 2, 2021 21:39

This comment has been minimized.

Sign in to view

sentry-breakpad: bump minimum required cmake version

7f6d449

This comment has been minimized.

Sign in to view

conan-center-bot removed the Failed label Jun 2, 2021

github-actions bot mentioned this pull request Jun 2, 2021

Pull Requests Ready to be Reviewed prince-chrismc/conan-center-index-pending-review#1

Open

SSE4 requested a review from uilianries June 3, 2021 08:33

SSE4 reviewed Jun 3, 2021

View reviewed changes

SSE4 previously approved these changes Jun 3, 2021

View reviewed changes

ericriff reviewed Jun 3, 2021

View reviewed changes

prince-chrismc reviewed Jun 4, 2021

View reviewed changes

sentry-breakpad: implement feedback + MSVC support

48418da

madebr dismissed SSE4’s stale review via 48418da June 6, 2021 14:59

This comment has been minimized.

Sign in to view

conan-center-bot added the Failed label Jun 6, 2021

sentry-breakpad: no build_subfolder

02bc23e

This comment has been minimized.

Sign in to view

sentry-breakpad: add macos support to test_package

e08d9e9

This comment has been minimized.

Sign in to view

sentry-breakpad: don't do global use namespace

3e39eff

This comment has been minimized.

Sign in to view

sentry-breakpad: older versions don't support apple or android

2a90125

This comment has been minimized.

Sign in to view

sentry-breakpad: old versions do not support Windows as well

6ebf26c

conan-center-bot removed the Failed label Jun 6, 2021

prince-chrismc approved these changes Jun 6, 2021

View reviewed changes

ericriff reviewed Jun 7, 2021

View reviewed changes

conan-center-bot requested a review from jgsogo June 8, 2021 11:59

uilianries approved these changes Jun 10, 2021

View reviewed changes

conan-center-bot requested a review from memsharded June 11, 2021 11:59

SSE4 approved these changes Jun 11, 2021

View reviewed changes

conan-center-bot merged commit 3407e05 into conan-io:master Jun 11, 2021

madebr deleted the sentry_breakpad branch September 8, 2023 12:37

sentry-breakpad: add recipe #5737

sentry-breakpad: add recipe #5737

Conversation

madebr commented Jun 2, 2021 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

conan-center-bot commented Jun 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madebr commented Jun 2, 2021 •

edited

Loading