Skip to content

Conversation

@ryanofsky
Copy link
Contributor

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

@jonasschnelli
Copy link
Contributor

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
Copy link
Contributor Author

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

@maflcko
Copy link
Member

maflcko commented Jun 3, 2017

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.

Currently these test failures are not caught by travis leading to bugs like:
bitcoin#10506
@ryanofsky
Copy link
Contributor Author

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.

@maflcko
Copy link
Member

maflcko commented Jun 5, 2017

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
Copy link
Contributor

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

@ryanofsky
Copy link
Contributor Author

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.

@maflcko
Copy link
Member

maflcko commented Jun 5, 2017

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)

@maflcko
Copy link
Member

maflcko commented Jun 6, 2017

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 ryanofsky force-pushed the pr/travqt branch 2 times, most recently from 850e592 to cd986ea Compare June 6, 2017 18:50
@ryanofsky
Copy link
Contributor Author

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)

@maflcko maflcko requested a review from theuni June 6, 2017 22:05
@maflcko
Copy link
Member

maflcko commented Jun 19, 2017

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

@laanwj
Copy link
Member

laanwj commented Jun 22, 2017

ping @theuni for build system changes

@theuni
Copy link
Member

theuni commented Jul 6, 2017

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
Copy link
Contributor Author

ryanofsky commented Jul 6, 2017

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.

Copy link
Member

@theuni theuni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Contributor Author

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
Copy link
Contributor

jtimon commented Jul 13, 2017

Concept ACK

@theuni
Copy link
Member

theuni commented Jul 13, 2017

@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.

@laanwj laanwj merged commit 4f92b5f into bitcoin:master Jul 25, 2017
laanwj added a commit that referenced this pull request Jul 25, 2017
4f92b5f Run Qt wallet tests on travis (Russell Yanofsky)

Pull request description:

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

Tree-SHA512: db1e4ff5b17bcd6fd000a3d21aa74f6b7e4c194e0663c1896a100612671910a7cdadd896b59642420ea016598895b54a8468914847ebefef105a3c47c311d4b2
sickpig referenced this pull request in sickpig/BitcoinUnlimited Oct 19, 2017
4f92b5f Run Qt wallet tests on travis (Russell Yanofsky)

Pull request description:

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

Tree-SHA512: db1e4ff5b17bcd6fd000a3d21aa74f6b7e4c194e0663c1896a100612671910a7cdadd896b59642420ea016598895b54a8468914847ebefef105a3c47c311d4b2
codablock pushed a commit to codablock/dash that referenced this pull request Sep 23, 2019
4f92b5f Run Qt wallet tests on travis (Russell Yanofsky)

Pull request description:

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

Tree-SHA512: db1e4ff5b17bcd6fd000a3d21aa74f6b7e4c194e0663c1896a100612671910a7cdadd896b59642420ea016598895b54a8468914847ebefef105a3c47c311d4b2
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants