Add breakpad recipe #5639

ericriff · 2021-05-26T15:34:29Z

Specify library name and version: breakpad/cci.20210521

This is the last missing requirement of sentry-native. https://github.com/conan-io/conan-center-index/blob/master/recipes/sentry-native/all/conanfile.py#L66

Some help is required around the components/cpp_info part since this library only provides pkg config files and I'm not familiar with it, so I'm not sure about how to model it on Conan.

CC: @madebr

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.

ericriff · 2021-05-26T15:34:58Z

recipes/breakpad/all/conanfile.py

+        if self.settings.os == "Linux":
+            self.requires("linux-syscall-support/cci.20200813")
+
+    def _patch_sources(self):


I'll make this a .patch once the PR is ready.

ericriff · 2021-05-26T15:35:46Z

recipes/breakpad/all/conanfile.py

+                "#include <linux_syscall_support.h>"
+            )
+
+        # Let Conan handle fPIC


Is this something we want? Or should we assume that it is hardcoded "for a reason" and don't mess with it?

This is hardcoded so users don't have to change anything to use it in a shared library.

ericriff · 2021-05-26T15:37:23Z

recipes/breakpad/all/conanfile.py

+        tools.rmdir(os.path.join(self.package_folder, "share"))
+        tools.rmdir(os.path.join(self.package_folder, "lib", "pkgconfig"))
+
+    def package_info( self ):


Some help is needed here. sentry-native consumes this package using pkgConfig, but I'm not familiar with that system, so I don't know how to model it on Conan.
https://github.com/getsentry/sentry-native/blob/master/CMakeLists.txt#L439

I'll fork this recipe and open a pr with some fixes.

I think it would be interesting to create a sentry-breakpad recipe (such as I'm doing for sentry-crashpad), to see what components are needs to interface it with sentry-native.

recipes/breakpad/all/conanfile.py

madebr · 2021-05-26T15:42:51Z

recipes/breakpad/all/conanfile.py

+        if self.settings.os == "Linux":
+            self._patch_sources()


I think it's okay to always patch?

Suggested change

if self.settings.os == "Linux":

self._patch_sources()

self._patch_sources()

All the changes are Linux specific, but since the paths to the files are hardcored, E.g. src/client/linux/minidump_writer/line_reader.h I don't know if Windows will take them (doesn't it use \ instead of / for the folder separation?)
Besides I was planning on make this a real patch file once it the PR is stable.

Indeed.
I don't think the autotools build system was written with support for Windows built-in.
I think we should raise a ConanInvalidConfiguration for win.

I don't have a clue what to do with Macos. Throwing an exception would be fine for me too.

madebr · 2021-05-26T15:43:20Z

recipes/breakpad/all/conanfile.py

+        tools.rmdir(os.path.join(self.package_folder, "share"))
+        tools.rmdir(os.path.join(self.package_folder, "lib", "pkgconfig"))
+
+    def package_info( self ):


I'll fork this recipe and open a pr with some fixes.

recipes/breakpad/config.yml

madebr · 2021-05-26T15:44:56Z

recipes/breakpad/all/conanfile.py

+        return "source_subfolder"
+
+    def config_options(self):
+        if self.settings.os == "Windows":


Does it build on Windows?

I tried to build it with gyp, but I couldn't get it working.

madebr · 2021-05-26T15:46:12Z

recipes/breakpad/all/conanfile.py

+                "#include <linux_syscall_support.h>"
+            )
+
+        # Let Conan handle fPIC


This is hardcoded so users don't have to change anything to use it in a shared library.

Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

ericriff · 2021-05-26T16:26:19Z

I'm wondering why the test_package fails to run on Macos

/Users/jenkins/w/BuildSingleReference@2/conan-center-index/recipes/breakpad/all/test_package/test_package.cpp:2:10: fatal error: 'client/linux/handler/exception_handler.h' file not found
#include "client/linux/handler/exception_handler.h"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I'm so smart

madebr · 2021-05-26T20:34:16Z

recipes/breakpad/all/test_package/conanfile.py

+
+    def test(self):
+        if not tools.cross_building(self.settings):
+            bin_path = os.path.join("bin", "test_package")


This one does not need to use the binaries in the bin package folder? (as the crashpad test package does?)

I think everything in bin are tools.

Yup. I was asking whether breakpad has a handler binary, such as crashpad.

Nop. Breakpad is easier to use since you just link the library to your app. No need to ship an extra handler.

does not match this

@prince-chrismc
breakpad ships utility executables to examine stacktraces post-crash, but no crashpad_handler-like executable that must be executed in parallel with the victim-executable.

Ahhhhhh thank you for explaining

madebr · 2021-05-26T22:48:51Z

I've opened ericriff#6

A recipe for sentry-breakpad can be found at https://github.com/madebr/conan-center-index/tree/sentry_breakpad
I will open a pr adding that one after #5536 gets merged.

ericriff · 2021-05-27T14:35:01Z

Thanks for the help with the components part!
According to the docs this library can be used on Linux, MacOS and Windows. No instructions on how to build though.
On MacOS this builds as is, but the test_package doesn't build since I included a header of client/linux. I inspected the packages built locally and I only have linux headers, so I'm guessing the build system only installs the headers related to the OS you're targeting.
Maybe I can make the test_package work using some #ifdef to change the code built. Any recommendation of which macro to check on the #ifdef?

