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

Crashpad recipe #5511

Merged
merged 27 commits into from
May 24, 2021
Merged

Crashpad recipe #5511

merged 27 commits into from
May 24, 2021

Conversation

madebr
Copy link
Contributor

@madebr madebr commented May 10, 2021

Specify library name and version: crashpad/some-git-hash

This packages crashpad.
It also embeds mini_chromium, of which the library + build system is used.

Chromium's mini_chromium project does not have a self-containing gn build system.

Also, Windows is (currently) not supported by this recipe because it requires python 2.x.


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

@ghost
Copy link

ghost commented May 10, 2021

I detected other pull requests that are modifying crashpad/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.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@madebr
Copy link
Contributor Author

madebr commented May 10, 2021

@jgsogo
c3i is telling me to not raise ConanInvalidConfiguration from build(), but I'm not doing that at all (only in validate).
Can you please look at the error in #5511 (comment)?

Thanks!

@ericriff
Copy link
Contributor

This recipe needs components, otherwise it won't be consumable by sentry-native.
https://github.com/getsentry/sentry-native/blob/master/CMakeLists.txt#L396

@madebr
Copy link
Contributor Author

madebr commented May 11, 2021

This recipe needs components, otherwise it won't be consumable by sentry-native.
https://github.com/getsentry/sentry-native/blob/master/CMakeLists.txt#L396

Thanks for noticing.
Although the components are sentry-crashpad specific, I think there is great value in having them here too.
I will add them in the next commits.

@jgsogo
Copy link
Contributor

jgsogo commented May 11, 2021

c3i is telling me to not raise ConanInvalidConfiguration from build(), but I'm not doing that at all (only in validate).
Can you please look at the error in #5511 (comment)?

Thanks!

Found... It surprises me it is the first time this happens. The problem comes from the gn package, it requires C++17. When this package is going to be built, the build-requires are evaluated and gn raises a ConanInvalidConfiguration for some configurations (it is build time for crashpad).

But the binary gn is already available, so this check is not applicable at this time.

(I keep thinking about this issue and solution)

@jgsogo
Copy link
Contributor

jgsogo commented May 11, 2021

After approaching the issue in many different ways, I think that the best we can do is to allow these ConanInvalidConfiguration from the build step. They are not desired, but if we consider them an error, some PRs like this one will be blocked unless we fix a couple of issues in Conan client.

This is my proposal (I've already submitted a PR to the library):

  • short term: this will no longer be an error, instead we will show a warning so the author/reviewer can have a look to it and decide if this is something that needs to be fixed or it is unrelated to this PR.
  • long term: there are a couple of issues/features for Conan client:
    • the command conan info <reference> --build should provide all the information from libraries and build-requires, it should execute the function validate() as well to return if the build-requires can be computed for the given configuration.
    • packageID (and compatible packages) must be checked before raising any exception from validate() if the recipe is not going to be built.

Hopefully it will be approved, merged, and deployed soon. If we go for a different implementation I will let you know.

@jgsogo jgsogo added the blocked Affected by an external issue and waiting until it is solved label May 11, 2021
@madebr
Copy link
Contributor Author

madebr commented May 11, 2021

Thanks for the quick feedback!
I'm glad to hear there's a short-term fix and path to a fix long term.

# FIXME: is this true?
self.output.warn("crashpad needs a shared libcurl library")
if self.settings.os == "Windows":
raise ConanInvalidConfiguration("Windows is not (yet) supported by this recipe because the build system requires python 2.x which is not (yet) available on CCI.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it is better to just patch the sources? Recipe for cpython is being hard.

Choose a reason for hiding this comment

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

Is this really still the case? depot_tools in recent versions should work fine with python 3.8, which AFAIK is the dependency that had the python issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It works on Python 3.9.5 on Linux (works on my machine).
But an error is thrown when building on Windows.
Maybe it can be patched..


self.copy("*.a", src=os.path.join(self._source_subfolder, "out", "Default"), dst="lib", keep_path=False)
self.copy("crashpad_handler", src=os.path.join(self._source_subfolder, "out", "Default"), dst="bin", keep_path=False)
self.copy("crashpad_handler.exe", src=os.path.join(self._source_subfolder, "out", "Default"), dst="bin", keep_path=False)
Copy link

Choose a reason for hiding this comment

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

On Windows there seems to be two executables provided. (missing .com)

https://chromium.googlesource.com/crashpad/crashpad/+/master/doc/overview_design.md

On Windows, this executable is built by default as a Windows GUI app, so no console will appear in normal usage. This is the version that will typically be used. A second copy is also made with a .com extension, rather than .exe. In this second copy, the PE header is modified to indicate that it’s a console app. This is useful because the .com is found in the path before the .exe, so when run normally from a shell using only the basename (without an explicit .com or .exe extension), the .com console version will be chosen, and so stdio will be hooked up as expected to the parent console so that logging output will be visible.

Copy link
Contributor Author

@madebr madebr May 21, 2021

Choose a reason for hiding this comment

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

Fixed in eb7a82a but untested

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it. It's called crashpad_handler_com.com.

Choose a reason for hiding this comment

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

How come it's called crashpad_handler_com.com instead of just crashpad_handler.com as according to the documentation?

@conan-center-bot

This comment has been minimized.

ericriff
ericriff previously approved these changes May 21, 2021
recipes/crashpad/all/conanfile.py Show resolved Hide resolved
Copy link
Contributor

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

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

Couple of comments quickly going over

recipes/crashpad/all/conanfile.py Outdated Show resolved Hide resolved
recipes/crashpad/all/conanfile.py Show resolved Hide resolved
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot
Copy link
Collaborator

All green in build 21 (63a61f8ffd58511a3604843f92deaee69a645c84):

  • crashpad/cci.20210507@:
    All packages built successfully! (All logs)

@prince-chrismc
Copy link
Contributor

Awesome work!

@conan-center-bot conan-center-bot merged commit 2e42473 into conan-io:master May 24, 2021
@madebr madebr deleted the crashpad_recipe branch May 24, 2021 08:43
madebr added a commit to madebr/conan-center-index that referenced this pull request Jun 21, 2021
* sentry: add recipe (WIP)

* switch to github fork

* crashpad: only require libcurl when curl is needed

* crashpad: add windows system libraries

* crashpad: add explicit zlib

* crashpad: add tls option if http_transport=socket + add tls option if http_transport=socket

* crashpad: fixes

* crashpad: various

* crashpad: no Windows for you

* crashpad: print found files in library folder (DEBUG)

* crashpad: handle libs per component

* crashpad: needs CoreFoundation framework

* crashpad: cosmetup changes

* crashpad: apple thingies

* crashpad: fixie

* crashpad: fixie^2

* crashpad: add ninja to build_requires

* crashpad: add components + bump git hash

* crashpad: drop verbose ninja build

* crashpad: crashpad requires c++14 (pass through CMake target)

* crashpad: add provides attribute to recipe

* crashpad: add mini_chromium provides + add bin to PATH env

* crashpad: build crash_handler.com on Windows

* crashpad: use patches + msvc + win_helper.py -> py3

* crashpad: remove unused patch

* crashpad: update comments

* crashpad: check compiler versions in validate()
ericLemanissier added a commit to bincrafters/community that referenced this pull request Oct 18, 2021
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

8 participants