disable sanitizer by default #517
Conversation
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
There was a problem hiding this comment.
The security risks are not worth enabling this test by default.
I wonder however if it would be more intuitive with this change to turn around the logic and have it called ENABLE_SANITIZERS and set it default to OFF. That would make it more explicit if you wanted to enable them locally by calling it with -DENABLE_SANITIZERS=ON.
I guess this could be tabled for another PR though.
|
If you are fine with renaming the option I am happy to update the pull request. I just didn't want to change API. |
|
The 7 test failures in the Rolling PR build seem to be expected. The latest dev build has the same 7 test failures + the |
|
Can this be backported to dashing, eloquent and foxy as well? The same failures are happening in CI for those:
|
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Related to #57 (comment)
Atm the test passes but the sanitizer fails to run within the CI builds inside Docker: see
I didn't find a way to conditionally enable the sanitizer and therefore this patch changes the default turning the sanitizer off. Since the CI builds can't run the leak sanitizer atm (see ros-infrastructure/ros_buildfarm#832) this only affect local builds.