-
Notifications
You must be signed in to change notification settings - Fork 545
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
Support Qt6 #170
base: rolling
Are you sure you want to change the base?
Support Qt6 #170
Conversation
Signed-off-by: Alejandro Hernandez Cordero <[email protected]>
Signed-off-by: Alejandro Hernandez Cordero <[email protected]>
b4d8dcf
to
0fee1fa
Compare
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
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.
One small thing I think we should fix, this otherwise looks good to me.
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
friendly ping @clalancette |
1 similar comment
friendly ping @clalancette |
turtlesim/CMakeLists.txt
Outdated
find_package(Qt6 COMPONENTS Widgets QUIET) | ||
set(QT_VERSION_MAJOR 6) | ||
if(NOT Qt6_FOUND) | ||
find_package(Qt5 REQUIRED COMPONENTS Widgets) | ||
set(QT_VERSION_MAJOR 5) | ||
endif() |
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'm somewhat concerned about this logic.
In particular, I think we want to continue to use Qt5 right now, but have everything in place so that we could switch to Qt6 in the future.
Because of the way we build binary packages, we will only install Qt5 libraries, and thus we'll continue to build against Qt5 for binaries.
But in from-source builds, it depends on what the user has installed. Thus, I'm a bit afraid that we'll get bug reports from users saying things won't compile properly due to Qt6 being installed. And I don't think we are quite ready for that.
So my suggestion here is to hide this whole thing behind a CMake option, something like USE_QT6
. The the logic would be something like:
if (USE_QT6)
find_package(Qt6 REQUIRED COMPONENTS Widgets)
set(QT_VERSION_MAJOR 6)
else()
find_package(Qt5 REQUIRED COMPONENTS Widgets)
set(QT_VERSION_MAJOR 5)
endif()
What do you think?
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
friendly ping @clalancette |
Support Qt6