-
Notifications
You must be signed in to change notification settings - Fork 219
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
feat: support both qt5 and qt6 #1187
base: rolling
Are you sure you want to change the base?
Conversation
c9d9a3e
to
b9e5b58
Compare
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.
windows script uses Qt5_DIR here. |
@ahcorde I appreciate it if you could rerun ci. |
@ahcorde The ci still failed, so I modified to use env. |
@ahcorde I'm sorry for the inconvenience, there was a small mistake in statement, so I fixed it. |
I'm sorry, but I have no idea why the windows ci failed. It may take some time to fix the issue. |
One thing you should know about our Qt installation on Windows; it is using a very old version of Qt, version 5.12.10: https://github.com/ros-infrastructure/ros2-cookbooks/blob/8ac2ddf1ceec864211971478bf3d515fbe71298e/cookbooks/ros2_windows/recipes/qt5.rb#L74 That's because after that version, Qt stopped providing downloadable executable files for Windows, so we had no way to auto-install it. In any case, that very old version may not have the |
@ahcorde Could you please help to handle this issue. On the latest waffle triage meeting we decided to move it to the backlog with the following notes: Failure on Windows. Windows setup script hardcoded installation for qt5 only. Qt5 doesn't support the new style find_package(QT NAMES QT6 QT5) https://github.com/ros2/rviz/pull/1187/files#diff-f058f5df521489857a0241a6838777373aa24c99a073c3cd9cf2eee427b167eaR26-R27. A proper way to handle it would be to use conda package manager. It seems a lot of work will need to switch to conda. |
I added workaround for windows ci again. I appreciate it if someone could rerun ci. |
The error says
so I added the following workaround for windows ci
|
I also updated workaround in |
note: need to fix this error
|
Great to see Qt6 support for rviz. The Line 31 in 32eb8b9
Is there a preferred method to optionally include either Qt5 or Qt6 dependencies (or include them both but make them optional)? |
We have conditionals in package.xml, but they can really only select on an environment variable. In reality, though, there will only be one "supported" configuration at a time. That is, we can leave these as Qt5 dependencies for now, and then when we officially switch over, we can change these to Qt6 dependencies. For users of the other one (that is not officially supported), they can still build from source, they just won't be able to successfully run |
@ahcorde @clalancette I appreciate it if you could rerun windows ci. |
Can we make sure to run Windows Debug as well? Otherwise, this is great! |
Sure, added to the list |
@clalancette @ahcorde Is it possible to go ahead to merge? |
@wep21 not really, there are some failing tests on Windows Debug. I will take a look too |
e4b41d5
to
3f4704b
Compare
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 was able to compile rviz2 with qt6, just using the comparison that I mentioned in the comment
rviz_common/CMakeLists.txt
Outdated
@@ -31,7 +31,17 @@ find_package(ament_cmake REQUIRED) | |||
# do find_package(rviz_ogre_vendor) first to make sure the custom OGRE is found | |||
find_package(rviz_ogre_vendor REQUIRED) | |||
|
|||
find_package(Qt5 REQUIRED COMPONENTS Widgets) | |||
if(MSVC) |
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.
instead of using in the condition windows why don't use the Qt version instead?
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.
addressed here 3977093
3f4704b
to
d32501f
Compare
Building |
Signed-off-by: wep21 <[email protected]>
Signed-off-by: wep21 <[email protected]>
Signed-off-by: wep21 <[email protected]>
Signed-off-by: wep21 <[email protected]>
Signed-off-by: wep21 <[email protected]>
Signed-off-by: wep21 <[email protected]>
Signed-off-by: wep21 <[email protected]>
Signed-off-by: wep21 <[email protected]>
Signed-off-by: wep21 <[email protected]>
Signed-off-by: wep21 <[email protected]>
3977093
to
444cfdf
Compare
rework on #707