Repository navigation
ci: pin detect_leaks=1 on the asan test runs - #146
Open
tesseract-ripple wants to merge 1 commit into
Open
tesseract-ripple wants to merge 1 commit into
tesseract-ripple wants to merge 1 commit into
Conversation
The .asan matrix cells already catch leaks, because LeakSanitizer is part of ASan on Linux and defaults to on, but nothing in the repo says so. Setting ASAN_OPTIONS explicitly makes the TOB-RIPCTXR-3 leak regression coverage a stated property rather than an inherited default, so it cannot quietly disappear with a profile or container change. Gated on the .asan cells: Darwin ASan ships no LeakSanitizer and rejects the option, and the macOS matrix entries set no sanitizer_ext. Refs XRPLF#121.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
#121 asks for an ASan CI target with leak detection so the TOB-RIPCTXR-3 fix (#78) has regression coverage. Most of that turned out to already exist, just not the way the issue proposed: the build-and-test matrix carries .asan, .tsan and .ubsan cells via the conan profiles (build.yml:27, CONAN_PROFILE at :78), and ctest runs inside each of them, so ASan is exercised on gcc and clang, Debug and Release, x86_64 and arm64. I confirmed those cells run and pass on the last green main run.
What is genuinely missing is smaller. Leak detection there is inherited rather than declared: LeakSanitizer is part of ASan on Linux and defaults to on, so leaks do fail today, but nothing in the repo states it. If a profile or container-image change ever turned it off, leak coverage would disappear with nothing failing to announce it, which is the same silent-failure shape the issue is worried about. This sets ASAN_OPTIONS=detect_leaks=1 on the test step so the guarantee is explicit.
It is gated to the .asan cells on purpose. Darwin ASan ships no LeakSanitizer and errors on the option, and the macOS matrix entries set no sanitizer_ext, so the expression yields an empty value there.
I would suggest #121 gets narrowed or closed on the back of this rather than kept open for a MPT_CRYPTO_SANITIZE_ADDRESS CMake option, since the profile route already covers it and a second mechanism would just be another thing to keep in step. Happy to be told otherwise.
Testing
No behaviour change to verify locally, and it is a no-op on today's runners by construction. I checked the workflow still parses and that the env lands on the right step. CI on this PR exercises it, though note the matrix is skipped on forks (build.yml:20), so the asan cells will only really run once this is on an XRPLF branch.