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: If both Qt4 and Qt5 are installed, use Qt5 #6938

Merged
merged 1 commit into from Nov 4, 2015

Conversation

Projects
None yet
6 participants
@laanwj
Member

laanwj commented Nov 4, 2015

If both Qt4 and Qt5 development headers are installed, use Qt5. Building against Qt5 should be encouraged as that is where active development happens.

@laanwj laanwj added the Build system label Nov 4, 2015

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Nov 4, 2015

Member

Concept ACK

Member

MarcoFalke commented Nov 4, 2015

Concept ACK

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Nov 4, 2015

Contributor

Please also remove --with-gui from ./configure --with-gui=qt5 in doc/build-osx.md.

Contributor

paveljanik commented Nov 4, 2015

Please also remove --with-gui from ./configure --with-gui=qt5 in doc/build-osx.md.

@fanquake

This comment has been minimized.

Show comment
Hide comment
@fanquake

fanquake Nov 4, 2015

Member

Concept ACK

On Wednesday, November 4, 2015, paveljanik notifications@github.com wrote:

Please also remove --with-gui from ./configure --with-gui=qt5 in doc/
build-osx.md.


Reply to this email directly or view it on GitHub
#6938 (comment).

Member

fanquake commented Nov 4, 2015

Concept ACK

On Wednesday, November 4, 2015, paveljanik notifications@github.com wrote:

Please also remove --with-gui from ./configure --with-gui=qt5 in doc/
build-osx.md.


Reply to this email directly or view it on GitHub
#6938 (comment).

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 4, 2015

Member

Please also remove --with-gui from ./configure --with-gui=qt5 in doc/build-osx.md.

But that means you may end up without a GUI at all, if the proper dependencies cannot be found. Is that the intended behavior? If not, actively setting a GUI makes sense

Member

laanwj commented Nov 4, 2015

Please also remove --with-gui from ./configure --with-gui=qt5 in doc/build-osx.md.

But that means you may end up without a GUI at all, if the proper dependencies cannot be found. Is that the intended behavior? If not, actively setting a GUI makes sense

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 4, 2015

Member

utACK!

Please also remove --with-gui from ./configure --with-gui=qt5 in doc/build-osx.md.

We should definitively keep this.

Member

jonasschnelli commented Nov 4, 2015

utACK!

Please also remove --with-gui from ./configure --with-gui=qt5 in doc/build-osx.md.

We should definitively keep this.

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Nov 4, 2015

Contributor

@laanwj IMO configure without arguments should build whatever it can build (based on dependencies, and warn user that it can't build GUI because of missing deps) and you should be able to disable GUI with --disable-gui if you want to save compile time, do not care about GUI etc. Selecting which UI has sense only when you can and should select one of supported systems, but as we deprecate Qt4...

Contributor

paveljanik commented Nov 4, 2015

@laanwj IMO configure without arguments should build whatever it can build (based on dependencies, and warn user that it can't build GUI because of missing deps) and you should be able to disable GUI with --disable-gui if you want to save compile time, do not care about GUI etc. Selecting which UI has sense only when you can and should select one of supported systems, but as we deprecate Qt4...

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 4, 2015

Member

I think ./configure should only build the UI if the user did pass the --with-gui argument. In self build environment, i guess most users like to build bitcoin-core without the UI. But no strong opinion.

Member

jonasschnelli commented Nov 4, 2015

I think ./configure should only build the UI if the user did pass the --with-gui argument. In self build environment, i guess most users like to build bitcoin-core without the UI. But no strong opinion.

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Nov 4, 2015

Contributor

I'm happy with setting the --with-gui default to no, but when --with-gui is used, please do not require an argument and autoselect the highest qt available.

Contributor

paveljanik commented Nov 4, 2015

I'm happy with setting the --with-gui default to no, but when --with-gui is used, please do not require an argument and autoselect the highest qt available.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli
Member

jonasschnelli commented Nov 4, 2015

agree with @paveljanik

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 4, 2015

Member

I have no clue what happens if you do --with-gui without argument. Have you tested? You can do --without-gui if you want to build without GUI.

The default if you don't provide it is auto, which means that (with this pull), it will

  • build with Qt5 if Qt5 is installed
  • otherwise, build with Qt4 if Qt4 is installed
  • otherwise, build without GUI

I have no intention of changing the semantics there. The only thing I'm changing is what Qt is selected by default, which IMO should be Qt5.

Member

laanwj commented Nov 4, 2015

I have no clue what happens if you do --with-gui without argument. Have you tested? You can do --without-gui if you want to build without GUI.

The default if you don't provide it is auto, which means that (with this pull), it will

  • build with Qt5 if Qt5 is installed
  • otherwise, build with Qt4 if Qt4 is installed
  • otherwise, build without GUI

I have no intention of changing the semantics there. The only thing I'm changing is what Qt is selected by default, which IMO should be Qt5.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 4, 2015

Member

Agree. --with-gui already does auto-select. But right,.. as @laanwj mention, this PR would only change this auto-behavior to Qt5 instead of Qt4.

Do we need adaptation here:?
https://github.com/bitcoin/bitcoin/blob/master/build-aux/m4/bitcoin_qt.m4#L53

Member

jonasschnelli commented Nov 4, 2015

Agree. --with-gui already does auto-select. But right,.. as @laanwj mention, this PR would only change this auto-behavior to Qt5 instead of Qt4.

Do we need adaptation here:?
https://github.com/bitcoin/bitcoin/blob/master/build-aux/m4/bitcoin_qt.m4#L53

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 4, 2015

Member

@jonasschnelli good catch, updated

Member

laanwj commented Nov 4, 2015

@jonasschnelli good catch, updated

build: If both Qt4 and Qt5 are installed, use Qt5
If both Qt4 and Qt5 development headers are installed, use Qt5. Building
against Qt5 should be encouraged as that is where active development
happens.
@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Nov 4, 2015

Contributor

I just want to save the work again at qt6 bumb: just do not document the qt version in the doc/build-osx.md the Qt version at all.

Contributor

paveljanik commented Nov 4, 2015

I just want to save the work again at qt6 bumb: just do not document the qt version in the doc/build-osx.md the Qt version at all.

@laanwj laanwj merged commit dbacc69 into bitcoin:master Nov 4, 2015

1 check passed

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

laanwj added a commit that referenced this pull request Nov 4, 2015

Merge pull request #6938
dbacc69 build: If both Qt4 and Qt5 are installed, use Qt5 (Wladimir J. van der Laan)
@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Nov 5, 2015

Member

Shouldn't the preferred Qt be based on what the OS default is configured for...?

Member

luke-jr commented Nov 5, 2015

Shouldn't the preferred Qt be based on what the OS default is configured for...?

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 5, 2015

Member

@luke-jr That may have been a better choice in the beginning, but right now, we're slowly moving away from Qt4. I expect to retain compatibility for the foreseeable future, but the GUI experience may be worse than Qt5, so we pick what is best for us.

Member

laanwj commented Nov 5, 2015

@luke-jr That may have been a better choice in the beginning, but right now, we're slowly moving away from Qt4. I expect to retain compatibility for the foreseeable future, but the GUI experience may be worse than Qt5, so we pick what is best for us.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Nov 5, 2015

Member

Note: I need to set --with-gui=qt4 or merge #6248, to get this working on fedora.

Member

MarcoFalke commented Nov 5, 2015

Note: I need to set --with-gui=qt4 or merge #6248, to get this working on fedora.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Nov 5, 2015

Member

Tested ACK. Nice, bitcoin-qt looks so much more clean when built with qt5! (No missing icons, styles, etc.)

Member

MarcoFalke commented Nov 5, 2015

Tested ACK. Nice, bitcoin-qt looks so much more clean when built with qt5! (No missing icons, styles, etc.)

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