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

Qt6 use bundled dependencies and option to disable features. #9254

Closed
wants to merge 8 commits into from

Conversation

Cyriuz
Copy link
Contributor

@Cyriuz Cyriuz commented Feb 3, 2022

Not sure if this goes against the philosophy of conan but for simplicity and for the sake of using the tested versions of qts dependencies I would like to build with the bundled dependencies instead of using libraries from conan.

I also have a bunch of features disabled which was possible to do with qt5s configure option.


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

@CLAassistant
Copy link

CLAassistant commented Feb 3, 2022

CLA assistant check
All committers have signed the CLA.

@conan-center-bot

This comment has been minimized.

@ghost
Copy link

ghost commented Feb 3, 2022

I detected other pull requests that are modifying qt/6.x.x recipe:

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

recipes/qt/6.x.x/conanfile.py Outdated Show resolved Hide resolved
@ericLemanissier
Copy link
Contributor

Well, using bundled dependency for which we already have a conan package is an anti-pattern, yes.
We would rather understand with you why this is needed in the first place?

On the other hand, the fix for strawberryperl and for disabled features are welcome, thanks !

@ghost ghost mentioned this pull request Feb 4, 2022
4 tasks
@Cyriuz
Copy link
Contributor Author

Cyriuz commented Feb 4, 2022

Well it is all about reducing the complexity of the dependency tree and using "officially approved" versions to make Qt work well. For example we have our own conan server and want to make sure we can build all the packages from source where sub dependencies builds have broken on multiple occasions on different platforms. Basically using the bundled dependencies via qts buildsystem means that we don't have to make sure all the individual conanfiles are working. It is also a feature provided by Qt so it can be argued that it should also be exposed in the conanfile :)

@ericLemanissier
Copy link
Contributor

I would approve this pull request if you used the provides attribute to detect conflicts with conan packages.
typical example is:


if self.options.with_xmp == "bundled":
# recipe has bundled xmp-toolkit-sdk of old version
# avoid conflict with a future xmp recipe
self.provides.append("xmp-toolkit-sdk")

(but I cannot guarantee that other reviewer would approve it. I'm not sure we have an official policy on bundled dependencies)

@conan-center-bot

This comment has been minimized.

@Cyriuz
Copy link
Contributor Author

Cyriuz commented Feb 4, 2022

Didn't know that was possible! 👍

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

This comment has been minimized.

Also cleaned up the bundled dependencies check as it was a bit fragile to have to check for bool values.
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@ericLemanissier
Copy link
Contributor

@Cyriuz don't forget:

Sorry, the build is only launched for Access Request users. You can request access writing in this issue.

@SpaceIm
Copy link
Contributor

SpaceIm commented Feb 10, 2022

Honestly, I'm not in favor to allow vendored dependencies, it's an anti-pattern and increase recipe complexity. It makes options confusing.

@Cyriuz
Copy link
Contributor Author

Cyriuz commented Feb 10, 2022

Okay, well then I'll use my own fork for that. Should I remove those changes from this pr or make a new one?

@conan-center-bot
Copy link
Collaborator

All green in build 7 (3652581b3bc38a3698b2667ab9c7e9e67a23eff2):

  • qt/6.2.1@:
    All packages built successfully! (All logs)

  • qt/6.2.0@:
    All packages built successfully! (All logs)

  • qt/6.2.2@:
    All packages built successfully! (All logs)

  • qt/6.2.3@:
    All packages built successfully! (All logs)

  • qt/6.1.3@:
    All packages built successfully! (All logs)

  • qt/6.0.4@:
    All packages built successfully! (All logs)

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