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

Add Qt integration to sentry #5440

Merged
merged 15 commits into from
May 25, 2021
Merged

Conversation

MartinDelille
Copy link
Contributor

Specify library name and version: sentry/0.4.9

See the doc here: https://docs.sentry.io/platforms/native/guides/qt/

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

Copy link
Contributor

@ericLemanissier ericLemanissier left a comment

Choose a reason for hiding this comment

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

You could add

if self.options.qt:
    self.requires("qt/....")

recipes/sentry-native/all/conanfile.py Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented May 5, 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.

Co-authored-by: ericLemanissier <ericLemanissier@users.noreply.github.com>
@MartinDelille
Copy link
Contributor Author

@ericLemanissier I personnaly don't use the Qt recipe from CCI yet (I'd love to do so but it isn't very clear for me how to do it, especially with my local environment and Qt Creator). Won't this cause trouble for my setup ?

@ericLemanissier
Copy link
Contributor

If you require another qt recipe in your recipe (qt/...@bincrafters/stable for example), then this qt package will be used instead of CCI's package https://docs.conan.io/en/latest/using_packages/conanfile_txt.html#overriding-requirements

If you are using a qt coming from somewhere else than conan (manually built, official qt binary, or provided by some other package manager), it's true that it could be a problem.

@MartinDelille
Copy link
Contributor Author

This is quite out of topic but I'll be happy to know how people use the CCI qt recipe with Qt Creator for example. Do I need to run conan and then add the kit manually ?

@conan-center-bot

This comment has been minimized.

@madebr
Copy link
Contributor

madebr commented May 5, 2021

You could add

if self.options.qt:
    self.requires("qt/....")

The recipe is missing self.requires("qt/...").
Does this recipe actually build when no qt is installed and configured with -o sentry-native:qt=True?

@SSE4 SSE4 requested a review from uilianries May 6, 2021 07:58
Copy link
Contributor

@SSE4 SSE4 left a comment

Choose a reason for hiding this comment

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

please clarify if Qt integration requires Qt dependency to build/run. I am not familiar with sentry, so it's hard to judge for me right away.
(e.g. I can imagine it may not require an actual Qt if it only generates some headers containing #include <QtWidgets> or similar code)

@MartinDelille
Copy link
Contributor Author

The recipe does indeed need Qt to build if the option is set https://github.com/getsentry/sentry-native/blob/master/CMakeLists.txt#L462-L475

Unfortunately when trying to add the requirement, cmake is unable to find Qt when building sentry-native...

I pushed the update recipe with qt option set to true to share my issue.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@mjvankampen
Copy link
Contributor

In my opinion the CCI Qt recipe is not a full drop in replacement for the official Qt binaries (mostly qml related). So I wouldn't impose Qt as a requirement.

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

@SSE4
Copy link
Contributor

SSE4 commented May 25, 2021

from that I see, it reports merge conflicts which don't exist (at least GitHub doesn't report them):

+ git merge --no-ff 2f71c2f10dfbe4a87595bdc619aeef8262a9e6d0
Removing recipes/vorbis/all/patches/link-libm.patch
Removing recipes/srt/all/patches/002-cmake-link-targets.patch
Removing recipes/srt/all/patches/001-cmake-link-targets.patch
Auto-merging recipes/sentry-native/all/conanfile.py
Auto-merging recipes/sentry-native/all/conandata.yml
CONFLICT (content): Merge conflict in recipes/sentry-native/all/conandata.yml
Removing recipes/pkgconf/all/patches/0002-c89-compatible.patch
Automatic merge failed; fix conflicts and then commit the result.

probably, bug in CI? @jgsogo @danimtb

@SSE4 SSE4 added the infrastructure Waiting on tools or services belonging to the infra label May 25, 2021
@jgsogo
Copy link
Contributor

jgsogo commented May 25, 2021

@MartinDelille , please, push an empty commit to your branch:

git commit -m "touch" --allow-empty
git push

@conan-center-bot
Copy link
Collaborator

All green in build 19 (51be319783e47c1a20921bfcff688bbb9335ce1b):

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

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

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

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

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

}
default_options = {
"shared": False,
"fPIC": True,
"backend": "inproc",
"transport": "curl"
"transport": "curl",
"qt": False,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is qt integration by default useful?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes but at the expense of current users who do no require it

https://docs.sentry.io/platforms/native/guides/qt/#install

Looks off by default from upstream

Copy link
Contributor

Choose a reason for hiding this comment

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

buildroot has some kind of ci system where it builds random combinations to find regressions.
Something similar where non-default option combinations are built would be useful.

@conan-center-bot conan-center-bot merged commit 3768c7f into conan-io:master May 25, 2021
madebr pushed a commit to madebr/conan-center-index that referenced this pull request Jun 21, 2021
* Add Qt integration to sentry

* Update recipes/sentry-native/all/conanfile.py

Co-authored-by: ericLemanissier <ericLemanissier@users.noreply.github.com>

* Add qt requirement

* Set temporary qt to True

* Add openssl/1.1.1j requirement

* Find conan qt patch

* Remove qt requirement

* Restore qt requirement and simpler patch

* Handle older version

* Specific patch for 0.4.7

* Add missing patch

* Move patch to build()

* Bump openssl version to 1.1.1k

Co-authored-by: Chris Mc <prince.chrismc@gmail.com>

* touch

Co-authored-by: ericLemanissier <ericLemanissier@users.noreply.github.com>
Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
@MartinDelille MartinDelille deleted the sentry-qt branch July 15, 2021 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Waiting on tools or services belonging to the infra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet