build: Remove unnecessary executables from gitian release by laanwj · Pull Request #7776 · bitcoin/bitcoin

laanwj · 2016-03-31T12:56:14Z

This removes the following executables from the binary gitian release:

test_bitcoin-qt[.exe]
bench_bitcoin[.exe]

@jonasschnelli and me discussed this on IRC a few days ago - unlike the normal bitcoin_tests which is useful to see if it is safe to run bitcoin on a certain OS/environment combination, there is no good reason
to include these. Better to leave them out to reduce the download size.

Sizes from the 0.12 release:

2.4M bitcoin-0.12.0/bin/bench_bitcoin.exe
 22M bitcoin-0.12.0/bin/test_bitcoin-qt.exe

This adds an option --disable-gui-tests to the build system.

maflcko · 2016-03-31T13:03:04Z

utACK 8e25246

btcdrak · 2016-03-31T13:10:56Z

utACK 8e25246

jonasschnelli · 2016-03-31T13:32:17Z

utACK 8e25246

cdecker · 2016-03-31T14:27:03Z

utACK 8e25246

theuni · 2016-03-31T17:05:09Z

No objections to the changes here, but I think we also want theuni@72946e1 ?
That prevents them from being installed by 'make install' under any config.

fanquake · 2016-04-01T09:29:30Z

utACK 8e25246

laanwj · 2016-04-01T12:20:15Z

No objections to the changes here, but I think we also want theuni/bitcoin@72946e1 ?

I think it's a bit inconsistent - what if people want them installed?
In any case I think it's a separate change with a separate goal to this, probably should do it as a separate PR.

laanwj · 2016-04-02T06:39:43Z

I think it's a bit inconsistent - what if people want them installed?

A consistent policy there would be "do not install the tests at all". But then we'd have to take special care to put test_bitcoin into the distribution :( I mean it's harder to make a case for the build system (which is also used by other distributions which may have other choices) that that one is special, than on the gitian descriptor level.
Let's leave it for another PR anyhow.

Testing this in gitian now.

theuni · 2016-04-02T07:08:34Z

Yep, i suppose I agree. ut ACK.

laanwj · 2016-04-02T08:50:38Z

~~Linux works~~ UHM I mean doesn't work, builds but the stupid files are still there 🐴
[snip]
May have been using the wrong descriptors, retrying...

laanwj · 2016-04-02T09:50:07Z

It's not properly disabling test_bitcoin-qt. And this is probably the reason:

$ git grep BUILD_TEST_QT
configure.ac:    BUILD_TEST_QT="test"
configure.ac:AC_SUBST(BUILD_TEST_QT)

E.g. we don't actually use BUILD_TEST_QT anywhere.

@jonasschnelli

This removes the following executables from the binary gitian release: - test_bitcoin-qt[.exe] - bench_bitcoin[.exe] @jonasschnelli and me discussed this on IRC a few days ago - unlike the normal `bitcoin_tests` which is useful to see if it is safe to run bitcoin on a certain OS/environment combination, there is no good reason to include these. Better to leave them out to reduce the download size. Sizes from the 0.12 release: ``` 2.4M bitcoin-0.12.0/bin/bench_bitcoin.exe 22M bitcoin-0.12.0/bin/test_bitcoin-qt.exe ```

laanwj · 2016-04-03T13:15:15Z

@theuni Pushed a new version which does work. Can you take a look? I removed a few AC_SUBST( which weren't used at all. But used these variables, which are set at the place where the log message is printed,

  BUILD_TEST_QT=""
  ...
  AC_MSG_CHECKING([whether to build test_bitcoin-qt])
  if test x$use_gui_tests$bitcoin_enable_qt_test = xyesyes; then
    AC_MSG_RESULT([yes])
    BUILD_TEST_QT="yes"
  else

and then use them at the end:

AM_CONDITIONAL([ENABLE_TESTS],[test x$BUILD_TEST = xyes])
AM_CONDITIONAL([ENABLE_QT_TESTS],[test x$BUILD_TEST_QT = xyes])

I think this is clearer - no need to repeat the logic. But maybe I'm doing something completely against autoconf gospels I don't know....

theuni · 2016-04-04T18:08:02Z

@laanwj Looks ok to me, and works as described in a quick test.

The one case that's unclear is "--disable-tests --enable-gui-tests". As-is it built test_bitcoin-qt but not bitcoin_test. That's fine by me (I don't really care either way which one overrides), but it might help to make the help string more clear that the gui tests don't imply non-gui tests.

laanwj · 2016-04-05T10:38:12Z

The one case that's unclear is "--disable-tests --enable-gui-tests". As-is it built test_bitcoin-qt but not bitcoin_test. That's fine by me

That was the intended behavior, yes. This allows the maximum flexibility: if you want the GUI tests but not normal tests, or normal tests but not GUI tests, both is possible. Could try to improve the help string.

Edit: On second thought

do not compile GUI tests

I'm not sure how I would go around making this clearer.

f063863 build: Remove unnecessary executables from gitian release (Wladimir J. van der Laan)

@jonasschnelli

This removes the following executables from the binary gitian release: - test_bitcoin-qt[.exe] - bench_bitcoin[.exe] @jonasschnelli and me discussed this on IRC a few days ago - unlike the normal `bitcoin_tests` which is useful to see if it is safe to run bitcoin on a certain OS/environment combination, there is no good reason to include these. Better to leave them out to reduce the download size. Sizes from the 0.12 release: ``` 2.4M bitcoin-0.12.0/bin/bench_bitcoin.exe 22M bitcoin-0.12.0/bin/test_bitcoin-qt.exe ``` Github-Pull: #7776 Rebased-From: f063863

@jonasschnelli

This removes the following executables from the binary gitian release: - test_bitcoin-qt[.exe] - bench_bitcoin[.exe] @jonasschnelli and me discussed this on IRC a few days ago - unlike the normal `bitcoin_tests` which is useful to see if it is safe to run bitcoin on a certain OS/environment combination, there is no good reason to include these. Better to leave them out to reduce the download size. Sizes from the 0.12 release: ``` 2.4M bitcoin-0.12.0/bin/bench_bitcoin.exe 22M bitcoin-0.12.0/bin/test_bitcoin-qt.exe ``` Github-Pull: bitcoin#7776 Rebased-From: f063863

Forgot to do this in bitcoin#7776.

Upstream gitian updates This PR pulls in all gitian-related PRs that have been merged upstream since 0.11.2. The only ones I left out were documentation-only PRs, because we removed `doc/gitian-building.md` at some point. Here are the commits applied here, in the order shown in `git log` (ie. last to first): - bitcoin/bitcoin#7283 - fa42a67 - fa58c76 - bitcoin/bitcoin#8175 - 74c1347 - bitcoin/bitcoin#8167 - 7e7eb27 - ad38204 - b676f38 - bitcoin/bitcoin#7776 - f063863 - bitcoin/bitcoin#7424 - a81c87f ~ we already partly applied - a8ce872 - f3d3eaf ~ we already partly applied - 475813b - ~~cd27bf5~~ X we already applied - bitcoin/bitcoin#7060 - 3b468a0 ~ we removed doc/gitian-building.md - ~~99fda26~~ X we removed doc/gitian-building.md - bitcoin/bitcoin#7251 - fa09562 - bitcoin/bitcoin#6900 - ~~2cecb24~~ X we removed doc/gitian-building.md - 957c0fd - 2e31d74 - ~~0b416c6~~ X we removed QT - 9f251b7 - bitcoin/bitcoin#6854 - 579b863 ~ we already partly applied Part of #540

@jonasschnelli

This removes the following executables from the binary gitian release: - test_bitcoin-qt[.exe] - bench_bitcoin[.exe] @jonasschnelli and me discussed this on IRC a few days ago - unlike the normal `bitcoin_tests` which is useful to see if it is safe to run bitcoin on a certain OS/environment combination, there is no good reason to include these. Better to leave them out to reduce the download size. Sizes from the 0.12 release: ``` 2.4M bitcoin-0.12.0/bin/bench_bitcoin.exe 22M bitcoin-0.12.0/bin/test_bitcoin-qt.exe ``` Github-Pull: #7776 Rebased-From: f063863

laanwj added Build system Needs backport labels Mar 31, 2016

laanwj force-pushed the 2016_03_gitian_release_cleanup branch from 8e25246 to f063863 Compare April 3, 2016 13:11

laanwj merged commit f063863 into bitcoin:master Apr 5, 2016

laanwj added a commit that referenced this pull request Apr 5, 2016

Merge #7776: build: Remove unnecessary executables from gitian release

3cc0fb3

f063863 build: Remove unnecessary executables from gitian release (Wladimir J. van der Laan)

laanwj mentioned this pull request Jun 8, 2016

gitian: Ship debug tarballs/zips with debug symbols #8167

Merged

laanwj mentioned this pull request Jun 8, 2016

gitian: Add --disable-bench to config flags for windows #8175

Merged

laanwj added a commit to laanwj/bitcoin that referenced this pull request Jun 9, 2016

gitian: Add --disable-bench to config flags for windows

74c1347

Forgot to do this in bitcoin#7776.

schinzelh pushed a commit to dashpay/dash that referenced this pull request Jun 20, 2016

gitian: Add --disable-bench to config flags for windows

bccc3b9

Forgot to do this in bitcoin#7776.

schinzelh mentioned this pull request Jun 20, 2016

V0.12.1.x gitian improvements dashpay/dash#888

Merged

laanwj removed the Needs backport label Sep 26, 2016

laanwj added this to the 0.12.2 milestone Sep 26, 2016

laanwj added the Needs backport label Sep 26, 2016

str4d mentioned this pull request Oct 15, 2016

Upstream gitian updates zcash/zcash#1541

Merged

fanquake removed the Needs backport label Mar 7, 2018

jonasschnelli mentioned this pull request May 21, 2018

Re-add bench_bitcoin to gitian binaries #13292

Closed

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

Conversation

laanwj commented Mar 31, 2016

Uh oh!

maflcko commented Mar 31, 2016

Uh oh!

btcdrak commented Mar 31, 2016

Uh oh!

jonasschnelli commented Mar 31, 2016

Uh oh!

cdecker commented Mar 31, 2016

Uh oh!

theuni commented Mar 31, 2016

Uh oh!

fanquake commented Apr 1, 2016

Uh oh!

laanwj commented Apr 1, 2016

Uh oh!

laanwj commented Apr 2, 2016

Uh oh!

theuni commented Apr 2, 2016

Uh oh!

laanwj commented Apr 2, 2016

Uh oh!

laanwj commented Apr 2, 2016

Uh oh!

laanwj commented Apr 3, 2016

Uh oh!

theuni commented Apr 4, 2016

Uh oh!

laanwj commented Apr 5, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants