-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
qcoro: migrate to Conan v2 #18837
base: master
Are you sure you want to change the base?
qcoro: migrate to Conan v2 #18837
Conversation
valgur
commented
Jul 22, 2023
•
edited
Loading
edited
- Closes Library qcoro is not compatible with Conan v2 #25668.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Closing temporarily to avoid unnecessary load on the CI. Will reopen when I'm actively working on the PR again. |
This comment has been minimized.
This comment has been minimized.
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit 97ef690qcoro/0.4.0@#61d49973536d17e28db248685d1181c2
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The CI bot failed to post Conan 2.x results, but here is the link: https://c3i.jfrog.io/c3i/misc-v2/summary.html?json=https://c3i.jfrog.io/c3i/misc-v2/logs/pr/18837/6-windows-msvc/qcoro/0.4.0//summary.json It failed during the test package:
|
This comment has been minimized.
This comment has been minimized.
The Windows build failed for Conan 2.x: |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This comment has been minimized.
This comment has been minimized.
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
don't close |
f53d12e
to
3c9b962
Compare
This comment has been minimized.
This comment has been minimized.
Missing an assigned reviewer as well. @uilianries (sorry for the spam, let me know if these mentions are unnecessary). |
recipes/qcoro/all/conanfile.py
Outdated
# Required for Qt's moc and qtpaths | ||
VirtualRunEnv(self).generate(scope="build") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you really need to inject RunEnv in build scope ? Qt is also in build requirements, so I would have expected that this trick wouldn't be needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After experimenting with adding cross-compilation support for the Qt6 recipe recently, I have a strong suspicion the exported Qt CMake modules in combination with the package_info() are not using the correct version of the tools and always rely on the Qt6::moc
etc targets from the host profile. This also explains why the VirtualRunEnv is necessary when Qt6 has been built with shared=True
.
I suppose the correct solution in this recipe for the time being would be to drop the tool_requires dependency of Qt instead.
recipes/qcoro/all/conanfile.py
Outdated
|
||
def requirements(self): | ||
self.requires("qt/6.3.1") | ||
self.requires("qt/[>=6.6.0 <7]", transitive_headers=True, transitive_libs=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that qcoro is also compatible with Qt5 looking at the logic in this file: https://github.com/qcoro/qcoro/blob/v0.10.0/cmake/QCoroFindQt.cmake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but how would you change the recipe accordingly? Add a "qt": [5, 6]
option? The user is already able to force=True
an older version, if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just change version range by something like this qt/[>=5.15 <7]
. I don't see any reason to add an option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add that, but it probably won't have any real benefit, though. Conan does practically no conflict resolution and will most likely just complain and fail due to the other recipes are not using the newest version that is <7.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would allow for this to be used when users have an extra dependency on QT that is pinned/resolves to a 5.x version.
If the changes to the recipe to support the older version are just allowing it in the version range, I'd go for it with a comment, otherwise if it's more difficult than that, we can keep the current approach
Conan v1 pipeline ✔️Warning Conan Center will stop receiving updates for Conan 1.x packages soon - please see announcement. All green in build 5 (
Conan v2 pipeline ✔️
All green in build 5 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments, thanks!
} | ||
|
||
int main(int argc, char **argv) { | ||
QCoreApplication app(argc, argv); | ||
QTimer::singleShot(0, startTask); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep this call to be able to ensure the compiler is not optimizing away the whole function declaration
recipes/qcoro/all/conanfile.py
Outdated
|
||
def requirements(self): | ||
self.requires("qt/6.3.1") | ||
self.requires("qt/[>=6.6.0 <7]", transitive_headers=True, transitive_libs=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would allow for this to be used when users have an extra dependency on QT that is pinned/resolves to a 5.x version.
If the changes to the recipe to support the older version are just allowing it in the version range, I'd go for it with a comment, otherwise if it's more difficult than that, we can keep the current approach