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

build: Add installation support and fix cmake files #168

Merged
merged 21 commits into from
Mar 12, 2020
Merged

build: Add installation support and fix cmake files #168

merged 21 commits into from
Mar 12, 2020

Conversation

madebr
Copy link
Contributor

@madebr madebr commented Mar 11, 2020

This pr:

  • refactors the cmake build script a bit by:
    • reordering,
    • adding a cmake install configuration script,
    • installing all libraries for shared and dynamic builds of the sentry library
  • fix the enumeration in the readme
  • fix a warning about redefinitions of some compile definitions
  • fix error about wrong calling convention

I tested this on Linux and MSVC

@Amphaal
Copy link
Contributor

Amphaal commented Mar 11, 2020

Changing WIN32 flags to MSVC are indeed required since Linux>Windows cross-compilation will fail to build Crashpad right now.

Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

This is already a lot better!
I do have headaches and nightmares dealing with cmake actually. I’m happy to learn how to effectively use it!

Would be nice if you could check out the failures on CI. Most are just failing due to missing curl, linux fails in the unwinder. not exactly sure what causes that though.

CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@gocarlos
Copy link
Contributor

@madebr are you working on conan-io/conan-center-index#1089?

@madebr
Copy link
Contributor Author

madebr commented Mar 11, 2020

@madebr are you working on conan-io/conan-center-index#1089?

Looks like it 😄
Let's first have upstream a nice cmake build script before adding it to CCI (and having to patch it).

@madebr
Copy link
Contributor Author

madebr commented Mar 11, 2020

@Amphaal

Changing WIN32 flags to MSVC are indeed required since Linux>Windows cross-compilation will fail to build Crashpad right now.

Indeed.

After a small fix to sentry's crashpad cmake scripts (replacing WIN32 with MSVC), the build fails at:
https://github.com/getsentry/crashpad/blob/8a2ddfc0a475bff9e25bfc7af693bac373adebe3/compat/win/sys/types.h#L18-L19

@madebr
Copy link
Contributor Author

madebr commented Mar 12, 2020

I have some questions

  • Until now, I haven't touched libunwindstack and libstacktrace. Are these backends, equivalent to inproc and crashpad?
  • Is libunwindstack a backend or can it be used together with another backend?
  • It looks like libbacktrace is not used. There is only mention of a WITH_LIBBACKTRACE variable, but nothing else.
  • Is CURL support usable on all platforms? Windows, Macos, Linux, Android, IPhone, ...?

@Amphaal
Copy link
Contributor

Amphaal commented Mar 12, 2020

@Amphaal

Changing WIN32 flags to MSVC are indeed required since Linux>Windows cross-compilation will fail to build Crashpad right now.

Indeed.

After a small fix to sentry's crashpad cmake scripts (replacing WIN32 with MSVC), the build fails at:
https://github.com/getsentry/crashpad/blob/8a2ddfc0a475bff9e25bfc7af693bac373adebe3/compat/win/sys/types.h#L18-L19

...And that's only the tip of the iceberg 😆 I'll try to push a patch for the Sentry's Crashpad fork once I get the whole process working with MinGW.

Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

lgtm, would like some input from @jan-auer as well.
I do agree that throwing a hard error when curl is not found is a great idea! Rather than silently compiling without, and having people complain why no reports are actually sent.

src/CMakeLists.txt Outdated Show resolved Hide resolved
tests/__init__.py Show resolved Hide resolved
tests/conftest.py Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@Swatinem Swatinem requested a review from jan-auer March 12, 2020 10:00
@jan-auer jan-auer changed the title Add installation support for cmake + fix various things build: Add installation support and fix cmake files Mar 12, 2020
@Swatinem
Copy link
Member

I just merged breakpad support in #169 , which will cause conflicts here.

Sorry for the churn, and I hope I can learn some cmake skills by looking at how you adapt this PR.

@madebr
Copy link
Contributor Author

madebr commented Mar 12, 2020

A word of caution, all unit tess have now CURL disabled.
But with some pytest magic, I think it can be easily conditionally enabled.

@Swatinem
Copy link
Member

A word of caution, all unit tess have now CURL disabled.

I think thats fine for now. The tests don’t test http yet anyway. I want to get there eventually.

@Swatinem Swatinem merged commit 7acd79f into getsentry:master Mar 12, 2020
@Swatinem
Copy link
Member

Thank you very much @madebr , awesome work!

@madebr madebr deleted the cmake branch March 25, 2020 11:25
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.

5 participants