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

Travis: Test build against system libs (& Qt4) #7142

Merged
merged 2 commits into from Sep 12, 2017

Conversation

Projects
None yet
7 participants
@luke-jr
Member

luke-jr commented Dec 1, 2015

Also removes now-unnecessary bc dependency from other builds.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Dec 1, 2015

Member

Concept ACK on testing with qt4, but I'm not entirely happy about adding yet another build for this, Travis is slow enough these days.

Member

laanwj commented Dec 1, 2015

Concept ACK on testing with qt4, but I'm not entirely happy about adding yet another build for this, Travis is slow enough these days.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Dec 1, 2015

Member

Concept ACK.
Agree with @laanw but IMO another build is more valuable then a speedy CI.

Member

jonasschnelli commented Dec 1, 2015

Concept ACK.
Agree with @laanw but IMO another build is more valuable then a speedy CI.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Dec 1, 2015

Member

@jonasschnelli I wonder if there's a compromise, if we can combine the qt4 build with one of the existing builds

Member

laanwj commented Dec 1, 2015

@jonasschnelli I wonder if there's a compromise, if we can combine the qt4 build with one of the existing builds

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Dec 1, 2015

Member

We only have one linux "full" Qt5 build "32bit + dash". The other linux QT5 build does not build the wallet. Maybe it would be worth to change one of the windows builds (Win32) to build with Qt4? On the other hand, Qt4 mostly makes sense on Linux, but even though, a Win32 Qt4 build should at least detect most Qt4 incompatibilities.

Member

jonasschnelli commented Dec 1, 2015

We only have one linux "full" Qt5 build "32bit + dash". The other linux QT5 build does not build the wallet. Maybe it would be worth to change one of the windows builds (Win32) to build with Qt4? On the other hand, Qt4 mostly makes sense on Linux, but even though, a Win32 Qt4 build should at least detect most Qt4 incompatibilities.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Dec 1, 2015

Member

Yeah. That may work.
Anyhow this is up to @theuni. Another option would be to combine this build with something else that we wanted to test already, so that it's not just added for qt4.

Member

laanwj commented Dec 1, 2015

Yeah. That may work.
Anyhow this is up to @theuni. Another option would be to combine this build with something else that we wanted to test already, so that it's not just added for qt4.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Dec 2, 2015

Member

@theuni Can't seem to get this to build with Qt - any ideas? Shouldn't --with-qt=qt4 configure fail if it has a problem? :/

Member

luke-jr commented Dec 2, 2015

@theuni Can't seem to get this to build with Qt - any ideas? Shouldn't --with-qt=qt4 configure fail if it has a problem? :/

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Dec 2, 2015

Member

@laanwj Oh, an idea if Travis time is a concern: make this build use only system libraries (no depends). That's probably better test coverage anyway...

Member

luke-jr commented Dec 2, 2015

@laanwj Oh, an idea if Travis time is a concern: make this build use only system libraries (no depends). That's probably better test coverage anyway...

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Dec 2, 2015

Member

@laanwj Oh, an idea if Travis time is a concern: make this build use only system libraries (no depends). That's probably better test coverage anyway...

Not a bad idea because of the extra test coverage with old libraries, but I don't think that will save much time as the depends are supposed to be built once and cached anyway.

Member

laanwj commented Dec 2, 2015

@laanwj Oh, an idea if Travis time is a concern: make this build use only system libraries (no depends). That's probably better test coverage anyway...

Not a bad idea because of the extra test coverage with old libraries, but I don't think that will save much time as the depends are supposed to be built once and cached anyway.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Dec 2, 2015

Member

supposed to be built once and cached anyway.

Does travis cache between builds, I don't think so?

Member

MarcoFalke commented Dec 2, 2015

supposed to be built once and cached anyway.

Does travis cache between builds, I don't think so?

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Dec 2, 2015

Member

No, not between builds, I think. In most cases this would make no sense as they have different flags or even a different platform.

Member

laanwj commented Dec 2, 2015

No, not between builds, I think. In most cases this would make no sense as they have different flags or even a different platform.

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Dec 2, 2015

Member

Right. To clarify, depends are cached and re-used for subsequent builds of the same config. Each config is an island though.

Personally, I don't think this is worth adding another build for. If anything, I'd rather do as @laanwj said and combine it with another.

Member

theuni commented Dec 2, 2015

Right. To clarify, depends are cached and re-used for subsequent builds of the same config. Each config is an island though.

