Use winehq debs for win32 tests #12931

ajtowns commented at 8:40 AM on April 10, 2018: member

When running tests with wine 1.6, "make check" takes about 20 minutes; with wine 3.0, "make check" takes about 7 minutes, allowing the travis timeout to be lowered. Debs for wine 3.0 are available from winehq, so this patch includes their repo key.

fanquake added the label Tests on Apr 10, 2018

laanwj requested review from theuni on Apr 10, 2018

laanwj commented at 8:57 AM on April 10, 2018: member

I like using a recent wine. Unlike other kinds, winehq's wine doesn't age well.

This is their "wine-stable" so we don't (as we shouldn't) take upon us the job of testing experimental new wine, either.

Would even more prefer to bump entire test environment to a more recent distribution (ref #12511) but I don't think Travis makes that possible (or easy), so this is a good intermediate step.

utACK a833fe6df5c86c7cf3bae319a000b70048031e31(demoted to Concept ACK until this passes Travis)

Additional motivation (from IRC):

<aj> wumpus: ^^ is a first pass at stopping 686-w32 travis builds from being constantly horrible

kallewoof commented at 9:06 AM on April 10, 2018: member

Timeout (15 minutes) reached. -- I think non-Win jobs occasionally take up to 30 mins (for some reason), so this may be too low. That aside, utACK!

practicalswift commented at 9:43 AM on April 10, 2018: contributor

utACK a833fe6df5c86c7cf3bae319a000b70048031e31 module adjustment/removal of timeout change

in .travis.yml:85 in a833fe6df5 outdated

  81 | @@ -78,7 +82,7 @@ script:
  82 |      - ./configure --cache-file=../config.cache $BITCOIN_CONFIG_ALL $BITCOIN_CONFIG || ( cat config.log && false)
  83 |      - make $MAKEJOBS $GOAL || ( echo "Build failure. Verbose build follows." && make $GOAL V=1 ; false )
  84 |      - export LD_LIBRARY_PATH=$TRAVIS_BUILD_DIR/depends/$HOST/lib
  85 | -    - if [ "$RUN_TESTS" = "true" ]; then travis_wait 50 make $MAKEJOBS check VERBOSE=1; fi
  86 | +    - if [ "$RUN_TESTS" = "true" ]; then travis_wait 15 make $MAKEJOBS check VERBOSE=1; fi

practicalswift commented at 10:32 AM on April 10, 2018:

What is the reasoning behind the change from 50 to 15 here?

What is the drawback of keeping it at 50 minutes for now and lower it in the future when we've collected more data on the runtime of Windows Travis job after this change?

kallewoof commented at 10:41 AM on April 10, 2018:

It was raised from 30 to 50. I think it should be dropped to 30 again.

practicalswift commented at 10:56 AM on April 10, 2018:

30 sounds reasonable.

ajtowns commented at 12:02 PM on April 10, 2018: member

@kallewoof Hmm, the timeout was for the x86-64 win32 build, which I left using wine1.6. Anyone have any examples of a non-win32 travis job where "make check" took longer than 5 or 10 minutes?

laanwj commented at 12:18 PM on April 10, 2018: member

@kallewoof Hmm, the timeout was for the x86-64 win32 build, which I left using wine1.6.

I think that's confusing - let's switch all the wine builds around for consistency.

MarcoFalke commented at 1:08 PM on April 10, 2018: member

Needs rebase if still relevant

ajtowns force-pushed on Apr 10, 2018

ajtowns commented at 2:15 PM on April 10, 2018: member

Rebased post #12926 merge.

MarcoFalke commented at 3:07 PM on April 10, 2018: member

Seems make check would run twice as fast, now.

laanwj commented at 3:21 PM on April 10, 2018: member

re-utACK https://github.com/bitcoin/bitcoin/pull/12931/commits/63b19a67610c635385c134a21f71bb771b201c16

jnewbery commented at 5:26 PM on April 10, 2018: member

Concept ACK.

The apt-get install step takes ~ 6 minutes (https://travis-ci.org/bitcoin/bitcoin/jobs/364620673#L494). Is it possible to cache the dependency in Travis (perhaps using https://docs.travis-ci.com/user/caching#Arbitrary-directories)?

MarcoFalke commented at 6:48 PM on April 10, 2018: member

Ouch. That dwarfs any speedups gained later on. I'd highly suspect the bottleneck is installing the ~200 packages and not fetching them, so caching woulnd't help here.

jamesob commented at 7:57 PM on April 10, 2018: member

I'd highly suspect the bottleneck is installing the ~200 packages and not fetching them, so caching woulnd't help here.

That seems to somewhat contradict what Travis' documentation says on what to cache:

Large files that are quick to install but slow to download do not benefit from caching, as they take as long to download from the cache as from the original source

Why not try it?

Edit: ah, they specifically call out "Debian packages" below as something not to cache. Oops :).

MarcoFalke commented at 8:01 PM on April 10, 2018: member

Edit: ah, they specifically call out "Debian packages" below as something not to cache. Oops :).

Yeah, I'd rather not install those. I'd surprised if all of them are actually needed.

kallewoof commented at 4:49 AM on April 11, 2018: member

Could the --install-recommends part be fine tuned (replace with explicit package names) to exclude unnecessary stuff?

laanwj commented at 5:49 AM on April 11, 2018: member

Could the --install-recommends part be fine tuned (replace with explicit package names) to exclude unnecessary stuff?

Good idea. --install-recommends is almost always a bad idea (especially in the non-interactive case - it will install documentation and such!). As test_bitcoin is a basic console program I wouldn't be surprised if we only need the wine base package.

ajtowns force-pushed on Apr 11, 2018

laanwj commented at 12:12 PM on April 11, 2018: member

Needs update to commit message and PR title now that all windows tests are performed using the new wine.

jnewbery commented at 8:31 PM on April 11, 2018: member

Much better! apt-get is now taking 6 seconds on the i686-w64-mingw32 job and 30 seconds on the x86_64-w64-mingw32 job.

ajtowns renamed this:
~~Use winehq debs for i686 win32 tests~~
Use winehq debs for win32 tests
on Apr 12, 2018

ajtowns commented at 1:39 PM on April 12, 2018: member

PR title updated, commit message had already been updated. Even with --no-install-recommends, it's grabbing a lot of packages (100MB versus 130MB by the looks), and it also seems like travis's download speeds can be wildly inconsistent:

Fetched 109 MB in 2s (51.9 MB/s) -- https://travis-ci.org/bitcoin/bitcoin/jobs/364960356
Fetched 133 MB in 5min 9s (429 kB/s) -- https://travis-ci.org/bitcoin/bitcoin/jobs/364620673
Fetched 133 MB in 2s (47.3 MB/s) -- https://travis-ci.org/bitcoin/bitcoin/jobs/364574088

MarcoFalke commented at 2:04 PM on April 12, 2018: member

utACK e5997988149b7ecf41e73815c6b161b3c1dff1b4 (Would still be an improvement if it took travis 6 minutes to install winehq)

previous make check times would be between 915 and 1350 and new times between 405 and 530, so as long as we spend less than 6 minutes in apt-get it is a net win.

MarcoFalke commented at 2:04 PM on April 12, 2018: member

Just reset the travis run and it seems the slow fetch was a one-off. Changes look good to me...

jnewbery commented at 2:14 PM on April 12, 2018: member

@MarcoFalke

I'd highly suspect the bottleneck is installing the ~200 packages and not fetching them @ajtowns it also seems like travis's download speeds can be wildly inconsistent:

It appears that fetching the packages is the bottleneck here, which suggests that caching locally would help. That could be investigated in a follow-up PR - no need to solve it here if this is already a net win.

MarcoFalke commented at 2:23 PM on April 12, 2018: member

@jnewbery As correctly pointed out by @jamesob above they should not be cached according to the documentation: https://docs.travis-ci.com/user/caching#Things-not-to-cache. I suspect the packages are already cached within the travis network, so adding them a second time to the cache (explicitly) wouldn't help. I guess this was just a one-off not worth to waste time on.

jnewbery commented at 2:34 PM on April 12, 2018: member

one-off is one-too-many-off for my liking (especially in a sample size of three) :)

Like I said, this can be investigated in a future if it turns out to be a regular problem. I can't believe that downloading 133MB from Travis's internal cache can ever take as long as 5 mintues.

MarcoFalke commented at 3:12 PM on April 12, 2018: member

Yeah, right. Though, downloading the packages from the mirror shouldn't take 5 minutes either. Note that the cache is not stored internally but on S3 [1], so it might be further away from gce (where our vms run) than the gce mirror of the ubuntu packages.

[1] https://docs.travis-ci.com/user/caching#How-does-the-caching-work%3F

theuni commented at 7:03 PM on April 12, 2018: member

Has anyone tracked down exactly what is sped up by newer Wine? I'm wondering if there's some one-off test that accounts for the bulk of the difference.

theuni commented at 9:28 PM on April 12, 2018: member

See #12970, which may negate some of the gains here.

Edit: Yea, those times are even better than these. It'd be interesting to see if there's an even further speedup with a newer Wine, though I suspect it's no longer worth the hassle.

Use winehq debs for win32 tests

When running tests with wine 1.6, "make check" takes about 20 minutes;
with wine 3.0, "make check" takes about 7 minutes, allowing the travis
timeout to be lowered. Debs for wine 3.0 are available from winehq, so
this patch includes their repo key. Lowers make check timeout back to 30m.

560086e764

ajtowns force-pushed on Apr 13, 2018

ajtowns commented at 2:15 AM on April 13, 2018: member

Rebased on top of the now-merged #12970. I think at this point it's not worth the added risk/complexity of pulling in out-of-tree packages, but it does seem interesting to see if there's any further improvement. :)

MarcoFalke commented at 1:54 PM on April 13, 2018: member

Assuming that you did the rebase correctly, this appears to be running slower even when ignoring the time it takes to install wine-hq, no?

MarcoFalke commented at 3:28 AM on April 17, 2018: member

Can be closed?

ajtowns commented at 3:41 AM on April 17, 2018: member

I think so. I suspect the "running slower" is just variance from travis, but either way any gains do seem small enough not to be worthwhile. wine-stable 3.0 is in bionic as well.

ajtowns closed this on Apr 17, 2018

MarcoFalke locked this on Sep 8, 2021