Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Update contrib/debian to latest Ubuntu PPA upload. #10328

Merged
merged 4 commits into from May 11, 2017

Conversation

Projects
None yet
6 participants
Contributor

TheBlueMatt commented May 3, 2017

This:

  • Partially reverts 9f68ed6 (which fixed spelling in a changelog,
    though generally changelogs should be append-only).
  • Disables UPnP support (PPA has not had it for a while, and I
    still don't trust miniupnpc, plus it seems uneccessary - its
    been a while since we needed to care about Bitcoin-Qt home users
    getting their inbound ports auto-mapped).
  • Enables ZMQ.
  • Forces GUI to Qt4 to fix various issues people have been seeing
    on Ubuntu and elsewhere with Qt5.
  • Reverts 70899d7 (Bitcoin does not enable "instant payments",
    not is transaction management "carried out collectively by the
    network", for whatever "transaction management" means, finally
    Bitcoin Core is not the only way to use the Bitcoin currency,
    as seemingly implied in the description).
Update contrib/debian to latest Ubuntu PPA upload.
This:
 * Partially reverts 9f68ed6 (which fixed spelling in a changelog,
   though generally changelogs should be append-only).
 * Disables UPnP support (PPA has not had it for a while, and I
   still don't trust miniupnpc, plus it seems uneccessary - its
   been a while since we needed to care about Bitcoin-Qt home users
   getting their inbound ports auto-mapped).
 * Enables ZMQ.
 * Forces GUI to Qt4 to fix various issues people have been seeing
   on Ubuntu and elsewhere with Qt5.
 * Reverts 70899d7 (Bitcoin does not enable "instant payments",
   not is transaction management "carried out collectively by the
   network", for whatever "transaction management" means, finally
   Bitcoin Core is not the only way to use the Bitcoin currency,
   as seemingly implied in the description).
Owner

laanwj commented May 3, 2017

Forces GUI to Qt4 to fix various issues people have been seeing
on Ubuntu and elsewhere with Qt5.

Which issues have you seen with Qt5 apart from the Unity tray icon problem?

Reverts 70899d7 (Bitcoin does not enable "instant payments",
not is transaction management "carried out collectively by the
network", for whatever "transaction management" means, finally
Bitcoin Core is not the only way to use the Bitcoin currency,
as seemingly implied in the description).

Probably want to change the README.md too, then, as that's where that description came from.

Contributor

TheBlueMatt commented May 3, 2017

Hm, I was under the impression there were tray icon issues outside of Ubuntu, but that may be out-of-date (or a static-linking issue?)

parazyd commented May 3, 2017

@TheBlueMatt There are no Qt5 issues besides Ubuntu. I've tested the build on Devuan's KDE5 (Plasma), which is in turn the exact same environment provided in Debian. So Ubuntu is your only problem. If you want to block all other distros from using current software because of one distro's bug, that's your choice, but it is not a sane one. Build it with Qt5 and then fix it in Ubuntu.

@laanwj laanwj added the Build system label May 3, 2017

Contributor

TheBlueMatt commented May 3, 2017

@parazyd up until today the only user of contrib/debian I was aware of was the PPA, and there are very few issues (aside from some cosmetic ones) with building with Qt4 instead of 5 (though that may be different on KDE5, apparently). I'm happy to keep on maintaining a fork of contrib/debain instead if you want to maintain contrib/debian, but at that point we might as well just delete the folder if everyone is gonna use their own.

Member

luke-jr commented May 3, 2017

I don't see how optional UPnP support is a bad thing. Even when built with it, it's still disabled by default.

Other than that, utACK.

parazyd commented May 3, 2017

ACK. Agreed with @luke-jr

Contributor

TheBlueMatt commented May 3, 2017

@luke I believe its enabled by default in Qt, no?

Member

luke-jr commented May 3, 2017

@TheBlueMatt Not since the last exploit IIRC.

This fails for precise:

The following packages have unmet dependencies:
 sbuild-build-depends-bitcoin-dummy : Depends: libzmq3-dev but it is not installable
Contributor

TheBlueMatt commented May 3, 2017

Member

luke-jr commented May 3, 2017

Ah. I guess precise doesn't matter anymore anyway (supposedly support for it ended last month).

Contributor

jaromil commented May 4, 2017

You need a neew maintainer for Debian builds, not a new PR rushed by the old neglecting maintainer when a new contribution comes in, IMHO. This turf-war attitude calls for burn out. At Dyne we are actively testing the builds proposed and can well deal with guidelines and complexities of gnu/linux packaging, so @bluematt once again (like at the time of autoconf) you are missing an occasion for having more contributors to core. Friendly yours

Contributor

TheBlueMatt commented May 4, 2017

@jaromil are y'all using contrib/debian for anything? If it gets use I'm happy to start maintaining it again (or @parazyd can). The PPA never falls behind, just this dir which I've neglected updating (but I assume not many people used it given it was missing the libevent dep for a long time).

Unrelatedly, would be nice if we have a way to switch to Qt5 when building on non-ubuntu, @parazyd any idea how to do that?

parazyd commented May 4, 2017

@TheBlueMatt Well, the debian/rules file is a Makefile. We could use environment flags to differ between Ubuntu and Debian I guess.

The contrib/debian is being used by at least three distributions at this moment: Ubuntu, Devuan, and heads.

Member

luke-jr commented May 4, 2017

But the qt4/5 dep is in debian/control - can that have conditionals?

parazyd commented May 4, 2017

@luke-jr I'll try to find out. If possible, then it is the best solution, since the configure script will then prefer what's found. And as long as it's built in some sort of CI, the environment is clean.

EDIT:
Indeed we have a thing like the following:
https://raphaelhertzog.com/2010/09/27/different-dependencies-between-debian-and-ubuntu-but-common-source-package/

Contributor

TheBlueMatt commented May 4, 2017

@luke-jr the deps of the built package are auto-guessed from library deps, I believe, so it may be sufficient to just change which libraries configure links against, even if buildeps requires both qt4 and qt5 (though I havent tested this).

contrib/debian/control
- libboost-thread-dev (>> 1.35) | libboost-thread1.35-dev,
- libboost-test-dev (>> 1.35) | libboost-test1.35-dev,
+ libevent-dev,
+ libboost-system1.48-dev | libboost-system-dev (>> 1.35),
@theuni

theuni May 4, 2017

Member

Minimum boost for 0.14 is 1.47.

@TheBlueMatt

TheBlueMatt May 4, 2017

Contributor

Fixed.

Contributor

jaromil commented May 4, 2017

@TheBlueMatt there are the distro efforts @parazyd mentions and we are planning for more hardened gnu/linux based VM environments (so to say containers) and what not, some fancy distro tricks for isolated environments. Within the sphere of Debian&co the backporting libdb4.8 to Jessie was a long due liberation step finally taken and you know well the berkeleydb mess we are talking about. So yea, there is attention on the contrib/debian and this and the other PR are here because we think we are ready to contribute. Said that I see nothing really problematic in this process and I'm glad you find the time to look into it. Knowing fairly well the autoconf pains your prolongued work deserves much respect.

Contributor

TheBlueMatt commented May 4, 2017

@parazyd I pushed a version that should use Qt5 automagically on Debian-based distros, but dont have a machine handy to test on, care to give it a whirl?

contrib/debian/rules
@@ -12,10 +12,16 @@ override_dh_auto_clean:
if [ -f Makefile ]; then $(MAKE) distclean; fi
rm -rf Makefile.in aclocal.m4 configure src/Makefile.in src/bitcoin-config.h.in src/build-aux src/qt/Makefile.in src/qt/test/Makefile.in src/test/Makefile.in
+ifeq ($(shell dpkg-vendor --derives-from Ubuntu && echo yes),yes)
@luke-jr

luke-jr May 4, 2017

Member
QT=$(shell dpkg-vendor --derives-from Ubuntu && echo qt4 || echo qt5)

or better yet (to enable Qt5 on Ubuntu 17.10+ which is GNOME instead of Unity):

QT=$(shell { [ $(lsb_release -s -i) = Ubuntu ] && { [ $(lsb_release -s -r | cut -d. -f1) -lt 17 ] || [ $(lsb_release -s -r) = 17.04 ]; } } && echo qt4 || echo qt5)
@TheBlueMatt

TheBlueMatt May 4, 2017

Contributor

