Details

Reviewers

Group Reviewers

Restricted Project

Commits

rABCf0323812f55b: [CMAKE] Run the leveldb tests serially

Summary

This adds a facility to create a job pool with an arbitrary number of
concurrent jobs and assign it to a test suite. This is used to limit the
leveldb suite to a single job and make the tests run serially.

This adds about 20% to the leveldb tests duration on my machine, but
since they are only running as part of the check-all (or higher) global
target the impact is minor.

This is expected to prevent spurious leveldb failures like
https://build.bitcoinabc.org/viewLog.html?tab=buildLog&buildTypeId=BitcoinABC_Master_BitcoinAbcMasterCoverage&buildId=87936&guest=1

Depends on D7073.

Test Plan

ninja all check-all
ninja check-leveldb

Check it still works with make, even if the pool has no effect:

cmake ..
make -j42 check-leveldb

Diff Detail

Repository

rABC Bitcoin ABC

Branch

cmake_tests_no_parallel

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 12242
Build 24702: Default Diff Build & Tests	build-diff
Build 24701: Build without Wallet	build-without-wallet
Build 24700: Build with clang-10	build-clang-10
Build 24699: arc lint + arc unit

Event Timeline

Fabien created this revision.Jun 26 2020, 19:10

Herald added a reviewer: Restricted Project. · View Herald TranscriptJun 26 2020, 19:10

Fabien requested review of this revision.Jun 26 2020, 19:10

You can create a specific job pool for this and set the job count to 1 rather than simply use terminal.

This revision now requires changes to proceed.Jun 26 2020, 19:14

Also: https://reviews.llvm.org/D6304

Setting the job pool for custom targets is only possible with cmake >= 3.15 and we enforce cmake >= 3.13 (debian 10 default): https://gitlab.kitware.com/cmake/cmake/-/merge_requests/3308.
Note that due to the wrapper, USES_TERMINAL will not dump the test to the console unless there is a failure. It's still not equivalent to a dedicated pool but close enough.

Harbormaster completed remote builds in B11740: Diff 21808.Jun 26 2020, 19:21

The difference is very relevent because this adds to the serial path for no good reasons.

This revision now requires changes to proceed.Jun 27 2020, 12:37

Fabien mentioned this in D7073: [CMAKE] Bump minimum cmake to 3.16.Jul 29 2020, 12:49

Use a JOB_POOL mechanism to limit the number of concurrent jobs.

Fabien edited the summary of this revision. (Show Details)Jul 29 2020, 13:06

Fabien edited the test plan for this revision. (Show Details)

Fabien added a parent revision: D7073: [CMAKE] Bump minimum cmake to 3.16.

Harbormaster completed remote builds in B12216: Diff 22603.Jul 29 2020, 13:31

deadalnix requested changes to this revision.Jul 30 2020, 00:30

deadalnix added inline comments.

cmake/modules/TestSuite.cmake
89 ↗	(On Diff #22603)	BELONG_TO_POOL is a boolean name, either it belongs or not. But this is not a boolean.

This revision now requires changes to proceed.Jul 30 2020, 00:30

Pick a better name for BELONG_TO_POOL

Harbormaster completed remote builds in B12242: Diff 22638.Jul 30 2020, 10:07

deadalnix accepted this revision.Jul 30 2020, 12:22

deadalnix added inline comments.

cmake/modules/TestSuite.cmake
101	This is not a parameter, but an argument. Parameters are declarations. int foo(int a) { ... } foo(3); In here, a is a parameter, 3 is an argument. `cmake_parse_arguments` does parse the arguments. what you give is is a description of the parameters.