[CMAKE] Set environment variables when running tests with sanitizers
ClosedPublic
Actions

Authored by Fabien on Apr 9 2020, 15:27.

Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Commits

rSTAGINGbbb9e4b48958: [CMAKE] Set environment variables when running tests with sanitizers
rABCbbb9e4b48958: [CMAKE] Set environment variables when running tests with sanitizers

Summary

This will allow to run the tests with sanitizers settings (such as
suppression files and various options) automatically set.

Test Plan

cmake -GNinja .. -DENABLE_SANITIZERS=undefined # Use any sanitizer
ninja check check-functional

cmake .. -DENABLE_SANITIZERS=undefined # Use any sanitizer
make -j42 check check-functional

Diff Detail

Repository

rABC Bitcoin ABC

Branch

cmake_sanitizers_suppression

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 10153
Build 18132: Default Diff Build & Tests
Build 18131: arc lint + arc unit

Event Timeline

Fabien created this revision.Apr 9 2020, 15:27

Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 9 2020, 15:28

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B10153: Diff 18706.Apr 9 2020, 15:39

deadalnix requested changes to this revision.Apr 9 2020, 16:52

deadalnix added a subscriber: deadalnix.

deadalnix added inline comments.

cmake/modules/Sanitizers.cmake
50	you can do `FOO=bar echo $FOO`, which is better because you don't change global state accross commands - which is bound to do something weird at some point down the road.

This revision now requires changes to proceed.Apr 9 2020, 16:52

deadalnix added inline comments.Apr 9 2020, 16:53

cmake/modules/TestSuite.cmake
14	Extract the commands, prepend the environment variables, forward everything. Just like is done for the depfile.

deadalnix added inline comments.Apr 9 2020, 16:55

cmake/modules/TestSuite.cmake
13	Why add verbatim? Does this need to be added to the depfile thing as well?

Fabien added inline comments.Apr 10 2020, 11:34

cmake/modules/Sanitizers.cmake
50	Ninja runs the custom commands (from cmake custom commands or custom targets) in a subshell so it's not an issue. But I checked and it won't work with make (I don't know what make does, but the exported variables are not set in the test environment). "Good" news is that it doesn't leak either.
cmake/modules/TestSuite.cmake
13	I'm adding VERBATIM because I'm adding commands, so I'd better have proper escaping. You don't really need to add it for the depfiles since all you're adding is a property. Also you're already forwarding VERBATIM from the caller so it's not a issue.
14	The issue here is that you can't easily extract the commands, because it's a multi keyword multi arguments inputs. That would require to either redefine the interface with a list of commands, something like COMMAND1, COMMAND2, etc... or redo the whole implementaion of the add_custom_target function argument parsing.

Change the API, make it environment variable only to take advantage of the cmake -E env command that works for (supposedly, at least ninja and make) all generators.

This is adding a limitation to the custom target as a single command can use these environments.
This is not limitant atm and can always be extended if needed in the future.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

Fabien edited the test plan for this revision. (Show Details)Apr 10 2020, 13:05

deadalnix requested changes to this revision.Apr 10 2020, 13:08

deadalnix added inline comments.

cmake/modules/TestSuite.cmake
16 ↗	(On Diff #18722)	Use something similar to https://reviews.bitcoinabc.org/source/bitcoin-abc/browse/master/cmake/modules/CustomCommandWithDepFile.cmake

This revision now requires changes to proceed.Apr 10 2020, 13:08

Fabien added inline comments.Apr 10 2020, 13:09

cmake/modules/TestSuite.cmake
59 ↗	(On Diff #18722)	Note to reviewers: the `CUSTOM_TARGET_ARGS` is necessary for the parsing to know when the command ends. Otherwise the `TEST_COMMAND` would be required to always be the last argument.