Maybe lets wait until 10.17 before we lock in using qt5 in 17.10 in case they slip to the next release or so?

@parazyd

parazyd May 4, 2017

@TheBlueMatt Your if clause doesn't pass $QT correctly, on my Devuan build it stays empty. @luke-jr 's lsb_release oneliner works.

@TheBlueMatt

TheBlueMatt May 4, 2017

Contributor

Wait, the dpkg-vendor version doesn't work, or the if doesnt hold the variable outside of the scope?

@parazyd

parazyd May 4, 2017

$(QT) is empty on the ./configure call

@luke-jr

luke-jr May 4, 2017

Member

That's odd, I would expect both to work... :/

@parazyd

parazyd May 4, 2017

OK, it seems if you don't indent the QT= assigns, it works.

contrib/debian/rules
# Yea, autogen should be run on the source archive, but I like doing git archive
override_dh_auto_configure:
./autogen.sh
- ./configure
+ ./configure --without-miniupnpc --with-gui=$(QT)
@luke-jr

luke-jr May 4, 2017

Member

Don't forget to drop --without-miniupnpc?

(and restore libminiupnpc8-dev | libminiupnpc-dev (>> 1.6), in build deps)

Introducing typos?

@@ -179,7 +292,7 @@ bitcoin (0.5.3-natty0) natty; urgency=low
bitcoin (0.5.2-natty1) natty; urgency=low
* Remove mentions on anonymity in package descriptions and manpage.
- These should never have been there, bitcoin isn't anonymous without
+ These should never have been there, bitcoin isnt anonymous without
@parazyd

parazyd May 4, 2017

Is there a reason for introducing these typos?

@luke-jr

luke-jr May 4, 2017

Member

Yes, read the OP...

Partially reverts 9f68ed6 (which fixed spelling in a changelog,
though generally changelogs should be append-only).

@parazyd

parazyd May 4, 2017

I fail to understand the reasoning.

Contributor

TheBlueMatt commented May 5, 2017

Fixed Qt version auto-detection and re-enabled UPnP.

luke-jr approved these changes May 5, 2017

utACK

parazyd approved these changes May 5, 2017

Looks good

@laanwj laanwj merged commit 91700aa into bitcoin:master May 11, 2017

1 check passed

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

laanwj added a commit that referenced this pull request May 11, 2017

Merge #10328: Update contrib/debian to latest Ubuntu PPA upload.
91700aa Re-enable upnp support in contrib/debian (Matt Corallo)
c5071e1 Build with QT5 on Debian-based systems using contrib/debian (Matt Corallo)
a8e9286 Bump minimum boost version in contrib/debian (Matt Corallo)
9970219 Update contrib/debian to latest Ubuntu PPA upload. (Matt Corallo)

Tree-SHA512: ee4d3c5927a9cfb2794672eaca883c4af5df541383afbdbc6500714ee17518e78b58f509b2e9805bbc424ef97a5e64be0b9a977212c5002cb682f0569d28099b

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 3, 2017

Update contrib/debian to latest Ubuntu PPA upload.
This:
 * Partially reverts 9f68ed6 (which fixed spelling in a changelog,
   though generally changelogs should be append-only).
 * Disables UPnP support (PPA has not had it for a while, and I
   still don't trust miniupnpc, plus it seems uneccessary - its
   been a while since we needed to care about Bitcoin-Qt home users
   getting their inbound ports auto-mapped).
 * Enables ZMQ.
 * Forces GUI to Qt4 to fix various issues people have been seeing
   on Ubuntu and elsewhere with Qt5.
 * Reverts 70899d7 (Bitcoin does not enable "instant payments",
   not is transaction management "carried out collectively by the
   network", for whatever "transaction management" means, finally
   Bitcoin Core is not the only way to use the Bitcoin currency,
   as seemingly implied in the description).

Github-Pull: #10328
Rebased-From: 9970219

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 3, 2017

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 3, 2017

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 3, 2017

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 3, 2017

