gui: Drop qt4 support #13458

laanwj · 2018-06-13T15:10:59Z

Implements #8263.

Qt4.x has been EOL since 2015, and at least Gentoo has, or is going to drop support for it. I wouldn't be surprised if other Linux distributions follow.

This removes Qt4 detection from the build system, as well as removes all Qt4 fallbacks from the code. Turns out there's more than I expected: this is going to make maintenance of the GUI code, as well as adding new features significantly easier.

(I know there's still some references left to qt4 in RPM and Debian build script, but I don't have the knowledge how to fix them)

fanquake · 2018-06-13T15:18:07Z

What would this make our minimum supported version of Qt? 5.4.x IIRC?

laanwj · 2018-06-13T15:22:40Z

What would this make our minimum supported version of Qt? 5.4.x IIRC?

That's a good question. I'm not sure that's something to be decided here, I don't change anything with regard to Qt5 versions supported. Though there are a few QT_VERSION checks for Qt 5.2 that would not be relevant in that case..

fanquake · 2018-06-13T15:43:15Z

build-aux/m4/bitcoin_qt.m4

@@ -113,63 +113,48 @@ AC_DEFUN([BITCOIN_QT_CONFIGURE],[
  TEMP_CXXFLAGS=$CXXFLAGS


Qt4 mentions in the comment here ^, and another starting at line 288.

fanquake · 2018-06-13T15:45:31Z

That's a good question. I'm not sure that's something to be decided here, I don't change anything with regard to Qt5 versions supported. Though there are a few QT_VERSION checks for Qt 5.2 that would not be relevant in that case..

Fair enough.

Some qt4 related code here:

bitcoin/src/qt/coincontroldialog.cpp

Line 125 in f532d52

// change coin control first column label due Qt4 bug.

bitcoin/src/qt/test/wallettests.cpp

Line 90 in f532d52

    
           //! Request context menu (call method that is public in qt5, but protected in qt4).

There are some qt4 related docs that could be dropped. i.e:
https://github.com/bitcoin/bitcoin/blame/master/doc/build-osx.md#L27
https://github.com/bitcoin/bitcoin/blob/master/doc/build-unix.md#dependencies-for-the-gui

laanwj · 2018-06-13T16:17:31Z

Thanks @fanquake, updated for those.

Edit: I left the // change coin control first column label due Qt4 bug. though, because I'm not sure what to do there otherwise. Looks like the statement can be moved, but I don't know where.

promag · 2018-06-13T16:19:01Z

Concept ACK, will review.

maflcko · 2018-06-13T16:36:20Z

ACK. A follow up pull request could clarify which versions of qt5 are supported.

maflcko · 2018-06-13T18:30:25Z

Concept ACK ecda9463649d7a5b3d35e31f0c6753f7f4f721c5.

Nits:

The section for fedora still says " To build with Qt 5 (recommended) you need the following:"
The link in src/qt/Readme.md should be updated to "https://doc.qt.io/qt-5/qdialog.html"
The link in src/qt/bitcoingui.cpp should be updated to "https://doc.qt.io/qt-5/gallery.html"

theuni · 2018-06-13T18:33:12Z

Concept ACK.

theuni · 2018-06-13T18:35:00Z

.travis.yml

-# Qt4 & system libs
-    - HOST=x86_64-unknown-linux-gnu PACKAGES="python3-zmq qt4-dev-tools libssl1.0-dev libevent-dev bsdmainutils libboost-system-dev libboost-filesystem-dev libboost-chrono-dev libboost-program-options-dev libboost-test-dev libboost-thread-dev libdb5.3++-dev libminiupnpc-dev libzmq3-dev libprotobuf-dev protobuf-compiler libqrencode-dev xvfb libqt4-dev" NO_DEPENDS=1 NEED_XVFB=1 RUN_TESTS=true GOAL="install" BITCOIN_CONFIG="--enable-zmq --with-incompatible-bdb --enable-glibc-back-compat --enable-reduce-exports --with-gui=qt4 CPPFLAGS=-DDEBUG_LOCKORDER" DISPLAY=:99.0
+# x86_64 Linux (Qt5 & system libs)
+    - HOST=x86_64-unknown-linux-gnu PACKAGES="python3-zmq qtbase5-dev qttools5-dev-tools libssl1.0-dev libevent-dev bsdmainutils libboost-system-dev libboost-filesystem-dev libboost-chrono-dev libboost-program-options-dev libboost-test-dev libboost-thread-dev libdb5.3++-dev libminiupnpc-dev libzmq3-dev libprotobuf-dev protobuf-compiler libqrencode-dev xvfb libqt4-dev" NO_DEPENDS=1 NEED_XVFB=1 RUN_TESTS=true GOAL="install" BITCOIN_CONFIG="--enable-zmq --with-incompatible-bdb --enable-glibc-back-compat --enable-reduce-exports --with-gui=qt5 CPPFLAGS=-DDEBUG_LOCKORDER" DISPLAY=:99.0


Missed libqt4-dev.

Also, I believe this means that we can get rid of xvfb, since qt5's 'minimal' plugin should work.

meshcollider · 2018-06-13T21:02:23Z

Concept ACK

fanquake · 2018-06-14T01:34:20Z

src/qt/guiutil.h

@@ -233,7 +233,7 @@ namespace GUIUtil
        void mouseReleaseEvent(QMouseEvent *event);
    };

-#if defined(Q_OS_MAC) && QT_VERSION >= 0x050000
+#if defined(Q_OS_MAC)


Note: Still need to test, but we might be able to remove this Qt workaround entirely.

jonasschnelli · 2018-06-14T07:10:26Z

Concept ACK
Gitian Build: https://bitcoin.jonasschnelli.ch/build/656

promag · 2018-06-14T08:53:56Z

Remove Qt 4 is also supported from bitcoin/src/qt/README.md.

promag · 2018-06-14T09:00:57Z

All QT_VERSION usages LGTM.

laanwj · 2018-06-14T11:10:05Z

Ok, I think all comments by @promag @MarcoFalke @theuni have been addressed.
@fanquake That'd be good. But maybe in a separate PR, I think this update should be dumbly removing Qt4 support but otherwise keep the logic for Qt5 the same.

maflcko · 2018-06-14T16:40:09Z

.travis.yml

-# Qt4 & system libs
-    - HOST=x86_64-unknown-linux-gnu PACKAGES="python3-zmq qt4-dev-tools libssl1.0-dev libevent-dev bsdmainutils libboost-system-dev libboost-filesystem-dev libboost-chrono-dev libboost-program-options-dev libboost-test-dev libboost-thread-dev libdb5.3++-dev libminiupnpc-dev libzmq3-dev libprotobuf-dev protobuf-compiler libqrencode-dev xvfb libqt4-dev" NO_DEPENDS=1 NEED_XVFB=1 RUN_TESTS=true GOAL="install" BITCOIN_CONFIG="--enable-zmq --with-incompatible-bdb --enable-glibc-back-compat --enable-reduce-exports --with-gui=qt4 CPPFLAGS=-DDEBUG_LOCKORDER" DISPLAY=:99.0
+# x86_64 Linux (Qt5 & system libs)
+    - HOST=x86_64-unknown-linux-gnu PACKAGES="python3-zmq qtbase5-dev qttools5-dev-tools libssl1.0-dev libevent-dev bsdmainutils libboost-system-dev libboost-filesystem-dev libboost-chrono-dev libboost-program-options-dev libboost-test-dev libboost-thread-dev libdb5.3++-dev libminiupnpc-dev libzmq3-dev libprotobuf-dev protobuf-compiler libqrencode-dev" NO_DEPENDS=1 NEED_XVFB=1 RUN_TESTS=true GOAL="install" BITCOIN_CONFIG="--enable-zmq --with-incompatible-bdb --enable-glibc-back-compat --enable-reduce-exports --with-gui=qt5 CPPFLAGS=-DDEBUG_LOCKORDER" DISPLAY=:99.0


I think cory mentioned you could get rid of xvfb?

Didn't I? oh, there's an environment variable too...

$DISPLAY was also related to xvfb and can be removed.

maflcko · 2018-06-14T19:43:14Z

.travis.yml

-# Qt4 & system libs
-    - HOST=x86_64-unknown-linux-gnu PACKAGES="python3-zmq qt4-dev-tools libssl1.0-dev libevent-dev bsdmainutils libboost-system-dev libboost-filesystem-dev libboost-chrono-dev libboost-program-options-dev libboost-test-dev libboost-thread-dev libdb5.3++-dev libminiupnpc-dev libzmq3-dev libprotobuf-dev protobuf-compiler libqrencode-dev xvfb libqt4-dev" NO_DEPENDS=1 NEED_XVFB=1 RUN_TESTS=true GOAL="install" BITCOIN_CONFIG="--enable-zmq --with-incompatible-bdb --enable-glibc-back-compat --enable-reduce-exports --with-gui=qt4 CPPFLAGS=-DDEBUG_LOCKORDER" DISPLAY=:99.0
+# x86_64 Linux (Qt5 & system libs)
+    - HOST=x86_64-unknown-linux-gnu PACKAGES="python3-zmq qtbase5-dev qttools5-dev-tools libssl1.0-dev libevent-dev bsdmainutils libboost-system-dev libboost-filesystem-dev libboost-chrono-dev libboost-program-options-dev libboost-test-dev libboost-thread-dev libdb5.3++-dev libminiupnpc-dev libzmq3-dev libprotobuf-dev protobuf-compiler libqrencode-dev" NO_DEPENDS=1 RUN_TESTS=true GOAL="install" BITCOIN_CONFIG="--enable-zmq --with-incompatible-bdb --enable-glibc-back-compat --enable-reduce-exports --with-gui=qt5 CPPFLAGS=-DDEBUG_LOCKORDER" DISPLAY=:99.0


Can probably also remove two instances of DISPLAY?

DrahtBot · 2018-06-14T20:12:23Z

Note to reviewers: This pull request conflicts with the following ones:

Remove boost::program_options dependency #13482 (Remove boost::program_options dependency by ken2812221)
[qt] Fixed tx-view min amount typing period on locales using comma #13278 ([qt] Fixed tx-view min amount typing period on locales using comma by GreatSock)
Make objects in range declarations immutable by default. Avoid unnecessary copying of objects in range declarations. #13249 (Make objects in range declarations immutable by default. Avoid unnecessary copying of objects in range declarations. by practicalswift)
Break circular dependency: init -> * -> init by extracting shutdown.h #13235 (Break circular dependency: init -> * -> init by extracting shutdown.h by Empact)
depends: Upgrade Qt to 5.9.6 #12971 (depends: Upgrade Qt to 5.9.5 by TheCharlatan)
[ci] Run functional tests using bitcoin-qt in one Travis job #12873 ([ci] Run functional tests using bitcoin-qt in one Travis job by jamesob)
Add -ftrapv to CFLAGS and CXXFLAGS when --enable-debug is used. Enable -ftrapv in Travis. #12686 (Add -ftrapv to CFLAGS and CXXFLAGS when --enable-debug is used. Enable -ftrapv in Travis. by practicalswift)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

practicalswift · 2018-06-14T20:36:16Z

Concept ACK

maflcko · 2018-06-15T14:27:48Z

@laanwj You wouldn't need to export DISPLAY to the docker as well? Could remove that as well?

https://github.com/laanwj/bitcoin/blob/e2dc0e7f02b63ab5990b6f3c806ceaedf40cf3ea/.travis.yml#L48

fanquake · 2018-06-15T15:43:02Z

Should be able to drop this check from paymentrequestplus.cpp:

#if QT_VERSION >= 0x050000
        if (qCert.isBlacklisted()) {
            ...snip...
            return false;
        }
#endif

laanwj · 2018-06-15T19:55:45Z

@MarcoFalke @fanquake done
this is becoming a palimpsest of commits, if there's no more things I missed I'm going to squash them into more sensible subdivision

TheBlueMatt · 2018-06-15T20:09:17Z

I presume, barring some major rewrite, this implies the PPA should simply not ship with a GUI except on 18.04? (And replace bitcoin-qt with a dummy package instead of letting it get stale?)

…

On June 15, 2018 7:56:01 PM UTC, "Wladimir J. van der Laan" ***@***.***> wrote: @MarcoFalke @fanquake done this is becoming a palimpsest of commits, if there's no more things I missed I'm going to squash them into more sensible subdivision -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: #13458 (comment)

maflcko · 2018-06-15T20:17:01Z

@TheBlueMatt I doubt anyone running ubuntu 14.04 is running the gui from the ppa. If there is one, they might as well download the release build.

Edit: Ah, according to #8043, using the release builds wouldn't help and xenial is affected as well. Though, since the only problem is the tray icon, we might as well just ship a package with a slight ui glitch instead of no package at all.

fanquake · 2018-06-16T10:19:05Z

laanwj · 2018-06-16T11:11:02Z

@TheBlueMatt I doubt anyone running ubuntu 14.04 is running the gui from the ppa. If there is one, they might as well download the release build.

Or a three-line patch to disable tray icon support in the PPA, for those versions. It seems ridiculous to me to remove the entire GUI package just because of a small glitch.
(or even just ship with the glitch, as @MarcoFalke says - note it as known issue, people can upgrade to Ubuntu 18.04 if it bothers them)

wumpus · 2018-06-16T15:13:44Z

Wrong @wumpus.

There were surprisingly many `#ifdef` fallbacks for Qt 4. Remiving them simplifies maintenance, as well as adding new GUI functionality.

Change Qt4 & system libs build to Qt5 & system libs build.

laanwj · 2018-06-18T10:33:28Z

Squashed into sensible commits:
56c25b36b17b73b4ad36d608c428aaa052130fab → af6ac3b
No other changes.

maflcko · 2018-06-18T11:17:35Z

Concept re-ACK af6ac3b. (No changes since feedback addressed, but I didn't review the build changes)

af6ac3b doc: Remove mention of Qt4 from build docs (Wladimir J. van der Laan) 462c71f test: Update travis to not test Qt4 anymore (Wladimir J. van der Laan) 907f73b gui: Remove QT_VERSION fallbacks for Qt < 5 (Wladimir J. van der Laan) bad068a build: Build system changes to support only Qt5 (Wladimir J. van der Laan) Pull request description: Implements #8263. Qt4.x has been EOL since 2015, and at least Gentoo has, or is going to drop support for it. I wouldn't be surprised if other Linux distributions follow. This removes Qt4 detection from the build system, as well as removes all Qt4 fallbacks from the code. Turns out there's more than I expected: this is going to make maintenance of the GUI code, as well as adding new features significantly easier. (I know there's still some references left to qt4 in RPM and Debian build script, but I don't have the knowledge how to fix them) Tree-SHA512: d495924fd4dda6f6566ba44ee96be7cbe62e69ba1ca993b80a8449f78da852b7f1bd3e8200d57cfa1d72233c340eeff4596fb0032ecbddc715d99aea63817d3f

af6ac3b doc: Remove mention of Qt4 from build docs (Wladimir J. van der Laan) 462c71f test: Update travis to not test Qt4 anymore (Wladimir J. van der Laan) 907f73b gui: Remove QT_VERSION fallbacks for Qt < 5 (Wladimir J. van der Laan) bad068a build: Build system changes to support only Qt5 (Wladimir J. van der Laan) Pull request description: Implements bitcoin#8263. Qt4.x has been EOL since 2015, and at least Gentoo has, or is going to drop support for it. I wouldn't be surprised if other Linux distributions follow. This removes Qt4 detection from the build system, as well as removes all Qt4 fallbacks from the code. Turns out there's more than I expected: this is going to make maintenance of the GUI code, as well as adding new features significantly easier. (I know there's still some references left to qt4 in RPM and Debian build script, but I don't have the knowledge how to fix them) Tree-SHA512: d495924fd4dda6f6566ba44ee96be7cbe62e69ba1ca993b80a8449f78da852b7f1bd3e8200d57cfa1d72233c340eeff4596fb0032ecbddc715d99aea63817d3f (cherry picked from commit dc53f7f) # Conflicts: # .travis.yml # build-aux/m4/bitcoin_qt.m4 # configure.ac # doc/build-osx.md # doc/build-unix.md # doc/dependencies.md # src/qt/README.md # src/qt/bitcoingui.cpp # src/qt/coincontroldialog.cpp # src/qt/dash.cpp # src/qt/guiutil.cpp # src/qt/macdockiconhandler.mm # src/qt/networkstyle.cpp # src/qt/openuridialog.cpp # src/qt/peertablemodel.cpp # src/qt/signverifymessagedialog.cpp # src/qt/test/wallettests.cpp # src/qt/transactionview.cpp

af6ac3b doc: Remove mention of Qt4 from build docs (Wladimir J. van der Laan) 462c71f test: Update travis to not test Qt4 anymore (Wladimir J. van der Laan) 907f73b gui: Remove QT_VERSION fallbacks for Qt < 5 (Wladimir J. van der Laan) bad068a build: Build system changes to support only Qt5 (Wladimir J. van der Laan) Pull request description: Implements bitcoin#8263. Qt4.x has been EOL since 2015, and at least Gentoo has, or is going to drop support for it. I wouldn't be surprised if other Linux distributions follow. This removes Qt4 detection from the build system, as well as removes all Qt4 fallbacks from the code. Turns out there's more than I expected: this is going to make maintenance of the GUI code, as well as adding new features significantly easier. (I know there's still some references left to qt4 in RPM and Debian build script, but I don't have the knowledge how to fix them) Tree-SHA512: d495924fd4dda6f6566ba44ee96be7cbe62e69ba1ca993b80a8449f78da852b7f1bd3e8200d57cfa1d72233c340eeff4596fb0032ecbddc715d99aea63817d3f (cherry picked from commit dc53f7f)

laanwj added the GUI label Jun 13, 2018

laanwj added this to the 0.17.0 milestone Jun 13, 2018

laanwj requested a review from theuni June 13, 2018 15:11

laanwj changed the title ~~gui: Drop qt5 support~~ gui: Drop qt4 support Jun 13, 2018

fanquake reviewed Jun 13, 2018

View reviewed changes

build: Build system changes to support only Qt5

bad068a

laanwj force-pushed the 2018_06_remove_qt4_support branch from f00d4d1 to bcf01c9 Compare June 13, 2018 16:16

maflcko added the Build system label Jun 13, 2018

theuni reviewed Jun 13, 2018

View reviewed changes

fanquake reviewed Jun 14, 2018

View reviewed changes

maflcko reviewed Jun 14, 2018

View reviewed changes

fanquake mentioned this pull request Jun 15, 2018

[RFC] gui: Minimum required Qt5 #13478

Closed

fanquake mentioned this pull request Jun 18, 2018

gui: Add leading 0 to QT_VERSION check for QFontDatabase #13493

Closed

laanwj added 3 commits June 18, 2018 12:22

gui: Remove QT_VERSION fallbacks for Qt < 5

907f73b

There were surprisingly many `#ifdef` fallbacks for Qt 4. Remiving them simplifies maintenance, as well as adding new GUI functionality.

test: Update travis to not test Qt4 anymore

462c71f

Change Qt4 & system libs build to Qt5 & system libs build.

doc: Remove mention of Qt4 from build docs

af6ac3b

laanwj force-pushed the 2018_06_remove_qt4_support branch from 56c25b3 to af6ac3b Compare June 18, 2018 10:25

DrahtBot mentioned this pull request Jun 19, 2018

Remove boost::program_options dependency #13482

Merged

sickpig mentioned this pull request Jun 21, 2018

[PORT] Update build_qt.m4 BitcoinUnlimited/BitcoinUnlimited#1162

Merged

laanwj merged commit af6ac3b into bitcoin:master Jun 24, 2018

Empact mentioned this pull request Jun 25, 2018

Break circular dependency: init -> * -> init by extracting shutdown.h #13235

Merged

Bushstar mentioned this pull request Jun 25, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#349

Merged

Fuzzbawls mentioned this pull request Aug 19, 2018

[Qt] Remove Qt4 build support & code fallbacks PIVX-Project/PIVX#698

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

		@@ -113,63 +113,48 @@ AC_DEFUN([BITCOIN_QT_CONFIGURE],[
		TEMP_CXXFLAGS=$CXXFLAGS

gui: Drop qt4 support #13458

gui: Drop qt4 support #13458

Uh oh!

Conversation

laanwj commented Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Jun 13, 2018

Uh oh!

laanwj commented Jun 13, 2018

Uh oh!

fanquake Jun 13, 2018

Choose a reason for hiding this comment

Uh oh!

fanquake commented Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Jun 13, 2018

Uh oh!

maflcko commented Jun 13, 2018

Uh oh!

maflcko commented Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theuni commented Jun 13, 2018

Uh oh!

theuni Jun 13, 2018

Choose a reason for hiding this comment

Uh oh!

meshcollider commented Jun 13, 2018

Uh oh!

fanquake Jun 14, 2018

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Jun 14, 2018

Uh oh!

promag commented Jun 14, 2018

Uh oh!

promag commented Jun 14, 2018

Uh oh!

laanwj commented Jun 14, 2018

Uh oh!

maflcko Jun 14, 2018

Choose a reason for hiding this comment

Uh oh!

laanwj Jun 14, 2018

Choose a reason for hiding this comment

Uh oh!

theuni Jun 14, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 14, 2018

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Jun 14, 2018

Uh oh!

maflcko commented Jun 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Jun 15, 2018

Uh oh!

laanwj commented Jun 15, 2018

Uh oh!

TheBlueMatt commented Jun 15, 2018 via email

Uh oh!

maflcko commented Jun 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Jun 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Jun 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Jun 13, 2018 •

edited

Loading

fanquake commented Jun 13, 2018 •

edited

Loading

laanwj commented Jun 13, 2018 •

edited

Loading

maflcko commented Jun 13, 2018 •

edited

Loading

DrahtBot commented Jun 14, 2018 •

edited

Loading

maflcko commented Jun 15, 2018 •

edited

Loading

maflcko commented Jun 15, 2018 •

edited

Loading

fanquake commented Jun 16, 2018 •

edited

Loading

laanwj commented Jun 16, 2018 •

edited

Loading