chore: Do not duplicate sanitizer flags - #7058
Conversation
There was a problem hiding this comment.
Pull request overview
This PR centralizes sanitizer flag construction in the Conan sanitizers profile and simplifies the CMake sanitizer module to only apply those precomputed flags, avoiding duplicated logic between Conan and CMake.
Changes:
- Refactors
conan/profiles/sanitizersto compute sanitizer compiler/linker flags and inject them into CMake via Conan toolchainextra_variables. - Simplifies
cmake/XrplSanitizers.cmaketo consume the injected variables, apply them to thecommoninterface target, and disable mold/gold/lld when using GCC + sanitizers. - Updates cspell wordlist to allow
cmaketoolchain.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| cspell.config.yaml | Adds cmaketoolchain to the spelling allowlist. |
| conan/profiles/sanitizers | Moves sanitizer parsing/flag construction into Conan and exports flags to CMake via toolchain variables. |
| cmake/XrplSanitizers.cmake | Removes sanitizer parsing/building logic and applies Conan-provided flags to common. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
This PR refactors sanitizer configuration so that sanitizer parsing/validation and flag construction happen in the Conan sanitizers profile, and CMake primarily consumes the precomputed flags (plus adds the Clang ignorelist and GCC linker constraints).
Changes:
- Add
cmaketoolchainto the cspell dictionary. - Rework
conan/profiles/sanitizersto validateSANITIZERS, build canonical compiler/linker flags, and inject them into the CMake toolchain viaextra_variables. - Simplify
cmake/XrplSanitizers.cmaketo apply Conan-provided flags and keep Clang’s-fsanitize-ignorelistbehavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cspell.config.yaml | Adds a new allowed word used in build tooling context. |
| conan/profiles/sanitizers | Centralizes sanitizer parsing/validation and constructs flags/defines for Conan + passes flags into CMake toolchain variables. |
| cmake/XrplSanitizers.cmake | Stops duplicating sanitizer parsing and instead applies Conan-provided flags, while still adding Clang ignorelist and GCC linker restrictions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1867200 to
9a6b2f4
Compare
There was a problem hiding this comment.
Pull request overview
This PR centralizes sanitizer parsing/validation and flag generation in the Conan sanitizers profile, and updates the CMake sanitizer module to consume those precomputed flags instead of duplicating logic.
Changes:
- Refactors
conan/profiles/sanitizersto parse/validateSANITIZERS, construct compiler/linker flags, and inject them into the Conan CMake toolchain viaextra_variables. - Simplifies
cmake/XrplSanitizers.cmaketo apply the injected flags and retain Clang’s ignorelist handling. - Adds
cmaketoolchainto the cspell dictionary.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cspell.config.yaml | Adds a new allowed word for spelling checks. |
| conan/profiles/sanitizers | Consolidates sanitizer selection/validation and exports computed flags/variables to CMake toolchain. |
| cmake/XrplSanitizers.cmake | Applies Conan-injected sanitizer flags and keeps Clang ignorelist + GCC linker disabling logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Refactors sanitizer build configuration so sanitizer selection/validation and flag construction are centralized in the Conan sanitizers profile, with CMake consuming the resulting flags (instead of re-deriving them), and updates docs/CI accordingly.
Changes:
- Move sanitizer parsing/validation and compiler/link flag construction into
conan/profiles/sanitizers, exporting flags to CMake via toolchainextra_variables. - Simplify
cmake/XrplSanitizers.cmaketo consume Conan-provided flags and keep only CMake-specific behavior (e.g., Clang ignorelist, GCC linker disabling, runtime reporting macro). - Update sanitizer build documentation and remove redundant
SANITIZERSenv propagation in the reusable CI workflow.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/build/sanitizers.md | Updates sanitizer build steps to align with Conan-driven configuration. |
| cspell.config.yaml | Adds a spelling allowlist entry for cmaketoolchain. |
| conan/profiles/sanitizers | Centralizes sanitizer parsing/validation and constructs compiler/linker flags; exports flags into CMake toolchain variables. |
| cmake/XrplSanitizers.cmake | Consumes Conan-injected sanitizer variables; keeps Clang ignorelist and GCC linker constraints. |
| BUILD.md | Updates sanitizer instructions to emphasize setting SANITIZERS during conan install. |
| .github/workflows/reusable-build-test-config.yml | Stops passing SANITIZERS to the CMake configure step (Conan toolchain now supplies needed values). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 3. Use `--profile:all sanitizers` with Conan to build dependencies with sanitizer instrumentation. [!NOTE]Building with sanitizer-instrumented dependencies is slower but produces fewer false positives. | ||
| 4. Set `ASAN_OPTIONS`, `LSAN_OPTIONS`, `UBSAN_OPTIONS` and `TSAN_OPTIONS` environment variables to configure sanitizer behavior when running executables. [More details below](#running-tests-with-sanitizers). |
There was a problem hiding this comment.
Keeping this as is for now, will add an option to only build rippled with sanitizer and dependencies without sanitizers back in another PR, because it will require separating profile in 2 different profiles
bthomee
left a comment
bthomee
left a comment
There was a problem hiding this comment.
Looking good so far! I'll make another pass over it later.
There was a problem hiding this comment.
Pull request overview
This PR centralizes sanitizer parsing/validation and flag construction in the Conan sanitizers profile, and updates CMake to consume the resulting flags rather than re-deriving them, reducing duplicated build logic across Conan and CMake.
Changes:
- Move sanitizer parsing/validation + compiler/linker flag construction into
conan/profiles/sanitizers, and pass the computed values to CMake viaCMakeToolchainextra_variables. - Simplify
cmake/XrplSanitizers.cmaketo apply the injected flags (and keep Clang ignorelist + GCC linker restrictions). - Update build documentation and CI workflow to reflect that
SANITIZERSonly needs to be provided to Conan (not CMake), plus a small cspell dictionary update.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
docs/build/sanitizers.md |
Updates sanitizer build steps to reflect Conan-driven configuration. |
cspell.config.yaml |
Adds cmaketoolchain to the dictionary. |
conan/profiles/sanitizers |
Centralizes sanitizer validation and constructs flags; injects them into CMake via toolchain extra_variables. |
cmake/XrplSanitizers.cmake |
Applies Conan-injected sanitizer flags; retains Clang ignorelist and GCC linker disabling. |
BUILD.md |
Updates sanitizer instructions to match the new Conan-only environment variable requirement. |
.github/workflows/reusable-build-test-config.yml |
Removes passing SANITIZERS to the CMake configure step (still provided to Conan steps). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #7058 +/- ##
=========================================
- Coverage 82.1% 82.1% -0.0%
=========================================
Files 1010 1010
Lines 76033 76033
Branches 7369 7374 +5
=========================================
- Hits 62413 62400 -13
- Misses 13620 13633 +13 see 6 files with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| To build dependencies and xrpld with sanitizer instrumentation, set the | ||
| `SANITIZERS` environment variable (only once before running conan and cmake) and use the `sanitizers` profile in conan: | ||
| `SANITIZERS` environment variable when running `conan install` and use the `sanitizers` profile: |
There was a problem hiding this comment.
CMake files still read SANITIZERS env. variable. So if a dev. sets this flag to one value before running conan install then sets it again before running cmake, there will be a conflict.
There was a problem hiding this comment.
Where do CMake files still read it?
There was a problem hiding this comment.
Used in cmake/XrplSanitizers.cmake, but where is SANITIZERS set?
There was a problem hiding this comment.
There is no reading of env variable SANITIZERS in this CMake file (or I haven't found it).
cmake variable SANITIZERS comes from using tools.cmake.cmaketoolchain:extra_variables conan option
So, unless someone does -DSANITIZERS (and we never had this way of passing sanitizers), so it's highly unlikely to happen, this variable will be the same as conan got.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {% else %} | ||
|
|
||
| {% if compiler == "msvc" %} | ||
| {{ "Sanitizers are not supported on Windows/MSVC. Please unset the SANITIZERS environment variable." }} |
There was a problem hiding this comment.
Is there a way to skip rendering rest of the profile and exit here if compiler is msvc? Else just check if compiler is clang or gcc.
There was a problem hiding this comment.
This is a way to actually error and return with this message in profiles during conan install. So, it won't render the rest of the profile
| separate_arguments( | ||
| sanitizers_compiler_flags | ||
| UNIX_COMMAND | ||
| "${SANITIZERS_COMPILER_FLAGS}" |
There was a problem hiding this comment.
Do we need to validate SANITIZERS_* flags here? In case user runs conan with one set of flags and cmake (in a later session) with another, but cache is still holding old flags?
There was a problem hiding this comment.
I tested it locally, using the same build dir:
- Ran conan with UB sanitizer
- Ran cmake
- Ran conan with Address sanitizer
- Ran cmake
The CMake output was proper both times.
pratikmankawde
left a comment
pratikmankawde
left a comment
There was a problem hiding this comment.
Added a-few minor comments otherwise looks good.
mathbunnyru
added
the
Ready to merge
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
High Level Overview of Change
Instead of duplicating logic between conan and cmake, we only do each thing once.
Most of things are done in the conan profile, since they affect both dependencies and rippled.
The things which only affect rippled are done in cmake.
For now, I'll remove an ability to build dependencies without sanitizers, but rippled with them, but I'll add it back later - it would require having 2 profiles, and I don't want to create a mess here.
Conan dependencies will recompile, because the order of flag changed.
Context of Change
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)