madebr · 2021-05-27T14:36:42Z

This is the build script by sentry. It only builds the client library.
https://github.com/getsentry/sentry-native/blob/master/external/CMakeLists.txt

See https://github.com/madebr/conan-center-index/tree/sentry_breakpad

The autotools build script by google only supports Linux.

ericriff · 2021-05-27T14:50:36Z

I cleaned up the recipe to remove all the OS checks since it is now Linux only, here e701de9

I can revert the commit if you think there is some value on keeping that around for the future.

conan-center-bot · 2021-05-27T22:39:00Z

All green in build 9 (53719d9520b68383dc119973ba2e725034532ed5):

breakpad/cci.20210521@:
All packages built successfully! (All logs)

prince-chrismc

Little contradiction, I flagged lines/converstation in question

prince-chrismc · 2021-05-28T12:46:38Z

recipes/breakpad/all/conanfile.py

+        bindir = os.path.join(self.package_folder, "bin")
+        self.output.info("Appending PATH environment variable: {}".format(bindir))
+        self.env_info.PATH.append(bindir)


prince-chrismc · 2021-05-28T12:46:51Z

recipes/breakpad/all/test_package/conanfile.py

+
+    def test(self):
+        if not tools.cross_building(self.settings):
+            bin_path = os.path.join("bin", "test_package")


does not match this

prince-chrismc · 2021-06-04T13:47:49Z

@madebr Since you are dependant on this PR you can review it 😝

* Add breakpad recipe * Build test_package with CXX 11 * Use 2 spaces to indent Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com> * Use double quotes and specify 'provides' Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com> * breakpad: add components * Raise if os != Linux * Cleanup recipe since it is Linux only * Move source patching to actual patch files Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com> Co-authored-by: Anonymous Maarten <anonymous.maarten@gmail.com>

Add breakpad recipe

2b0a9f5

ericriff commented May 26, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

conan-center-bot added the Failed label May 26, 2021

madebr reviewed May 26, 2021

View reviewed changes

Build test_package with CXX 11

fae5707

This comment has been minimized.

Sign in to view

ericriff and others added 2 commits May 26, 2021 13:21

Use 2 spaces to indent

db5adbc

Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

Use double quotes and specify 'provides'

5e9bc80

Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

This comment has been minimized.

Sign in to view

madebr reviewed May 26, 2021

View reviewed changes

breakpad: add components

6508e37

This comment has been minimized.

Sign in to view

Raise if os != Linux

1a29542

ericriff force-pushed the eriff/add-breakpad branch from 744cc38 to 1a29542 Compare May 27, 2021 14:47

Cleanup recipe since it is Linux only

e701de9

This comment has been minimized.

Sign in to view

conan-center-bot removed the Failed label May 27, 2021

Move source patching to actual patch files

53719d9

SSE4 requested a review from uilianries May 28, 2021 07:40

prince-chrismc reviewed May 28, 2021

View reviewed changes

prince-chrismc approved these changes May 28, 2021

View reviewed changes

github-actions bot mentioned this pull request May 28, 2021

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

Open

conan-center-bot requested review from danimtb and SSE4 May 31, 2021 11:57

madebr mentioned this pull request Jun 2, 2021

sentry-breakpad: add recipe #5737

Merged

4 tasks

SSE4 approved these changes Jun 3, 2021

View reviewed changes

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

madebr approved these changes Jun 4, 2021

View reviewed changes

conan-center-bot merged commit d0ce968 into conan-io:master Jun 4, 2021

SpaceIm mentioned this pull request Jun 5, 2021

[request] some of most relevant c++ libraries in github (by number of stars) #621

Closed

	if self.settings.os == "Linux":
	self._patch_sources()
	self._patch_sources()

Add breakpad recipe #5639

Add breakpad recipe #5639

Conversation

ericriff commented May 26, 2021 • edited Loading

Choose a reason for hiding this comment

ericriff May 26, 2021 • edited Loading

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.

Choose a reason for hiding this comment

ericriff May 26, 2021 • edited Loading

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.

ericriff commented May 26, 2021 • edited Loading

This comment has been minimized.

madebr May 26, 2021 • edited Loading

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

madebr commented May 26, 2021

ericriff commented May 27, 2021 • edited Loading

madebr commented May 27, 2021 • edited Loading

This comment has been minimized.

This comment has been minimized.

ericriff commented May 27, 2021 • edited Loading

This comment has been minimized.

conan-center-bot commented May 27, 2021

prince-chrismc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prince-chrismc commented Jun 4, 2021

ericriff commented May 26, 2021 •

edited

Loading

ericriff May 26, 2021 •

edited

Loading

ericriff May 26, 2021 •

edited

Loading

ericriff commented May 26, 2021 •

edited

Loading

madebr May 26, 2021 •

edited

Loading

ericriff commented May 27, 2021 •

edited

Loading

madebr commented May 27, 2021 •

edited

Loading

ericriff commented May 27, 2021 •

edited

Loading