Personally, I don't think this is worth adding another build for. If anything, I'd rather do as @laanwj said and combine it with another.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Dec 3, 2015

Member

Travis PR test is quite slow lately. I'd certainly prefer not to add another config.

Testing more obscure platforms and library versions (like Qt4) may be better suited for something like a nightly extended build (e.g. #7148)

Member

laanwj commented Dec 3, 2015

Travis PR test is quite slow lately. I'd certainly prefer not to add another config.

Testing more obscure platforms and library versions (like Qt4) may be better suited for something like a nightly extended build (e.g. #7148)

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jan 13, 2016

Member

Once #7339 is merged, I suggest this can be extended to test --without-libevent as well.

Member

luke-jr commented Jan 13, 2016

Once #7339 is merged, I suggest this can be extended to test --without-libevent as well.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jan 16, 2016

Member

Closing, this is part of #7148 now

Member

laanwj commented Jan 16, 2016

Closing, this is part of #7148 now

@laanwj laanwj closed this Jan 16, 2016

@laanwj laanwj reopened this May 25, 2017

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 24, 2017

Member

Needs rebase.

Member

laanwj commented Jun 24, 2017

Needs rebase.

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Jun 24, 2017

Member

Concept ACK

Member

jtimon commented Jun 24, 2017

Concept ACK

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 5, 2017

Member

Rebased, sorry for the delay

Member

luke-jr commented Jul 5, 2017

Rebased, sorry for the delay

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Jul 11, 2017

Contributor

Concept ACK. Only concern is are we willing to add more jobs to travis? Can we pay them to make things go faster?

Contributor

TheBlueMatt commented Jul 11, 2017

Concept ACK. Only concern is are we willing to add more jobs to travis? Can we pay them to make things go faster?

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jul 30, 2017

Member

@TheBlueMatt According to apache, it is possible to increase the number of concurrent builds. (https://blogs.apache.org/infra/entry/apache_gains_additional_travis_ci)

Though, I am slightly worried that this makes it harder for developers to fork the project and enable travis on their fork. After switching to the paid infrastructure, we might have faster machines or even extended timeouts for the build times. By forking the project, the new limits no longer apply and builds might time out or fail, which is a nuisance for new contributors.

Member

MarcoFalke commented Jul 30, 2017

@TheBlueMatt According to apache, it is possible to increase the number of concurrent builds. (https://blogs.apache.org/infra/entry/apache_gains_additional_travis_ci)

Though, I am slightly worried that this makes it harder for developers to fork the project and enable travis on their fork. After switching to the paid infrastructure, we might have faster machines or even extended timeouts for the build times. By forking the project, the new limits no longer apply and builds might time out or fail, which is a nuisance for new contributors.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jul 30, 2017

Member

Is this still WIP or ready for review?

Member

MarcoFalke commented Jul 30, 2017

Is this still WIP or ready for review?

Show outdated Hide outdated .travis.yml

@luke-jr luke-jr changed the title from [WIP] Travis: Test build against system Qt4 to Travis: Test build against system Qt4 Jul 30, 2017

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Aug 5, 2017

Member

Fixed issues, rebased, and removed bc from the other builds too.

Member

luke-jr commented Aug 5, 2017

Fixed issues, rebased, and removed bc from the other builds too.

@luke-jr luke-jr changed the title from Travis: Test build against system Qt4 to Travis: Test build against system libs (& Qt4) Aug 5, 2017

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Aug 5, 2017

Member

There, tests pass now, and it tests against more system dependencies as well.

Member

luke-jr commented Aug 5, 2017

There, tests pass now, and it tests against more system dependencies as well.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke
Member

MarcoFalke commented Aug 8, 2017

utACK 6d2aac8

@MarcoFalke MarcoFalke merged commit 6d2aac8 into bitcoin:master Sep 12, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

MarcoFalke added a commit that referenced this pull request Sep 12, 2017

Merge #7142: Travis: Test build against system libs (& Qt4)
6d2aac8 Travis: Test build against system libs (& Qt4) (Luke Dashjr)
8d82e13 Travis: Remove bc tool from dependencies (Luke Dashjr)

Pull request description:

  Also removes now-unnecessary `bc` dependency from other builds.

Tree-SHA512: 815215994eeba0acf27774f57cf3a0bf77bbc22834d3242a227e0d90b5948a05f8b5ef846eb384e3ee575bec60880ae215ccc3882f13b60004a62549d3b3a28f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment