Page MenuHomePhabricator
Differential D7324

[CI] Allow for describing the build from the configuration file
ClosedPublic
Actions

Authored by Fabien on Sep 2 2020, 20:03.

Details

Reviewers
jasonbcox
deadalnix
Group Reviewers
Restricted Project
Commits
rABC07debf4d3549: [CI] Allow for describing the build from the configuration file
Summary

This diff adds a new syntax for describing a build, which only requires
an entry in the configuration file instead of a script. Here are the
rules:

  • If a script is defined, only the script is run.
  • Otherwise, the configuration can define a list of targets, each entry being a list of the targets to run in parallel. They are run in order.

Example: the following configuration:

targets": [
    ["all", "install"],
    ["check", "check-functional"]
]

will lead to the following calls:

ninja all install
ninja check check-functional
  • An optional cmake_flags list takes arguments to be passed to cmake.
  • A few common options are also implemented as shortcuts, and match the options from the build_cmake.sh script.

The features allow for converting about the half of the tests. More will
be converted later as features are added.

Depends on D7314.

Test Plan

Ran all the builds with the new configuration format.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
ci_configure_build_steps
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 12566
Build 25248: Build Diffbuild-diff · build-without-wallet · build-clang-tidy · build-clang-10
Build 25247: arc lint + arc unit

Event Timeline

Fabien created this revision.Sep 2 2020, 20:03
Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 2 2020, 20:03
Fabien requested review of this revision.Sep 2 2020, 20:03
teamcity added a comment.Sep 2 2020, 20:36
Build Bitcoin ABC Diffs / Diff Testing (build-clang-tidy) failed.

Snippet of first build failure:

[20:28:14]i:				 [/usr/bin/git -c credential.helper= fetch --progress origin +refs/tags/phabricator/diff/23201:refs/tags/phabricator/diff/23201] remote: Compressing objects:  80% (34/42)        
[20:28:14]i:				 [/usr/bin/git -c credential.helper= fetch --progress origin +refs/tags/phabricator/diff/23201:refs/tags/phabricator/diff/23201] remote: Compressing objects:  83% (35/42)        
[20:28:14]i:				 [/usr/bin/git -c credential.helper= fetch --progress origin +refs/tags/phabricator/diff/23201:refs/tags/phabricator/diff/23201] remote: Compressing objects:  85% (36/42)        
[20:28:14]i:				 [/usr/bin/git -c credential.helper= fetch --progress origin +refs/tags/phabricator/diff/23201:refs/tags/phabricator/diff/23201] remote: Compressing objects:  88% (37/42)        
[20:28:14]i:				 [/usr/bin/git -c credential.helper= fetch --progress origin +refs/tags/phabricator/diff/23201:refs/tags/phabricator/diff/23201] remote: Compressing objects:  90% (38/42)        
[20:28:14]i:				 [/usr/bin/git -c credential.helper= fetch --progress origin +refs/tags/phabricator/diff/23201:refs/tags/phabricator/diff/23201] remote: Compressing objects:  92% (39/42)        
[20:28:14]i:				 [/usr/bin/git -c credential.helper= fetch --progress origin +refs/tags/phabricator/diff/23201:refs/tags/phabricator/diff/23201] remote: Compressing objects:  95% (40/42)        
[20:28:14]i:				 [/usr/bin/git -c credential.helper= fetch --progress origin +refs/tags/phabricator/diff/23201:refs/tags/phabricator/diff/23201] remote: Compressing objects:  97% (41/42)        
[20:28:14]i:				 [/usr/bin/git -c credential.helper= fetch --progress origin +refs/tags/phabricator/diff/23201:refs/tags/phabricator/diff/23201] remote: Compressing objects: 100% (42/42)        
[20:28:14]i:				 [/usr/bin/git -c credential.helper= fetch --progress origin +refs/tags/phabricator/diff/23201:refs/tags/phabricator/diff/23201] remote: Compressing objects: 100% (42/42), done.        
[20:28:14]i:				 [/usr/bin/git -c credential.helper= fetch --progress origin +refs/tags/phabricator/diff/23201:refs/tags/phabricator/diff/23201] remote: Total 42 (delta 34), reused 0 (delta 0), pack-reused 0
[20:28:14]i:				 [/usr/bin/git -c credential.helper= fetch --progress origin +refs/tags/phabricator/diff/23201:refs/tags/phabricator/diff/23201] From ssh://reviews.bitcoinabc.org:2221/source/bitcoin-abc-staging
[20:28:14]i:				 [/usr/bin/git -c credential.helper= fetch --progress origin +refs/tags/phabricator/diff/23201:refs/tags/phabricator/diff/23201]  * [new tag]             phabricator/diff/23201 -> phabricator/diff/23201
[20:28:15]i:				 [/usr/bin/git -c credential.helper= fetch --progress origin +refs/tags/phabricator/diff/23201:refs/tags/phabricator/diff/23201]  * [new tag]             phabricator/base/23201 -> phabricator/base/23201
[20:28:15] :			 [Update git mirror (/home/teamcity/buildAgent/system/git/git-48AA3180.git)] /usr/bin/git log -n1 --pretty=format:%H%x20%s 99c85d5b7ffbfa86731f0e104f50956385cc6f67 --
[20:28:15] :			 [Update git mirror (/home/teamcity/buildAgent/system/git/git-48AA3180.git)] /usr/bin/git pack-refs --all
[20:28:15] :		 [VCS Root: Bitcoin ABC Staging] Update checkout directory (/home/teamcity/buildAgent/work/jailed-build/bitcoin-abc)
[20:28:15] :			 [Update checkout directory (/home/teamcity/buildAgent/work/jailed-build/bitcoin-abc)] The .git directory is missing in '/home/teamcity/buildAgent/work/jailed-build/bitcoin-abc'. Running 'git init'...
[20:28:15] :			 [Update checkout directory (/home/teamcity/buildAgent/work/jailed-build/bitcoin-abc)] /usr/bin/git init
[20:28:15] :			 [Update checkout directory (/home/teamcity/buildAgent/work/jailed-build/bitcoin-abc)] /usr/bin/git config lfs.storage /home/teamcity/buildAgent/system/git/git-48AA3180.git/lfs
[20:28:15] :			 [Update checkout directory (/home/teamcity/buildAgent/work/jailed-build/bitcoin-abc)] /usr/bin/git config core.sparseCheckout true
[20:28:15] :			 [Update checkout directory (/home/teamcity/buildAgent/work/jailed-build/bitcoin-abc)] /usr/bin/git config http.sslCAInfo
[20:28:15] :			 [Update checkout directory (/home/teamcity/buildAgent/work/jailed-build/bitcoin-abc)] /usr/bin/git show-ref
[20:28:15] :			 [Update checkout directory (/home/teamcity/buildAgent/work/jailed-build/bitcoin-abc)] /usr/bin/git show-ref refs/tags/phabricator/diff/23201
[20:28:15] :			 [Update checkout directory (/home/teamcity/buildAgent/work/jailed-build/bitcoin-abc)] /usr/bin/git log -n1 --pretty=format:%H%x20%s 99c85d5b7ffbfa86731f0e104f50956385cc6f67 --
[20:28:15] :			 [Update checkout directory (/home/teamcity/buildAgent/work/jailed-build/bitcoin-abc)] /usr/bin/git -c credential.helper= checkout -q -f phabricator/diff/23201
[20:28:15] :			 [Update checkout directory (/home/teamcity/buildAgent/work/jailed-build/bitcoin-abc)] /usr/bin/git show-ref refs/tags/phabricator/diff/23201
[20:28:15] :			 [Update checkout directory (/home/teamcity/buildAgent/work/jailed-build/bitcoin-abc)] Cleaning Bitcoin ABC Staging in /home/teamcity/buildAgent/work/jailed-build/bitcoin-abc the file set ALL_UNTRACKED
[20:28:15] :			 [Update checkout directory (/home/teamcity/buildAgent/work/jailed-build/bitcoin-abc)] /usr/bin/git clean -f -d -x
[20:28:15] : Build preparation done
[20:28:15]E: Step 1/1: Command Line (8m:06s)
[20:28:15] :	 [Step 1/1] Ant JUnit report watcher
[20:28:15] :		 [Ant JUnit report watcher] Watching paths:
[20:28:15] :		 [Ant JUnit report watcher] +:results/test_bitcoin.xml
[20:28:15] :		 [Ant JUnit report watcher] +:results/**/junit_results*.xml
[20:28:15] :	 [Step 1/1] Starting: /home/teamcity/buildAgent/temp/agentTmp/custom_script1214261218045657699
[20:28:15] :	 [Step 1/1] in directory: /home/teamcity/buildAgent/work/jailed-build
[20:28:15] :	 [Step 1/1] ~/buildAgent/work/jailed-build/bitcoin-abc ~/buildAgent/work/jailed-build
[20:28:15] :	 [Step 1/1] ~/buildAgent/work/jailed-build
[20:28:15] :	 [Step 1/1] Building base image for: 99c85d5b7...
[20:28:15] :	 [Step 1/1] ~/buildAgent/work/jailed-build/bitcoin-abc ~/buildAgent/work/jailed-build
[20:28:17] :	 [Step 1/1] ~/buildAgent/work/jailed-build
[20:28:17] :	 [Step 1/1] Tag name: abc-base-image-99c85d5b7
[20:36:18] :	 [Step 1/1] Starting build build-clang-tidy
[20:36:18]W:	 [Step 1/1] Traceback (most recent call last):
[20:36:18]W:	 [Step 1/1]   File "./contrib/teamcity/build-configurations.py", line 462, in <module>
[20:36:18]W:	 [Step 1/1]     main()
[20:36:18]W:	 [Step 1/1]   File "./contrib/teamcity/build-configurations.py", line 458, in main
[20:36:18]W:	 [Step 1/1]     sys.exit(build.run(unknown_args)[0])
[20:36:18]W:	 [Step 1/1]   File "./contrib/teamcity/build-configurations.py", line 395, in run
[20:36:18]W:	 [Step 1/1]     return_code, message = super().run()
[20:36:18]W:	 [Step 1/1]   File "./contrib/teamcity/build-configurations.py", line 348, in run
[20:36:18]W:	 [Step 1/1]     self.configuration.create_build_steps(self.artifact_dir)
[20:36:18]W:	 [Step 1/1]   File "./contrib/teamcity/build-configurations.py", line 130, in create_build_steps
[20:36:18]W:	 [Step 1/1]     str(self.script_path)
[20:36:18]W:	 [Step 1/1] FileNotFoundError: The script file . does not exist or does not have execution permission
[20:36:18]W:	 [Step 1/1] mv: missing destination file operand after '/results'
[20:36:18]W:	 [Step 1/1] Try 'mv --help' for more information.
[20:36:22]W:	 [Step 1/1] Process exited with code 1
[20:36:22]E:	 [Step 1/1] Process exited with code 1 (Step: Command Line)
Fabien planned changes to this revision.Sep 2 2020, 20:39
Fabien updated this revision to Diff 23211.Sep 2 2020, 20:52

