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

QArchive: Add version 2.2.2 and modernize #14212

Merged
merged 11 commits into from
Dec 2, 2022

Conversation

ericriff
Copy link
Contributor

@ericriff ericriff commented Nov 15, 2022

Add support for the latest version and modernize recipe and test packages.


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

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@ericriff ericriff changed the title QArchive: Add version 2.2.1 QArchive: Add version 2.2.1 and modernize Nov 16, 2022
@conan-center-bot

This comment has been minimized.

Co-authored-by: Uilian Ries <uilianries@gmail.com>
@conan-center-bot

This comment has been minimized.

@ericriff ericriff changed the title QArchive: Add version 2.2.1 and modernize QArchive: Add version 2.2.2 and modernize Nov 17, 2022
@ericriff
Copy link
Contributor Author

The dev kindly created a new tag with antony-jr/QArchive#62 so I don't have to include that as a patch here

@conan-center-bot

This comment has been minimized.

@ericriff ericriff requested a review from uilianries November 17, 2022 20:25
prince-chrismc
prince-chrismc previously approved these changes Nov 18, 2022
@antony-jr antony-jr mentioned this pull request Nov 22, 2022
4 tasks
@antony-jr
Copy link

antony-jr commented Nov 22, 2022

@ericriff v2.2.3 adds a commit for using c++17 standard when Qt6 is used, like how quazip does. Qt6 requires c++17 but we use c++11 by default in versions older than 2.2.3, so if you use v2.2.3 for conan, the package can be used with Qt6 also.

Also you might want to set -DQARCHIVE_QT_VERSION_MAJOR=6 when building with Qt6 in conan. Kind of what quazip does now,

cmake.definitions["QUAZIP_QT_MAJOR_VERSION"] = self._qt_major

@ericriff
Copy link
Contributor Author

ericriff commented Nov 22, 2022

I updated the recipe to use the new tagged version and I'm passing QARCHIVE_QT_VERSION_MAJOR based on the version of Qt. Regardless, I don't like this pattern. I'm only following it to be consistent with Quazip but I believe this should be an option instead, something like with_qt that defaults to 5 but also 6 can be selected. Then we can change the requires accordingly (and also this CMake var). As it is now, if you want Qarchive/Quazip with qt6 you have to edit the recipe.

@conan-center-bot

This comment has been minimized.

@ericriff ericriff requested review from prince-chrismc and removed request for uilianries November 23, 2022 02:17
@antony-jr
Copy link

antony-jr commented Nov 23, 2022

I updated the recipe to use the new tagged version and I'm passing QARCHIVE_QT_VERSION_MAJOR based on the version of Qt. Regardless, I don't like this pattern. I'm only following it to be consistent with Quazip but I believe this should be an option instead, something like with_qt that defaults to 5 but also 6 can be selected. Then we can change the requires accordingly (and also this CMake var). As it is now, if you want Qarchive/Quazip with qt6 you have to edit the recipe.

In QArchive build, If the cmake variable is not set then cmake uses the default version of Qt found by find package, the variable is only needed to be set if both qt5 and qt6 is installed, to select between the two, but by default its qt5.

prince-chrismc
prince-chrismc previously approved these changes Nov 25, 2022
@prince-chrismc
Copy link
Contributor

What's wrong with the consumer overriding the qt dep to what ever version they want?

I mean --requires and --option will do the same thing and result in different pkg ids 🤔 and the UX is the same "I pick this version of Qt" 🤷 this is less likely to conflict with other recipes which support different majors

@ericriff
Copy link
Contributor Author

I was thinking about having to edit the actual conanfile, but you're right, you can override it without editing the recipe. Please disregard my comment.

@ericriff ericriff requested a review from uilianries November 28, 2022 15:00
Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

Only a small change to keep aligned with the migration guide.

Co-authored-by: Uilian Ries <uilianries@gmail.com>
@ericriff
Copy link
Contributor Author

Is this stuck? 🤔

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline

All green in build 12 (07918afed2e5597e0914f53f4dcb9251c2738479):

  • qarchive/2.2.3@:
    All packages built successfully! (All logs)

  • qarchive/2.1.1@:
    All packages built successfully! (All logs)

  • qarchive/2.0.2@:
    All packages built successfully! (All logs)

  • qarchive/2.0.1@:
    All packages built successfully! (All logs)

Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

LGTM

@conan-center-bot conan-center-bot merged commit 3989cec into conan-io:master Dec 2, 2022
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