qt: fix include path too long for msvc on windows #22151
Merged
Conversation
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
|
馃 Beep Boop! This pull request is making changes to 'recipes/qt//'. 馃憢 @ericLemanissier @jwillikers @MartinDelille you might be interested. 馃槈 |
|
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. |
This was referenced Jan 4, 2024
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Will wait for any of the linked people to weight in. Should this be reported upstream already? Thanks! |
ericLemanissier
suggested changes
Jan 6, 2024
Do I need to enter this issue in qt bug tracker? If we do not integrate this patch, it means qt will not compile with conan until qt is patched and a new version is released? We can do both in parallel no? |
|
Yes, once the patch is reviewed upstream, we can backport it to existing versions before the first release integrating the patch is officially available. |
Hi @fdgStilla - I'm afraid so. We will not accept patches to C or C++ code unless an issue has, at the very least, been reported upstream.
If I'm not mistaken, this is only an issue when certain modules are enabled, correct? And on some systems as well - or this is also an issue on Windows sytems where long paths are enabled?
We can consider doing this in parallel, however I'm not seeing any indication in the PR that any efforts have been made to report this issue upstream. I appreciate the time and effort to investigate and provide a solution to this issue - but we ourselves are not in a position to validate whether this C++ changes align with the intention of the Qt authors: that's a job for them to decide, and we will only accept a patch once they do. Also while your proposed changes mention "fix on Windows" - I am not seeing these being limited to only Windows, but seem to apply to all platforms, regardless. And given the issue description, it may be the case that the issue that we are trying to fix is not exercised by the Conan Center CI service because it's only apparent once certain options/modules are enabled. Thus, I do not have the confidence that this has been tested across all platforms that we intend to support, nor can I tell just by looking at this PR whether this can have unintended consequences for Linux and macOS users. Are the generated headers with long relative paths.. made "public" at all in the final package? Because those headers should not have absolute paths in them. My understanding is that some of the Qt modules are problematic even for Qt's own CI, due to how long some paths are. If there is anything that we can replicate from our end on our infrastructure, without patching C++ code, I would happily look into it. |
|
I posted a bug and a code review on the qt repo: |
ericLemanissier
suggested changes
Jan 12, 2024
Co-authored-by: ericLemanissier <ericLemanissier@users.noreply.github.com>
ericLemanissier
approved these changes
Jan 12, 2024
This comment has been minimized.
This comment has been minimized.
|
The CI is missing system dependencies, does not seem to be linked to my changes |
There was a problem hiding this comment.
It's a bug in conan-center CI: when the recipe is not modified, it reuses the existing binary for the test, but it does not install system dependencies. The workaround is to modify the recipe in order to trigger a new build. you can bump some dependencies for example, or port the patch to 6.3.2 and 6.4.2
|
I fix only qt>6.5.3, the previous versions were using a perl script instead of the c++ file and I am not planning to fix it. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
I have another PR which fails with the same profile, it does not seem to be related to the changes, the CI seems broken 馃槩 |
There was a problem hiding this comment.
I checked the last build of qt6 and compared with this failure:
https://c3i.jfrog.io/c3i/misc/logs/pr/21843/8-macos-m1-clang/qt/6.3.2//7c31d49e6be16219d852fe0a676b2931284e5bc0-build.txt vs https://c3i.jfrog.io/c3i/misc/logs/pr/22151/5-macos-m1-clang/qt/6.3.2//7c31d49e6be16219d852fe0a676b2931284e5bc0-build.txt
The most obvious difference is that the working build uses cmake 3.27 whereas you PR is built with cmake 3.28. I bet you'll have to add this patch to the recipe : qt/qtbase@0efea80
This comment has been minimized.
This comment has been minimized.
This reverts commit 6afe0e7.
@ericLemanissier do you know if this issue is already tracked or if I shall enter a new issue in conan center? |
This comment has been minimized.
This comment has been minimized.
|
I am modifying the recipe but the CI still does not install the system dependencies, I don't understand how the other PR are doing, am I the only one to have this issue? |
There was a problem hiding this comment.
It's not specific to your PR. I don't think this problem has an issue, so do not hesitate to report it.
In this case, the CI did not restart a full build because it noticed that the recipe had already been built. You need to change the recipe to something different from all the recipe already built by this recipe (and also from the current recipe on conan-center).
Doesn't the cmake need to be applied to all versions of qt ?
Also, the patch does not seem to be suited for 6.3.2 and 6.4.2:
qt/6.3.2: Apply patch (bugfix): CMake: Fix build with CMake 3.28 on macOS
qt/6.3.2: WARN: /Users/jenkins/w/prod-v2/bsr/74852/fdeeb/p/qte8a737003be6f/s/patches/fix_cmake3.28.patch: file 1/1: b'cmake/FindWrapOpenGL.cmake'
qt/6.3.2: WARN: /Users/jenkins/w/prod-v2/bsr/74852/fdeeb/p/qte8a737003be6f/s/patches/fix_cmake3.28.patch: hunk no.1 doesn't match source file at line 17
qt/6.3.2: WARN: /Users/jenkins/w/prod-v2/bsr/74852/fdeeb/p/qte8a737003be6f/s/patches/fix_cmake3.28.patch: expected: b''
qt/6.3.2: WARN: /Users/jenkins/w/prod-v2/bsr/74852/fdeeb/p/qte8a737003be6f/s/patches/fix_cmake3.28.patch: actual : b' # On Darwin platforms FindOpenGL sets IMPORTED_LOCATION to the absolute path of the library'
qt/6.3.2: /Users/jenkins/w/prod-v2/bsr/74852/fdeeb/p/qte8a737003be6f/s/patches/fix_cmake3.28.patch: source file is different - b'cmake/FindWrapOpenGL.cmake'
|
I only tested the 'patch' command locally which succeed with a warning: The patch is already backported to 6.6 and 6.5 by qt. |
|
The build is now finished. You can take a look at the results here |
ericLemanissier
approved these changes
Jan 15, 2024
Conan v1 pipeline 鉁旓笍All green in build 9 (
Conan v2 pipeline 鉁旓笍
All green in build 9 ( |
Hooks produced the following warnings for commit e239a3bqt/6.6.0@#e58aef83499eea394aeb3c3358f8a396qt/6.3.2@#10c89c368aecc2af51532256aba04d59qt/6.5.3@#c76b9b1c5fc64d661699e986c5527158qt/6.4.2@#f0daa00d4b8bba65b8742fd7f46b458aqt/6.6.1@#881c2fad703565aee25391c7edfa4cc1 |
ghost
mentioned this pull request
|
@jcar87 could you please lift you request for changes, now that the change has been accepted upstream ? |
jcar87
approved these changes
Jan 17, 2024
|
Approving after reviewing the changes in this PR and taking into account that https://codereview.qt-project.org/c/qt/qtbase/+/529795 has been merged into qtbase. Thanks for reporting this upstream! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fix #21652
Backported from https://codereview.qt-project.org/c/qt/qtbase/+/529795
Qt generates header files in the build folder that include a header file to another relative location in the source folder.
However since the build and the source folder are very distant with conan 2, the concatenated root path + relative path is longer than 260, which is not supported by cl compiler (yes that's very sad to hear in 2024 馃槩 ).
The issue can only be reproduced with qt/*:qtdeclarative=True (which is not set in the conan center CI).
Even if the conan home is very short (like C:\.c) the issue appears.
The fix consists of patching the script used by qt to generate the header, so that the header to the source folder is written with absolute path instead of relative path.
It should not impact the build since it was already their fallback mechanism when the source and the build folder were on different disk (relative path is then impossible).
We could also suggest this fix in the upstream qt repo for the future versions of qt.
Note: I fix only qt>6.5.3, the previous versions were using a perl script instead and I am not planning to fix it for now.