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

build: Bump minimum Qt version to 5.5.1 #15393

Merged
merged 1 commit into from Feb 14, 2019

Conversation

@Sjors
Copy link
Member

commented Feb 12, 2019

Fixes #13478

Compiled and lightly tested on 10.14.3 against QT 5.12.0.

@MarcoFalke MarcoFalke changed the title Bump minimum Qt version to 5.5.1 build: Bump minimum Qt version to 5.5.1 Feb 12, 2019

@MarcoFalke MarcoFalke added this to the 0.18.0 milestone Feb 12, 2019

@luke-jr

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

Is there an actual benefit to this?

Has anyone done a check for what versions come with current stable distros?

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

Examples:

Also,

  • 5.5.1 is long EOL anyway and it'd be better if the user built qt from our depends
  • Bitcoin Core wouldn't compile with qt5.2 anyway
@fanquake

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

Concept ACK

Show resolved Hide resolved build-aux/m4/bitcoin_qt.m4 Outdated
Show resolved Hide resolved doc/dependencies.md Outdated
@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

Concept ACK

@Sjors Sjors force-pushed the Sjors:2019/02/qt-5_5 branch from 45855af to a4d80ff Feb 13, 2019

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2019

@luke-jr wrote:

Is there an actual benefit to this?

See discussion in #13478

I added a link to that discussion in doc/dependencies.md and dropped the Q_IMPORT_PLUGIN(AccessibleFactory) line.

@@ -27,9 +27,7 @@
#include <QTest>
#include <QTextEdit>
#include <QtGlobal>
#if QT_VERSION >= 0x050000
#include <QtTest/QtTestWidgets>

This comment has been minimized.

Copy link
@practicalswift

practicalswift Feb 13, 2019

Member

Could sort includes :-)

@@ -276,7 +258,7 @@ AC_DEFUN([_BITCOIN_QT_CHECK_QT5],[
#endif
]],
[[
#if QT_VERSION < 0x050200 || QT_VERSION_MAJOR < 5
#if QT_VERSION < 0x050501 || QT_VERSION_MAJOR < 5

This comment has been minimized.

Copy link
@flack

flack Feb 13, 2019

Contributor

is the QT_VERSION_MAJOR clause still needed? At first glance, it looks like a leftover from when Qt 4 was still supported

This comment has been minimized.

Copy link
@Sjors

Sjors Feb 13, 2019

Author Member

I was assuming this is a generic check to see if QT >=5.5.1 is present at all, but haven't tried what happens with older QT versions.

This comment has been minimized.

Copy link
@flack

flack Feb 13, 2019

Contributor

according to https://doc.qt.io/qt-5/qtglobal.html#QT_VERSION if QT_MAJOR_VERSION is 4, then QT_VERSION should be something like 0x040102, so it would already fail the first clause. IOW, I don't think there is any constellation where the first clause is false and the second one is true

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Feb 13, 2019

Member

Could ask @TheCharlatan if there was a reason to do this in 28482ef

This comment has been minimized.

Copy link
@Sjors

Sjors Feb 13, 2019

Author Member

@flack I thought you were referring to the entire line. Yes, I agree the || QT_VERSION_MAJOR < 5 bit seems redundant. That maybe have been in order to support both 4.x and 5.x from certain minor versions onward.

This comment has been minimized.

Copy link
@TheCharlatan

TheCharlatan Feb 14, 2019

Contributor

I was confused about what the actual supported minor version was at the time. Either way, it is definitely redundant with this pr.

@fanquake

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

@Sjors Could you add release notes to this PR as well?

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

Gitian builds for commit 0d1160e (master):

Gitian builds for commit 1dae32e (master and this pull):

@Sjors Sjors force-pushed the Sjors:2019/02/qt-5_5 branch from a4d80ff to 39766ec Feb 14, 2019

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

Added release note. The gitian binaries should still be valid.

@Sjors Sjors force-pushed the Sjors:2019/02/qt-5_5 branch from 39766ec to fd46c4c Feb 14, 2019

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

I removed the redundant || QT_VERSION_MAJOR < 5.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

There was a discussion on IRC that debian 8 comes with 5.3 (https://packages.debian.org/jessie/qt5-default)

The conclusion was that anyone on debian 8 should use our ./depends qt package: http://www.erisian.com.au/bitcoin-core-dev/log-2019-02-14.html#l-405

@hebasto

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

utACK fd46c4c

1 similar comment
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

utACK fd46c4c

@jonasschnelli jonasschnelli merged commit fd46c4c into bitcoin:master Feb 14, 2019

1 of 2 checks passed

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

jonasschnelli added a commit that referenced this pull request Feb 14, 2019

Merge #15393: build: Bump minimum Qt version to 5.5.1
fd46c4c Bump minimum Qt version to 5.5.1 (Sjors Provoost)

Pull request description:

  Fixes #13478

  Compiled and lightly tested on 10.14.3 against QT 5.12.0.

Tree-SHA512: 6890331969bbf4c66dc0993b8817b1f0831d008f5863554e9c09a38f4700260b84044ff961664c377decc9fb8300e3543c267f935ec64fbc97b20f8fb396247a

@Sjors Sjors deleted the Sjors:2019/02/qt-5_5 branch Feb 14, 2019

@lucayepa

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

Ok, I'm late to the party, but we are going to have a release that will not build both on Debian Jessie (QT 5.3.2) and Ubuntu Trusty (QT 5.2.1).

I agree with @jonasschnelli :

"but we need to expect users run it close to the LTS end".

The LTS end date of Ubuntu Trusty is April 2019. The LTS end date of Debian Jessie is June 30, 2020.

IMO we should have a policy not to break compatibility with LTS distributions, unless this is really something we need (i.e. bugs or big issues).

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2019

@lucayepa it's better to bring this up in #13478.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

@lucayepa It hasn't been building on those distros before this pull request because no one cared (found it worthwhile) to fix the code

Trusty will be EOL before the final release and if you are on oldstable debian, you can build from depends or use the deterministic binaries.

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.