Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add SANITIZE cmake option and ASAN github action #3045

Merged
merged 6 commits into from
Nov 3, 2022
Merged

Add SANITIZE cmake option and ASAN github action #3045

merged 6 commits into from
Nov 3, 2022

Conversation

rsanchez15
Copy link
Contributor

@rsanchez15 rsanchez15 commented Oct 26, 2022 •
edited
Loading

Description

Contributor Checklist

  • Commit messages follow the project guidelines.
  • The code follows the style guidelines of this project.
  • Tests that thoroughly check the new feature have been added/Regression tests checking the bug and its fix have been added.
  • Any new/modified methods have been properly documented using Doxygen.
  • Fast DDS test suite has been run locally.
  • Changes are ABI compatible.
  • Changes are API compatible.
  • Documentation builds and tests pass locally.
  • New feature has been added to the versions.md file (if applicable).
  • New feature has been documented/Current behavior is correctly described in the documentation.

Reviewer Checklist

  • Check contributor checklist is correct.
  • Check CI results: changes do not issue any warning.
  • Check CI results: failing tests are unrelated with the changes.

@rsanchez15 rsanchez15 marked this pull request as draft October 26, 2022 13:10
@rsanchez15 rsanchez15 force-pushed the feature/asan-ci branch 4 times, most recently from 8380caf to 04c1884 Compare October 26, 2022 15:31
@rsanchez15
Add asan build option and create asan github action
3cf463e 
Signed-off-by: RaulSanchez <raul@eprosima.com>
@jparisu jparisu mentioned this pull request Oct 27, 2022
Closed
@rsanchez15
Improve sanitizer cmake option to cover address and thread sanitizers
af36bfa 
Signed-off-by: RaulSanchez <raul@eprosima.com>
@rsanchez15 rsanchez15 changed the title [WIP] Add ASAN build option and create ASAN github action [WIP] Add SANITIZE cmake option and ASAN github action Oct 27, 2022
@rsanchez15 rsanchez15 marked this pull request as ready for review October 27, 2022 12:57
@rsanchez15 rsanchez15 changed the title [WIP] Add SANITIZE cmake option and ASAN github action Add SANITIZE cmake option and ASAN github action Oct 27, 2022
@rsanchez15
List all failed tests
8647014 
Signed-off-by: RaulSanchez <raul@eprosima.com>
jparisu
jparisu suggested changes Nov 2, 2022
View reviewed changes
.github/workflows/asan.yml Outdated Show resolved Hide resolved
.github/workflows/asan.yml Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
@rsanchez15
Avoid running flaky tests
a6428b4 
Signed-off-by: RaulSanchez <raul@eprosima.com>
@rsanchez15
Add instrumentation not supported warning
7e9459c 
Signed-off-by: RaulSanchez <raul@eprosima.com>
jparisu
jparisu previously approved these changes Nov 2, 2022
View reviewed changes
Copy link
Contributor

@jparisu jparisu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with green CI (without ASAN errors)

@rsanchez15
Copy link
Contributor Author

LGTM with green CI (without ASAN errors)

There will be ASAN errors until all the other PRs are merged, so I'll merge this first to start checking ASAN errors ASAP.

@EduPonz EduPonz added no-test Skip CI tests if PR marked with this label no-aarch Skip build & test for aarch64 labels Nov 2, 2022
@EduPonz
Copy link

EduPonz commented Nov 2, 2022

@richiprosima please test this

@rsanchez15
Removed duplicated action
0a7d589 
Signed-off-by: RaulSanchez <raul@eprosima.com>
@EduPonz
Copy link

EduPonz commented Nov 3, 2022

@richiprosima please test this

@EduPonz EduPonz added this to the v2.8.1 milestone Nov 3, 2022
@rsanchez15
Copy link
Contributor Author

Documentation PR:

JLBuenoLopez
JLBuenoLopez approved these changes Nov 3, 2022
View reviewed changes
Copy link
Contributor

@JLBuenoLopez JLBuenoLopez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@MiguelCompany MiguelCompany merged commit 118dda9 into master Nov 3, 2022
@MiguelCompany MiguelCompany deleted the feature/asan-ci branch November 3, 2022 11:49
@mjcarroll mjcarroll mentioned this pull request Nov 3, 2022
Merged
@clalancette
Copy link
Contributor

FYI, This is causing CMake errors in our ROS 2 CI builds: https://ci.ros2.org/job/ci_linux/17584/cmake/

@mjcarroll
Copy link

As a note, this pull request introduces a CMake warning in the case that sanitizers aren't being used. This caused our CI to report the warnings here: https://ci.ros2.org/job/ci_windows/18152/

I think in this case WARNING may be a bit strong, and it should fail more quietly.

@methylDragon methylDragon mentioned this pull request Nov 3, 2022
Merged
@JLBuenoLopez
Copy link
Contributor

Solve in #3060 that has just been merged. You should not see the warning anymore. Thanks for your report and sorry for the inconvenience.

@clalancette
Copy link
Contributor

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EduPonz EduPonz EduPonz approved these changes

@JLBuenoLopez JLBuenoLopez JLBuenoLopez approved these changes

@jparisu jparisu jparisu left review comments

Assignees
No one assigned
Labels
no-aarch Skip build & test for aarch64 no-test Skip CI tests if PR marked with this label
Projects
None yet
Milestone
v2.8.1
Development

Successfully merging this pull request may close these issues.
7 participants
@rsanchez15 @EduPonz @clalancette @mjcarroll @JLBuenoLopez @jparisu @MiguelCompany