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

Qt6 support #1019

Open
wants to merge 4 commits into
base: RB-2.7
Choose a base branch
from
Open

Qt6 support #1019

wants to merge 4 commits into from

Conversation

YakoYakoYokuYoku
Copy link
Member

Thanks for submitting a pull request! Please provide enough information so that others can review your pull request. Additionally, make sure you've done all of these things:

PR Description

What type of PR is this? (Check one of the boxes below)

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (non-breaking change which does not add functionality nor fixes a bug but improves Natron in some way)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • My change requires a change to the documentation
    • I have updated the documentation accordingly

What does this pull request do?

This adds support for building Natron with Qt6. Besides from compilation fixes this does a pair of things, removes Qt modules in includes and rewrites the Shiboken cleanup script in Python. This can contain breaking changes due to the update so report issues if you encounter them.

Show a few screenshots (if this is a visual change)

N/A.

Have you tested your changes (if applicable)? If so, how?

By building and running Natron.

Futher details of this pull request

It's been a while since I've commited here, I hope I'm not that rusty.

@YakoYakoYokuYoku YakoYakoYokuYoku added this to the 2.6 milestone Dec 16, 2024
@YakoYakoYokuYoku YakoYakoYokuYoku self-assigned this Dec 16, 2024
@devernay
Copy link
Member

Hi, can you please resubmit this PR on RB-2.7? RB-2.6 is Qt5 (mainly maintained by @acolwell ), which still wasn't released, and I wouldn't want to break anything in it.
@acolwell will you be available to review this?

@YakoYakoYokuYoku
Copy link
Member Author

Hi, can you please resubmit this PR on RB-2.7?

As you wish. I'm gonna push a new branch for it.

@YakoYakoYokuYoku YakoYakoYokuYoku changed the base branch from RB-2.6 to RB-2.7 December 16, 2024 19:54
@acolwell
Copy link
Collaborator

Hi, can you please resubmit this PR on RB-2.7? RB-2.6 is Qt5 (mainly maintained by @acolwell ), which still wasn't released, and I wouldn't want to break anything in it. @acolwell will you be available to review this?

I don't have a lot of time to devote to Natron right now. My preference would be to do as many changes that apply to Qt5 and Qt6 on RB-2.6 and only do the breaking Qt6 changes on RB-2.7. For example, I'm pretty sure the include changes, the KeySequence changes, RegularExpression changes, Mutex changes, and a few others are Qt5 compatible. This would at least keep the 2 branches relatively similar and allow us to verify behavior with Qt5 before making the Qt6 leap. I'm pretty sure some changes could also be structured in such a way that would minimize the Qt6 #ifdefs to a small number of locations by using typedefs (e.g. typedef QEvent/QEnterEvent to NatronQEnterEvent and QMutex/QRecursiveMutex to NatronQRecursiveMutex temporarily until transition to Qt6 is complete).

Based on my attempt at moving to Qt6 a while ago, a lot of the risk of breaking stuff came from the switch to shiboken6. As far as I can tell there doesn't appear to be many tests that verify the Natron python API so I couldn't convince myself that the significant changes I had to make to get shiboken6 working were correct and didn't break the API. It would be nice if we could have some testing in place to verify that all the exposed APIs are visible in python and work as expected in Qt5 so that we can easily verify nothing breaks in the move to Qt6. It would also be nice to add tests as part of the QRegExp to QRegularExpression migration to make sure that behavior isn't changed. Both were on my todo list, but I just haven't had time to do it. I think both of these are necessary to confidently transition to Qt6.

I'm also a huge fan of doing these in smaller, focused, individual PRs so that it is easy to review and verify each step doesn't break anything. Grouping these into a single PR containing multiple different changes across A LOT of files is just too hard to review and ensure correctness. If others feel differently, that is fine and they should feel free to step forward to do the reviews.

@YakoYakoYokuYoku
Copy link
Member Author

@acolwell, the Windows builds are failing due to the include dirs variables not being defined there, please check the CMake config files of MSYS2 (at <prefix>/lib/cmake) as they might not define the variables.

@acolwell
Copy link
Collaborator

@acolwell, the Windows builds are failing due to the include dirs variables not being defined there, please check the CMake config files of MSYS2 (at <prefix>/lib/cmake) as they might not define the variables.

The installer builds don't use cmake. They use qmake so any changes made to the cmake files need to have equivalent changes made the qmake .pro files. It's a little annoying I know, but it is legacy that I haven't had time to fix. I'm not able to look at this right now, but I'll try to take a look sometime today.

@YakoYakoYokuYoku
Copy link
Member Author

I thought that the installer builds moved to CMake, I wasn't in the loop. Albeit, as support for QMake dwindles each and every release, perhaps it should be the time to do the move.

@acolwell
Copy link
Collaborator

I thought that the installer builds moved to CMake, I wasn't in the loop. Albeit, as support for QMake dwindles each and every release, perhaps it should be the time to do the move.

For as long as I have been involved, the Windows installer script has used QMake. I'd love to move away from it since I already do all my Natron dev work on Windows w/ CMake and then just do a final verify with an Qmake installer build. I just haven't had the time to do the necessary work to transition the installer build to cmake and verify it generates an equivalent binary.

@YakoYakoYokuYoku YakoYakoYokuYoku force-pushed the gui-sbk6 branch 3 times, most recently from f7293e3 to 453d0c7 Compare December 21, 2024 19:46
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.

3 participants