Fix script_path no longer being an Configuration object member

Harbormaster completed remote builds in B12570: Diff 23211.Sep 2 2020, 21:19
jasonbcox requested changes to this revision.Sep 2 2020, 22:35
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
contrib/teamcity/build-configurations.json
30 ↗(On Diff #23211)

Did you intend to remove the artifacts?

30 ↗(On Diff #23211)

configuration is incredibly general. perhaps cmake_configuration? Although we are very likely to stick with cmake, we *did* just get rid of autotools, so we know cmake isn't the only configuration to provide in the universe.

contrib/teamcity/build-configurations.py
152 ↗(On Diff #23211)

Support for this was added, but it looks like it was left out of the configurations above.

This revision now requires changes to proceed.Sep 2 2020, 22:35
Fabien added inline comments.Sep 2 2020, 22:48
contrib/teamcity/build-configurations.json
30 ↗(On Diff #23211)

Yes this is intentional.
The install targets will put the files to the artifact directory, which is a better way to achieve the same result.
I submitted a lot of changes recently to the install targets and various file placements so everything goes directly to the artifact directory without further configuration.

30 ↗(On Diff #23211)

The intent is to declare it as the configuration step... maybe configuration_flags is more accurate.

contrib/teamcity/build-configurations.py
152 ↗(On Diff #23211)

If you look at the default, it is enabled by default. This should always be enabled on CI, unless you have a good reason not to do so.

Fabien edited the summary of this revision. (Show Details)Sep 3 2020, 08:04
Fabien updated this revision to Diff 23229.Sep 3 2020, 08:07

Rename configuration to cmake_flags

Harbormaster completed remote builds in B12582: Diff 23229.Sep 3 2020, 08:23
teamcity added a comment.Sep 3 2020, 08:24
Build Bitcoin ABC Diffs / Diff Testing (build-diff) failed.

Snippet of first build failure:

wallet_create_tx.py                     | ✓ Passed  | 8 s
wallet_createwallet.py                  | ✓ Passed  | 4 s
wallet_createwallet.py --usecli         | ✓ Passed  | 4 s
wallet_disable.py                       | ✓ Passed  | 1 s
wallet_dump.py                          | ✓ Passed  | 4 s
wallet_encryption.py                    | ✓ Passed  | 5 s
wallet_groups.py                        | ✓ Passed  | 16 s
wallet_hd.py                            | ✓ Passed  | 5 s
wallet_import_rescan.py                 | ✓ Passed  | 6 s
wallet_import_with_label.py             | ✓ Passed  | 1 s
wallet_importmulti.py                   | ✓ Passed  | 5 s
wallet_importprunedfunds.py             | ✓ Passed  | 2 s
wallet_keypool.py                       | ✓ Passed  | 3 s
wallet_keypool_topup.py                 | ✓ Passed  | 3 s
wallet_labels.py                        | ✓ Passed  | 2 s
wallet_listreceivedby.py                | ✓ Passed  | 10 s
wallet_listsinceblock.py                | ✓ Passed  | 4 s
wallet_listtransactions.py              | ✓ Passed  | 13 s
wallet_multiwallet.py                   | ✓ Passed  | 15 s
wallet_reorgsrestore.py                 | ✓ Passed  | 4 s
wallet_resendwallettransactions.py      | ✓ Passed  | 12 s
wallet_txn_clone.py                     | ✓ Passed  | 3 s
wallet_txn_clone.py --mineblock         | ✓ Passed  | 3 s
wallet_txn_doublespend.py               | ✓ Passed  | 3 s
wallet_txn_doublespend.py --mineblock   | ✓ Passed  | 4 s
wallet_watchonly.py                     | ✓ Passed  | 1 s
wallet_watchonly.py --usecli            | ✓ Passed  | 1 s
wallet_zapwallettxes.py                 | ✓ Passed  | 4 s
wallet_multiwallet.py --usecli          | ✖ Failed  | 13 s

ALL                                     | ✖ Failed  | 721 s (accumulated) 
Runtime: 145 s

[237/722] cd /work/contrib/devtools/chainparams && /usr/bin/python3.7 ./test_make_chainparams.py
.....
----------------------------------------------------------------------
Ran 5 tests in 0.001s

OK
[256/722] Running seeder test suite
PASSED: seeder test suite
[272/722] Running pow test suite
PASSED: pow test suite
[274/722] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/script_tests.cpp.o
In file included from /usr/include/boost/test/unit_test.hpp:19,
                 from ../../src/test/script_tests.cpp:30:
../../src/test/script_tests.cpp: In member function ‘void script_tests::script_build::test_method()’:
../../src/test/script_tests.cpp:541:22: note: variable tracking size limit exceeded with -fvar-tracking-assignments, retrying without
 BOOST_AUTO_TEST_CASE(script_build) {
                      ^~~~~~~~~~~~
[715/722] Running bitcoin test suite
PASSED: bitcoin test suite
[717/722] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[720/722] Running bitcoin-upgrade-activated test suite
PASSED: bitcoin-upgrade-activated test suite
FAILED: test/CMakeFiles/check-functional 
cd /work/abc-ci-builds/build-diff/test && /usr/bin/cmake -E make_directory /work/abc-ci-builds/build-diff/test/junit && /usr/bin/cmake -E make_directory /work/abc-ci-builds/build-diff/test/log && /usr/bin/cmake -E env /usr/bin/python3.7 ./functional/test_runner.py "--testsuitename=Bitcoin ABC functional tests" --junitoutput=/work/abc-ci-builds/build-diff/test/junit/functional_tests.xml
ninja: build stopped: subcommand failed.
Build build-diff failed with exit code 1

Each failure log is accessible here:
Bitcoin ABC functional tests: wallet_multiwallet.py --usecli

deadalnix requested changes to this revision.Sep 3 2020, 18:05
deadalnix added a subscriber: deadalnix.

Figure out why the test suite is broken.

This revision now requires changes to proceed.Sep 3 2020, 18:05
Fabien requested review of this revision.Sep 3 2020, 18:41

The failure is unrelated (I was wondering why the test shows green despite the failure, it appears that Jason restarted the build after it failed).
I'll see if I can reproduce but that should not block this diff.

Edit: talked with Jason the be sure and the failure is already under radar, see D7340.

jasonbcox added inline comments.Sep 3 2020, 19:14
contrib/teamcity/build-configurations.py
145 ↗(On Diff #23229)

It doesn't look like this default behavior is useful (it's not used). It would be more informative to have an error if neither script nor targets are set. I think it's more likely that the config options are forgotten to be set than for ninja all to be the only intended target. Let me know if you disagree or foresee a use case that I'm missing.

Fabien added inline comments.Sep 3 2020, 19:33
contrib/teamcity/build-configurations.py
145 ↗(On Diff #23229)

The intent was only to provide a safe default path, but I think you're right and an error message can be more appropriated here. If all is really the only target to run the it can simply be set in the config.

Fabien updated this revision to Diff 23256.Sep 3 2020, 19:44

Raise an error if there is nothing to run.

Harbormaster completed remote builds in B12601: Diff 23256.Sep 3 2020, 20:19
deadalnix accepted this revision.Sep 3 2020, 21:39
deadalnix added inline comments.
contrib/teamcity/build-configurations.json
20 ↗(On Diff #23256)

Out of scope, but you should consider moving this to a file format that is less syntax heavy than json, such as yaml.

jasonbcox accepted this revision.Sep 4 2020, 00:37
This revision is now accepted and ready to land.Sep 4 2020, 00:37
jasonbcox added a child revision: D7353: Add check-source-control-tools build config.Sep 4 2020, 01:55
Fabien added inline comments.Sep 4 2020, 06:32
contrib/teamcity/build-configurations.json
20 ↗(On Diff #23256)

I've already done it, as well as its linter. I waited for this one to complete before submitting to avoid a rebase hell, due to possible syntax changes.

Closed by commit rABC07debf4d3549: [CI] Allow for describing the build from the configuration file (authored by Fabien). · Explain WhySep 4 2020, 06:49
This revision was automatically updated to reflect the committed changes.
Fabien added a commit: rABC07debf4d3549: [CI] Allow for describing the build from the configuration file.

Revision Contents
Changeset List

Diff 23201

View Options

contrib/teamcity/build-configurations.json

Loading...
View Options

contrib/teamcity/build-configurations.py

Loading...
View Options

contrib/teamcity/builds/build-asan.sh

Loading...
View Options

contrib/teamcity/builds/build-bench.sh

Loading...
View Options

contrib/teamcity/builds/build-clang-10.sh

Loading...
View Options

contrib/teamcity/builds/build-diff.sh

Loading...
View Options

contrib/teamcity/builds/build-master.sh

Loading...
View Options

contrib/teamcity/builds/build-tsan.sh

Loading...
View Options

contrib/teamcity/builds/build-ubsan.sh

Loading...
View Options

contrib/teamcity/builds/build-without-cli.sh

Loading...
View Options

contrib/teamcity/builds/build-without-wallet.sh

Loading...
View Options

contrib/teamcity/builds/build-without-zmq.sh

Loading...