Page MenuHomePhabricator

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

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

Details

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 10164
Build 18154: Default Diff Build & Tests
Build 18153: arc lint + arc unit

Event Timeline

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 ↗(On Diff #18706)

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
cmake/modules/TestSuite.cmake
14 ↗(On Diff #18706)

Extract the commands, prepend the environment variables, forward everything. Just like is done for the depfile.

cmake/modules/TestSuite.cmake
13 ↗(On Diff #18706)

Why add verbatim? Does this need to be added to the depfile thing as well?

cmake/modules/Sanitizers.cmake
50 ↗(On Diff #18706)

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 ↗(On Diff #18706)

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 ↗(On Diff #18706)

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.

deadalnix requested changes to this revision.Apr 10 2020, 13:08
deadalnix added inline comments.
This revision now requires changes to proceed.Apr 10 2020, 13:08
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.

Run the test command last, as additional commands are more likely to be useful before it.

cmake/modules/TestSuite.cmake
16 ↗(On Diff #18722)

You're too fast, see https://reviews.bitcoinabc.org/D5689?id=18722#inline-35330 for the rationale.

This revision is now accepted and ready to land.Apr 10 2020, 13:33