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

gui: Drop qt4 support #13458

Merged
merged 4 commits into from Jun 24, 2018

Conversation

Projects
None yet
@laanwj
Copy link
Member

laanwj commented Jun 13, 2018

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)

@laanwj laanwj added the GUI label Jun 13, 2018

@laanwj laanwj added this to the 0.17.0 milestone Jun 13, 2018

@laanwj laanwj requested a review from theuni Jun 13, 2018

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

@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Jun 13, 2018

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

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Jun 13, 2018

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..

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

This comment has been minimized.

@fanquake

fanquake Jun 13, 2018

Member

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

@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Jun 13, 2018

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:

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

//! 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 laanwj force-pushed the laanwj:2018_06_remove_qt4_support branch Jun 13, 2018

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Jun 13, 2018

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

This comment has been minimized.

Copy link
Member

promag commented Jun 13, 2018

Concept ACK, will review.

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Jun 13, 2018

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Jun 13, 2018

Concept ACK ecda9463649d7a5b3d35e31f0c6753f7f4f721c5.

Nits:

@theuni

This comment has been minimized.

Copy link
Member

theuni commented Jun 13, 2018

Concept ACK.

.travis.yml Outdated
# 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

This comment has been minimized.

@theuni

theuni Jun 13, 2018

Member

Missed libqt4-dev.

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

@MeshCollider

This comment has been minimized.

Copy link
Member

MeshCollider commented Jun 13, 2018

Concept ACK

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

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

This comment has been minimized.

@fanquake

fanquake Jun 14, 2018

Member

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

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Jun 14, 2018

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

@promag

This comment has been minimized.

Copy link
Member

promag commented Jun 14, 2018

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

@promag

This comment has been minimized.

Copy link
Member

promag commented Jun 14, 2018

All QT_VERSION usages LGTM.

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Jun 14, 2018

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.

.travis.yml Outdated
# 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

This comment has been minimized.

@MarcoFalke

MarcoFalke Jun 14, 2018

Member

I think cory mentioned you could get rid of xvfb?

This comment has been minimized.

@laanwj

laanwj Jun 14, 2018

Author Member

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

This comment has been minimized.

@theuni

theuni Jun 14, 2018

Member

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

.travis.yml Outdated
# 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

This comment has been minimized.

@MarcoFalke

MarcoFalke Jun 14, 2018

Member

Can probably also remove two instances of DISPLAY?

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Jun 14, 2018

Note to reviewers: This pull request conflicts with the following ones:
  • #13482 (Remove boost::program_options dependency by ken2812221)
  • #13278 ([qt] Fixed tx-view min amount typing period on locales using comma by GreatSock)
  • #13249 (Make objects in range declarations immutable by default. Avoid unnecessary copying of objects in range declarations. by practicalswift)
  • #13235 (Break circular dependency: init -> * -> init by extracting shutdown.h by Empact)
  • #12971 (depends: Upgrade Qt to 5.9.5 by TheCharlatan)
  • #12873 ([ci] Run functional tests using bitcoin-qt in one Travis job by jamesob)
  • #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

This comment has been minimized.

Copy link
Member

practicalswift commented Jun 14, 2018

Concept ACK

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Jun 15, 2018

@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

This comment has been minimized.

Copy link
Member

fanquake commented Jun 15, 2018

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

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

This comment has been minimized.

Copy link
Member Author

laanwj commented Jun 15, 2018

@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

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Jun 15, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Jun 15, 2018

@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

This comment has been minimized.

Copy link
Member

fanquake commented Jun 16, 2018

~~~@wumpus~~~ @laanwj That'd be great.

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Jun 16, 2018

@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

This comment has been minimized.

Copy link

wumpus commented Jun 16, 2018

Wrong @wumpus.

laanwj added some commits Jun 13, 2018

gui: Remove QT_VERSION fallbacks for Qt < 5
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
Change Qt4 & system libs build to Qt5 & system libs build.

@laanwj laanwj force-pushed the laanwj:2018_06_remove_qt4_support branch to af6ac3b Jun 18, 2018

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Jun 18, 2018

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Jun 18, 2018

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

@sickpig sickpig referenced this pull request Jun 21, 2018

Merged

[PORT] Update build_qt.m4 #1162

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jun 24, 2018

Merge #13458: gui: Drop qt4 support
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.