Page MenuHomePhabricator

[CI] Refactor the build configuration by moving out tests from the build
ClosedPublic

Authored by Fabien on Sep 4 2019, 15:33.

Details

Summary

This makes it easier to individually add and configure each step for
each configuration, and makes the whole build definition easier to read.

As a (good) side effect the functional tests are no longer run with the next
upgrade enabled with the asan and ubsan configurations, and use the
standard cutoff.

Depends on D3992 and D4014.

Test Plan

Run the CI builds/IBD and check the builds are green (ubsan excepted).

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Fabien created this revision.Sep 4 2019, 15:33
Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 4 2019, 15:33
Fabien updated this revision to Diff 11086.Sep 4 2019, 19:38
Fabien edited the summary of this revision. (Show Details)

Remove setup.sh, now it is a function in build-configurations.sh.

Fabien updated this revision to Diff 11087.Sep 4 2019, 19:40

Move to the build dir *AFTER* its creation...

jasonbcox requested changes to this revision.Sep 4 2019, 20:19
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
contrib/teamcity/build-configurations.sh
28 ↗(On Diff #11087)

double comment ##

30 ↗(On Diff #11087)

This can be collapsed into the line above it.

57 ↗(On Diff #11087)

It's unnecessary to pass this value into the function only to export it again. it's probably best to define it at the top of the script.

61 ↗(On Diff #11087)

typo: test.s

84 ↗(On Diff #11087)

Food for thought: This sort of improvement could be done in a separate diff and make the review easier. I won't block this diff on this change, but just keep in mind for the future. There are other small (mostly one-liner) changes that could have had the same fate.

contrib/teamcity/build.sh
14 ↗(On Diff #11087)

Removing these defaults will break IBD tests (IBD hasn't been migrated to build-configurations yet, so the config is still partially managed by TeamCity).

This revision now requires changes to proceed.Sep 4 2019, 20:19
Fabien added inline comments.Sep 6 2019, 12:56
contrib/teamcity/build-configurations.sh
30 ↗(On Diff #11087)

Doing so will discard the return value of the command line in favor of the return value of export, and cause a linter error.

84 ↗(On Diff #11087)

I'm not sure what improvement you're talking about, but using test_runner cannot really be separated from the global move as it requires the environment to be compliant with how the test is executed. Splitting the setup function would have break the build script, and the opposite is true as well.
I your comment is not about this then please ignore mine.

Fabien edited the summary of this revision. (Show Details)Sep 6 2019, 12:56
Fabien edited the test plan for this revision. (Show Details)
Fabien updated this revision to Diff 11122.Sep 6 2019, 13:41

Rebase on top of D4014 and address feedbacks.

jasonbcox requested changes to this revision.Sep 12 2019, 21:44
jasonbcox added inline comments.
contrib/teamcity/ibd.sh
1 ↗(On Diff #11122)

This file needs another rebase, sorry.

17 ↗(On Diff #11122)

Rather than removing TOPLEVEL and not defining BUILD_DIR, provide reasonable defaults:

: "${TOPLEVEL:=$(git rev-parse --show-toplevel)}"
: "${BUILD_DIR:=${TOPLEVEL}/build}"

This preserves the ability to run this script directly without any setup.

This revision now requires changes to proceed.Sep 12 2019, 21:44
Fabien edited the test plan for this revision. (Show Details)Sep 16 2019, 09:39
Fabien updated this revision to Diff 11338.Sep 16 2019, 09:40
Fabien edited the test plan for this revision. (Show Details)

Rebase and set defaults to allow running the scripts standalone.

jasonbcox accepted this revision.Sep 16 2019, 20:31
This revision is now accepted and ready to land.Sep 16 2019, 20:31