Skip to content
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

Add the option to build with Qt 6 #4929

Merged
merged 2 commits into from May 10, 2024
Merged

Add the option to build with Qt 6 #4929

merged 2 commits into from May 10, 2024

Conversation

cjmayo
Copy link
Contributor

@cjmayo cjmayo commented Jan 8, 2024

I've tried to do this in the way that makes it easiest to remove Qt 5 down the line.
For now there is no gamepad support with Qt 6.

@cjmayo cjmayo marked this pull request as ready for review January 8, 2024 19:23
@cjmayo
Copy link
Contributor Author

cjmayo commented Jan 8, 2024

Built with Qt 5 here, #4908 updated with this commit to build with Qt 6.

@cjmayo cjmayo mentioned this pull request Feb 3, 2024
@Neumann-A
Copy link

You probably want to avoid the whole code duplication and rather have a variable called OPENSCAD_QT_VERSION which can be either set to 5 or 6 (or auto). See e.g. https://gitlab.kitware.com/vtk/vtk/-/blob/master/CMake/vtkQt.cmake?ref_type=heads

Comment on lines +519 to +533
if (USE_QT6)
if (APPLE AND EXISTS /usr/local/opt/qt@6)
list(APPEND CMAKE_PREFIX_PATH "/usr/local/opt/qt@6")
endif()
if (APPLE AND EXISTS /opt/homebrew/opt/qt@6)
list(APPEND CMAKE_PREFIX_PATH "/opt/homebrew/opt/qt@6")
endif()
else()
if (APPLE AND EXISTS /usr/local/opt/qt@5)
list(APPEND CMAKE_PREFIX_PATH "/usr/local/opt/qt@5")
endif()
if (APPLE AND EXISTS /opt/homebrew/opt/qt@5)
list(APPEND CMAKE_PREFIX_PATH "/opt/homebrew/opt/qt@5")
endif()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is getting a bit messy - do you think there's a way of moving this out of the main CMakeLists file?

Same goes for the Qt5 vs. Qt6 logic. Perhaps a separate macro which deals with the details without making the main CMakeLists file so messy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No doubt that part could be moved out to a module and included, although it is relatively small compared to the entire file (especially when only one version of Qt is supported).

I agree with both of you it ain't pretty. But it isn't all duplication like this, there are some sections which are different, making anything cleaner a more complex proposition. Plus my take was that this is about a managed switch to Qt 6, not maintaining support for both 5 & 6. (I am biased, I have uninstalled Qt 5 completely and don't use a gamepad). Meaning that the Qt 5 code will be deleted and then we are back to the current state, with a few updates. But it is also because I am not a CMake expert, so I'm not sure exactly sure what it would look like.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, let's defer this until we have both Qt5+6 working and have a better picture

@kintel
Copy link
Member

kintel commented Mar 23, 2024

FYI: I managed to build this on macOS - looks good so far

@kintel
Copy link
Member

kintel commented Mar 23, 2024

@cjmayo If you rebase this to master, it should build on macOS too.

With CMAKE_OSX_DEPLOYMENT_TARGET=10.14 and Qt 6.6.2:

/usr/local/include/QtCore/qfile.h:59:58: error: 'path' is unavailable:
introduced in macOS 10.15

/usr/local/include/QtCore/qfile.h:64:40: error: 'native' is unavailable:
introduced in macOS 10.15
@cjmayo
Copy link
Contributor Author

cjmayo commented Mar 24, 2024

Rebased on master.

Good to know it actually runs on Qt 6 and doesn't just build. On Linux can't say I've noticed any difference.

@kintel
Copy link
Member

kintel commented Mar 24, 2024

We can probably merge this soon. I'll do a more proper review, and update #4909 accordingly

@kintel kintel merged commit dc0b6fa into openscad:master May 10, 2024
4 checks passed
@kintel kintel mentioned this pull request May 11, 2024
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants