Run Qt wallet tests on travis

[ryanofsky]

Currently these test failures are not caught by travis leading to bugs like:
#10506

[jonasschnelli]

Travis fails with /sbin/start-stop-daemon: unable to stat /usr/bin/Xvfb (No such file or directory).

tail from https://api.travis-ci.org/jobs/238751025/log.txt?deansi=true:

$ if [ "$RUN_TESTS" = "true" -a "${DEP_OPTS#*NO_QT=1}" = "$DEP_OPTS" ]; then export DISPLAY=:99.0; /sbin/start-stop-daemon --start --pidfile /tmp/custom_xvfb_99.pid --make-pidfile --background --exec /usr/bin/Xvfb -- :99 -ac; fi
/sbin/start-stop-daemon: unable to stat /usr/bin/Xvfb (No such file or directory)


The command "if [ "$RUN_TESTS" = "true" -a "${DEP_OPTS#*NO_QT=1}" = "$DEP_OPTS" ]; then export DISPLAY=:99.0; /sbin/start-stop-daemon --start --pidfile /tmp/custom_xvfb_99.pid --make-pidfile --background --exec /usr/bin/Xvfb -- :99 -ac; fi" failed and exited with 2 during .

Your build has been stopped.

[ryanofsky]

Updated 030a9e9 -> 019ac7b (pr/travqt.1 -> pr/travqt.2)

[MarcoFalke]

We only have a 50 minute time slot to run one entry of the matrix. It turns out that functional tests can not be run after compiling depends/qt from scratch.

[ryanofsky]

It turns out that functional tests can not be run after compiling depends/qt from scratch.

It sounds like the only way to deal with this would be to split the RUN_TESTS variable into separate variables for python rpc and c++ tests, and then add a new travis configuration enabling wallet and Qt and RUN_TESTS for c++ but not python.

Is there a cost to creating a new travis configuration, and do people think this is worth doing? If so, I can update the PR to implement this. If not, I will close this PR, and create a new one just documenting why NO_QT is set here.

Also, rebased 019ac7b -> 76f303a (pr/travqt.2 -> pr/travqt.3) after #10509 merge.

[MarcoFalke]

The cost would be that each push takes a bit longer to run through travis. As ccache is enabled on travis, we are probably talking about 3-5 minutes + whatever it takes to run the tests.

If we are going to add a new configuration, I'd prefer a [qt4, wallet, qt_tests, no_functional_tests] config. You might take a look at #7142, which adds the qt4 stuff only.

[jonasschnelli]

Wild thoughts: what about using PPA qt5 via apt instead of a depends build for this configuration?

[ryanofsky]

The cost would be that each push takes a bit longer to run through travis. As ccache is enabled on travis, we are probably talking about 3-5 minutes + whatever it takes to run the tests.

I'm confused about whether different travis configurations run in parallel or not. If so, would adding a new configuration necessarily slow down travis pushes? And would parallel builds share the same ccache?

CCACHE_SIZE is also set to 100M on travis, which seems small. ccache on my machine after just one non-depends build fills up to 900M so I wonder if we are actually using ccache for much on travis.

Wild thoughts: what about using PPA qt5 via apt instead of a depends build for this configuration?

#7142 seems to use the qt4-dev-tools package without a PPA.

[MarcoFalke]

Indeed, one qt5 depends build should be enough. We can use packages for the other qt configurations.

Edit: Also note that the ccache is compressed, so 100M is likely enough. (The size is about 68-89M)

[MarcoFalke]

I'm confused about whether different travis configurations run in parallel or not. If so, would adding a new configuration necessarily slow down travis pushes? And would parallel builds share the same ccache?

There are 5 workers running in parallel, so if the build matrix has 7 entries, at least 2 jobs are executed with a slight delay.

Also, the cache folder/url should be unique for each configuration in the matrix.

[ryanofsky]

Got this working without having to add a new build configuration using jonasschnelli's idea and making travis build bitcoin against a packaged Qt instead of no Qt at all.

Doing this required adding a new HOST_PKG depends option so the generated config.site file would allow pkg-config to return information about locally installed libraries. The HOST_PKG option works, but I'm not sure that the naming or mechanics are very intuitive, so I'd appreciate any suggestions on improving or replacing it.

Updated 76f303a -> cd986ea (pr/travqt.3 -> pr/travqt.5)

[MarcoFalke]

Tested ACK cd986ea, though I can't comment on the build system changes.

[laanwj]

ping @theuni for build system changes

[theuni]

Concept ACK, but I'd really rather not add travis hacks into the config.site.

@ryanofsky: Rather than introducing a new var into the depends build, how about allowing the vars to be overridden if set instead? Then the build can just set --with-qt-* manually, and there's no hidden magic making it work?

It should be sufficient to specify all of the necessary --with-qt=foo options or QT_BAR vars, though it's possible that we need to check for set values in bitcoin_qt.m4 before invoking pkg-config.

[ryanofsky]

Updated cd986ea -> 4f92b5f (pr/travqt.5 -> pr/travqt.6)

I cleaned up the implementation a little, renaming the HOST_PKG option to ALLOW_HOST_PACKAGES and disentangling it from the bindir path settings.

@theuni, I think this approach is pretty clean in its current form. It seems less fragile to me to be able to use pkg-config instead of having to work around it by hardcoding a bunch of paths into .travis.yml.

[theuni]

@ryanofsky ok, I won't fight it because I agree that my proposed alternative was ugly, and this is a net win.

The entire point of our depends, though, is for deterministic builds. This change makes it possible to accidentally use system libs rather than our depends on accident. As an example of an unintended consequence, it's hard to say which openssl will be linked into bitcoin-qt here.

But as long as it's only used for Travis, I think it's ok.

I spent some time in the last few days working on a bump to qt5.9, which should build much more quickly. I'm really hoping that we'll be able to switch to that and eliminate the need for this at some point.

[ryanofsky]

The entire point of our depends, though, is for deterministic builds. This change makes it possible to accidentally use system libs rather than our depends on accident. As an example of an unintended consequence, it's hard to say which openssl will be linked into bitcoin-qt here.

To be sure, there shouldn't be any risk of nondeterminism unless you explicitly enable the ALLOW_HOST_PACKAGES option. And there should be no reason to enable this option unless you want to pull in an external dependency, which would pretty much give you a non-deterministic build by definition.

But maybe I could make the implications of setting the option clearer by renaming it to ALLOW_NONDETERMINISTIC_HOST_PACKAGES or printing a status line like "External packages enabled. Build may not be reproducible" when the option is enabled. Please let me know if you think a change along these lines would be helpful.

[jtimon]

Concept ACK

[theuni]

@ryanofsky I don't really mind either way. As long as it's not something a user will run into, I'm not picky about the name.