-
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
qt: use upstream package folder architecture #24193
Merged
conan-center-bot
merged 4 commits into
conan-io:master
from
kambala-decapitator:qt-directory-structure
Jun 11, 2024
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
When trying to build locally to test this out, I'm finding that I'm hitting this assert. Any idea how to tackle it?
I'm building in
if that helps :)
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.
wanted to test on my side but Qt 6.7.1 build failed, will try with a different version later. Testing with v1 and
(Xcode 15.4)
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.
Hi! If using Conan 2.4, you might want to use this fix #24236 for it to compile (Or wait until it gets merged in a few hours/tomorrow and update this PR with the changes)
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.
in my case build error was from Qt, and that PR is about Windows :)
just built with
-o 'qt/*:essential_modules=False'
and all went fine, didn't face that error. Could you post exact conan command you issued and the exact assert message (i.e. which executable wasn't found)?Also pipeline was green...
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.
At the moment this is unfortunately misleading, because the recipe is currently set to build on CI with different options than a user would get, see: #23911
This is wrong and needs to be fixed - however because of the large amount of versions we are maintaining, this would result in very long CI builds.
Will try to troubleshoot this locally, the expectation is that with
-o 'qt/*:essential_modules=True'
the changes in this PR (which are otherwise great) should not result in failuresThere 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 get this too, with a simple
keeping in mind that this causes
essential_modules=True
(by default)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.
Why is this being changed from a warning to an assert in this PR?
On master it doesn't find this tool either
qmltestrunner
, but it is a warning.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.
qmltestrunner
is not a documented qt app as far as I can find, and it seems like its only built when certain conditions are met (including, potentially, the building of tests) - so I've added a commit to remove it from the list of checked executables, until we can work out if this is needed and how to fix thisas a side note, and I appreciate the changes on this PR existed previously, it is very difficult to review pull requests when there are changes that have nothing to do with the PR description :(
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.
Tried building with v2 and faced that error as well.
Also input from Eric:
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.
qmltestrunner
is not packaged - either before or after the changes in this PR, because it is not being built.The problem exists outside of the changes in this PR and it unrelated - so let's address the package structure first.