Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sentry-native: add crashpad support #5524

Merged
merged 3 commits into from
May 18, 2021

Conversation

madebr
Copy link
Contributor

@madebr madebr commented May 13, 2021

Specify library name and version: sentry-native/all

Add crashpad support to sentry-native.
Depends on #5511 when using with -o crashpad:backend=crashpad.


  • 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.

@madebr madebr mentioned this pull request May 13, 2021
4 tasks
@ghost
Copy link

ghost commented May 13, 2021

I detected other pull requests that are modifying sentry-native/all recipe:

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

@conan-center-bot

This comment has been minimized.

@SSE4 SSE4 requested a review from uilianries May 14, 2021 07:20
@espkk
Copy link

espkk commented May 15, 2021

This looks nice, but IIRC sentry-native uses own crashpad fork
It basically allows sentry_options_add_attachment to be used. Without it crashpad backend vs raw crashpad does not make much sense...
Am I missing something?

@madebr
Copy link
Contributor Author

madebr commented May 15, 2021

@espkk
You're right. attachment support is a feature of getsentry's fork.
These are the changes between chromium's crahpad and getsentry's: https://github.com/getsentry/crashpad/compare/master..getsentry

I just looked at the release page of sentry-native, and it looks like they provide a sentry-native.zip file with everything included.
https://github.com/getsentry/sentry-native/releases

@conan-center-bot
Copy link
Collaborator

All green in build 2 (9aac11adff9e146069a06136b97ca7810e294f9a):

  • sentry-native/0.4.9@:
    All packages built successfully! (All logs)

  • sentry-native/0.4.7@:
    All packages built successfully! (All logs)

  • sentry-native/0.2.6@:
    All packages built successfully! (All logs)

  • sentry-native/0.4.8@:
    All packages built successfully! (All logs)

  • sentry-native/0.4.1@:
    All packages built successfully! (All logs)

@espkk
Copy link

espkk commented May 15, 2021

I just looked at the release page of sentry-native, and it looks like they provide a sentry-native.zip file with everything included.
https://github.com/getsentry/sentry-native/releases

That's nice! I was sure it is shipped without submodules.
I believe
self.requires("crashpad/cci.20210507") and
self._cmake.definitions["SENTRY_CRASHPAD_SYSTEM"] = True should be removed then
also, self.cpp_info.libs probably should include backend libraries

@madebr
Copy link
Contributor Author

madebr commented May 15, 2021

@espkk
I'm working on a sentry-crashpad recipe at #5536

@espkk
Copy link

espkk commented May 15, 2021

@espkk
I'm working on a sentry-crashpad recipe at #5536

I see... But could you please explain why we need it if it's anyway downloaded in the source bundle?
Considering that the fork already makes it cmake-compatiable I do not understand that additional dependency

@madebr
Copy link
Contributor Author

madebr commented May 15, 2021

Increased modularity.
You can use crashpad from multiple sources: the official sources (this pr) or a fork (sentry's one).

@espkk
Copy link

espkk commented May 15, 2021

Increased modularity.
You can use crashpad from multiple sources: the official sources (this pr) or a fork (sentry's one).

Ok, that makes sense.
Then may I ask you one more question since I'm not familar enough with conan yet.
How in practice can I switch between 'the official sources (this pr) or a fork (sentry's one)'?
I mean, I would expect some package:option that switches SENTRY_CRASHPAD_SYSTEM (and adds/removes self.requires)
Could you please show an example of configuration?

@madebr
Copy link
Contributor Author

madebr commented May 15, 2021

How to switch transparently (or from the command line or using profiles), that I don't know because I will hardcode sentry-crashpad in the requirement list in the sentry-native recipe.

By using provides, conan will complain if you try to mix multiple crashpad packages.

@jgsogo
Copy link
Contributor

jgsogo commented May 18, 2021

There are some recipes that are already modifying the requirements based on some option, you can find examples related to the different JPEG implementations

def requirements(self):

@conan-center-bot conan-center-bot merged commit 3b28cd4 into conan-io:master May 18, 2021
@Dev-vlad
Copy link

Dev-vlad commented Jun 1, 2021

Hi @madebr! I saw that you are working on a recipe for the sentry-crashpad. Do you plan to integrate it with the current one?
I'm not sure how conan ci works, but I'm getting a build error on windows:
sentry_backend_crashpad.cpp(254,31): error C2039: 'SetFirstChanceExceptionHandler': is not a member of 'cra shpad::CrashpadClient'
I've checked the generated projects and the version of crashpad used in the current recipe is not compatible with sentry-native on windows.

@madebr madebr deleted the sentry-crashpad-support branch June 1, 2021 13:12
@madebr
Copy link
Contributor Author

madebr commented Jun 1, 2021

@Dev-vlad
Can you open an issue containing a small reproducer cmake project?
I tested the test package on Linux and it worked.
sentry's fork of crashpad (sentry-crashpad) has also become available on cci,
so I'll add an option to the sentry-native recipe to switch between crashpad and sentry-crashpad.

@madebr
Copy link
Contributor Author

madebr commented Jun 1, 2021

@Dev-vlad
I could reproduce your issue by building sentry-native with MSVC. "It works" on Linux 😄 .
Soon, I'll open a pr.

@Dev-vlad
Copy link

Dev-vlad commented Jun 1, 2021

@madebr, thanks!
Yes, this issue is only for Windows. I checked sentry-crashpad and they have a different API under #ifdef (OS_WIN) that is not available in the original crashpad project.
I don't have much experience with conan, but will think about how we can cover this with a test to make sure not to break this in future.

@madebr
Copy link
Contributor Author

madebr commented Jun 1, 2021

@Dev-vlad
See #5717

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.

None yet

7 participants