Page MenuHomePhabricator

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

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

Details

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 requested review of this revision.Sep 2 2020, 20:03

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

Fix script_path no longer being an Configuration object member

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

Rename configuration to cmake_flags

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.

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.

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.

Raise an error if there is nothing to run.

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.

This revision is now accepted and ready to land.Sep 4 2020, 00:37
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.