-
Notifications
You must be signed in to change notification settings - Fork 38.7k
Run Qt wallet tests on travis #10508
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Travis fails with |
|
Updated 030a9e9 -> 019ac7b (pr/travqt.1 -> pr/travqt.2) |
|
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
It sounds like the only way to deal with this would be to split the 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 Also, rebased 019ac7b -> 76f303a (pr/travqt.2 -> pr/travqt.3) after #10509 merge. |
|
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. |
|
Wild thoughts: what about using PPA qt5 via apt instead of a depends build for this configuration? |
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?
#7142 seems to use the |
|
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) |
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. |
850e592 to
cd986ea
Compare
|
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 Updated 76f303a -> cd986ea (pr/travqt.3 -> pr/travqt.5) |
|
Tested ACK cd986ea, though I can't comment on the build system changes. |
|
ping @theuni for build system changes |
|
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. |
|
Updated cd986ea -> 4f92b5f (pr/travqt.5 -> pr/travqt.6) I cleaned up the implementation a little, renaming the @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 |
theuni
left a comment
There was a problem hiding this 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.
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 |
|
Concept ACK |
|
@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. |
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
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
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
Currently these test failures are not caught by travis leading to bugs like:
#10506