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

Qt: Make Qt 5.9 a hard requirement #7149

Merged
merged 3 commits into from Jul 10, 2018

Conversation

7 participants
@spycrab
Contributor

spycrab commented Jun 21, 2018

This has multiple benefits:

  • Allows us to remove the ugly ActionHelper.h
  • Allows us to remove the version check macro in Main
  • We won't have to worry about moc not being able to parse C++17 nested namespaces.
@spycrab

This comment has been minimized.

Contributor

spycrab commented Jun 21, 2018

The pr-ubu-x64 buildbot should be updated before this can be merged (pinging @delroth)

@spycrab spycrab referenced this pull request Jun 21, 2018

Closed

Qt: Remove ActionHelper #7091

@spycrab spycrab added the WIP label Jul 6, 2018

@spycrab

This comment has been minimized.

Contributor

spycrab commented Jul 6, 2018

Kind of messed this one up by accident, will recreate it soon.

@spycrab spycrab removed the WIP label Jul 9, 2018

@spycrab spycrab merged commit 71ff634 into dolphin-emu:master Jul 10, 2018

10 checks passed

default Very basic checks passed, handed off to Buildbot.
Details
lint Build succeeded on builder lint
Details
pr-android Build succeeded on builder pr-android
Details
pr-deb-dbg-x64 Build succeeded on builder pr-deb-dbg-x64
Details
pr-deb-x64 Build succeeded on builder pr-deb-x64
Details
pr-freebsd-x64 Build succeeded on builder pr-freebsd-x64
Details
pr-osx-x64 Build succeeded on builder pr-osx-x64
Details
pr-ubu-x64 Build succeeded on builder pr-ubu-x64
Details
pr-win-dbg-x64 Build succeeded on builder pr-win-dbg-x64
Details
pr-win-x64 Build succeeded on builder pr-win-x64
Details

@spycrab spycrab deleted the spycrab:qt_5.9 branch Jul 10, 2018

@StefanBruens

This comment has been minimized.

StefanBruens commented Jul 12, 2018

Allows us to remove the ugly ActionHelper.h

The required addAction is available since 5.6.0

Allows us to remove the version check macro in Main

Obviously, as the check is ">= 5.6.0", also only requires 5.6.0

We won't have to worry about moc not being able to parse C++17 nested namespaces.

Support was added in 5.8.0, https://codereview.qt-project.org/#/c/170035/
Is this already used/required in dolphin?

Otherwise I would suggest to lower the requirements to 5.6, or 5.8 at most.Qt 5.6 is an LTS with support until March 2019.

@Zexaron

This comment has been minimized.

Contributor

Zexaron commented Jul 12, 2018

@StefanBruens IMO Except that March 2019 from this date just doesn't look like "long term" to me. It's a magic circle, holding back Qt improvements fuels more reasons to keep Wx because those reasons would be using the cons of Qt as their argument for Wx. I agree with sypcrab to push with it while Qt momentum is still high so that it gets done good and quicker and doesn't drag the transition too long. The length of the transition is also a UX factor and it can only by it self create dilemmas in user preference and opinion as they have more time thinking and digging deeper in various comparisons.

@spycrab

This comment has been minimized.

Contributor

spycrab commented Jul 12, 2018

5.9 is the latest LTS release and was released over a year ago. Since 5.8 isn't even supported anymore, it's clearly the way to go IMO.

@StefanBruens

This comment has been minimized.

StefanBruens commented Jul 12, 2018

@Zexaron but dolphin does not use a single feature introduced after 5.6.0

Nested namespace specifiers are a C++17 feature which you may use one day, but currently it is not used, so there is no need for a newer moc.

Qt 5.9 or newer is only available in the distribution releases from the last 3 months, e.g. Ubuntu 18.04 (2018-04-26), openSUSE Leap 15.0 (2018-05-18). There is no stable Debian with anything later than 5.7.x. I have tested building it for openSUSE Leap 42.3, which only provides Qt 5.6.4.

The only change to make it compatible with 5.6 is lowering the version in the CMakeLists.txt

@lioncash

This comment has been minimized.

Member

lioncash commented Jul 12, 2018

@StefanBruens We actually already use nested namespace specifiers in the IOS and DSP code (and likely other places in the future). Of the list provided, we don't directly support openSUSE, we only have integration buildbots for Android, Debian (unstable, since stable likes to take forever to update and move with the rest of the world), Ubuntu, and FreeBSD as far as unix/unix-like support goes.

KAMiKAZOW added a commit to KAMiKAZOW/dolphin that referenced this pull request Jul 13, 2018

Set Qt version to 5.6
In dolphin-emu#7149 several changes were merged that were claimed to be only compatible with Qt 5.9. Actually it works fine with 5.6 as well.

@KAMiKAZOW KAMiKAZOW referenced this pull request Jul 13, 2018

Closed

Set Qt version to 5.6 #7259

@KAMiKAZOW

This comment has been minimized.

Contributor

KAMiKAZOW commented Jul 13, 2018

Artificially limiting platform support makes no sense, though. Qt 5.6 still works. Pls accept #7259

@lioncash

This comment has been minimized.

Member

lioncash commented Jul 13, 2018

Does it work with nested namespaces though? That support was added in 5.8, and the only reason it currently works is the avoiding of including those headers in other headers that get run through moc.

@StefanBruens

This comment has been minimized.

StefanBruens commented Jul 13, 2018

I fear you are confusing nested namespaces with nested namespace identifiers. E.g. DSP::Analyzser is a nested namespace, but the code does not use nested namespace identifiers, but the plain old C++98 syntax:
namespace DSP { namespace Analyzer { /* foo */ }}
The same using C++17 nested-namespace-identifiers would be
namespace DSP::Analyzer { /* foo */ }

@JosJuice

This comment has been minimized.

Contributor

JosJuice commented Jul 13, 2018

While nested namespace identifiers may be the more correct way to refer to it, lioncash's point is correct – we do use nested namespace identifiers. See this commit, for instance: fb124c2

@lioncash

This comment has been minimized.

Member

lioncash commented Jul 13, 2018

@StefanBruens Yes, I don't need to be "'splained" about what nested-namespaces are. "nested namespaces" and "nested namespace identifiers" are both commonly used to refer to the same thing. Thanks in advance.

Some parts of the DSP code do use nested namespaces, while other parts do not yet (example). Please don't cherry-pick otherwise, or just not thoroughly look. I can assure you I know where nested namespaces are used, since I've worked on that code. Again, thanks in advance.

@delroth

This comment has been minimized.

Member

delroth commented Jul 13, 2018

Let's not escalate this further please.

@KAMiKAZOW @StefanBruens we are not going to limit ourselves to the 5.6 feature set. We are targetting 5.9, and while we are not currently relying on anything that's not in 5.6 this is 1. by pure chance; 2. going to change.

Our benchmark for Linux compatibility re: external dependencies is "latest Ubuntu LTS". We are not going to subject ourselves to the slow update schedule of every single distro that exists.

If you distribution is not packaging Qt 5.9 yet, then I would recommend to not use latest Dolphin and keep to pre-5.9 required versions. Or maintain a patch as part of your package that decreases the Qt requirement until it breaks. Then you're on your own.

@dolphin-emu dolphin-emu locked as too heated and limited conversation to collaborators Jul 13, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.