Update contrib/debian to latest Ubuntu PPA upload.
This:
 * Partially reverts 9f68ed6 (which fixed spelling in a changelog,
   though generally changelogs should be append-only).
 * Disables UPnP support (PPA has not had it for a while, and I
   still don't trust miniupnpc, plus it seems uneccessary - its
   been a while since we needed to care about Bitcoin-Qt home users
   getting their inbound ports auto-mapped).
 * Enables ZMQ.
 * Forces GUI to Qt4 to fix various issues people have been seeing
   on Ubuntu and elsewhere with Qt5.
 * Reverts 70899d7 (Bitcoin does not enable "instant payments",
   not is transaction management "carried out collectively by the
   network", for whatever "transaction management" means, finally
   Bitcoin Core is not the only way to use the Bitcoin currency,
   as seemingly implied in the description).

Github-Pull: #10328
Rebased-From: 9970219

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 3, 2017

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 3, 2017

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 3, 2017

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 3, 2017

Update contrib/debian to latest Ubuntu PPA upload.
This:
 * Partially reverts 9f68ed6 (which fixed spelling in a changelog,
   though generally changelogs should be append-only).
 * Disables UPnP support (PPA has not had it for a while, and I
   still don't trust miniupnpc, plus it seems uneccessary - its
   been a while since we needed to care about Bitcoin-Qt home users
   getting their inbound ports auto-mapped).
 * Enables ZMQ.
 * Forces GUI to Qt4 to fix various issues people have been seeing
   on Ubuntu and elsewhere with Qt5.
 * Reverts 70899d7 (Bitcoin does not enable "instant payments",
   not is transaction management "carried out collectively by the
   network", for whatever "transaction management" means, finally
   Bitcoin Core is not the only way to use the Bitcoin currency,
   as seemingly implied in the description).

Github-Pull: #10328
Rebased-From: 9970219

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 3, 2017

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 3, 2017

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 3, 2017

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 5, 2017

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 5, 2017

nomnombtc added a commit to nomnombtc/bitcoin that referenced this pull request Jul 17, 2017

Update contrib/debian to latest Ubuntu PPA upload.
This:
 * Partially reverts 9f68ed6 (which fixed spelling in a changelog,
   though generally changelogs should be append-only).
 * Disables UPnP support (PPA has not had it for a while, and I
   still don't trust miniupnpc, plus it seems uneccessary - its
   been a while since we needed to care about Bitcoin-Qt home users
   getting their inbound ports auto-mapped).
 * Enables ZMQ.
 * Forces GUI to Qt4 to fix various issues people have been seeing
   on Ubuntu and elsewhere with Qt5.
 * Reverts 70899d7 (Bitcoin does not enable "instant payments",
   not is transaction management "carried out collectively by the
   network", for whatever "transaction management" means, finally
   Bitcoin Core is not the only way to use the Bitcoin currency,
   as seemingly implied in the description).

Github-Pull: #10328
Rebased-From: 9970219

nomnombtc added a commit to nomnombtc/bitcoin that referenced this pull request Jul 17, 2017

nomnombtc added a commit to nomnombtc/bitcoin that referenced this pull request Jul 17, 2017

nomnombtc added a commit to nomnombtc/bitcoin that referenced this pull request Jul 17, 2017

runn1ng added a commit to runn1ng/bitcoin that referenced this pull request Oct 30, 2017

Update contrib/debian to latest Ubuntu PPA upload.
This:
 * Partially reverts 9f68ed6 (which fixed spelling in a changelog,
   though generally changelogs should be append-only).
 * Disables UPnP support (PPA has not had it for a while, and I
   still don't trust miniupnpc, plus it seems uneccessary - its
   been a while since we needed to care about Bitcoin-Qt home users
   getting their inbound ports auto-mapped).
 * Enables ZMQ.
 * Forces GUI to Qt4 to fix various issues people have been seeing
   on Ubuntu and elsewhere with Qt5.
 * Reverts 70899d7 (Bitcoin does not enable "instant payments",
   not is transaction management "carried out collectively by the
   network", for whatever "transaction management" means, finally
   Bitcoin Core is not the only way to use the Bitcoin currency,
   as seemingly implied in the description).

Github-Pull: #10328
Rebased-From: 9970219

runn1ng added a commit to runn1ng/bitcoin that referenced this pull request Oct 30, 2017

runn1ng added a commit to runn1ng/bitcoin that referenced this pull request Oct 30, 2017

runn1ng added a commit to runn1ng/bitcoin that referenced this pull request Oct 30, 2017

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