chore: Do not duplicate sanitizer flags by mathbunnyru · Pull Request #7058 · XRPLF/rippled

mathbunnyru · 2026-04-30T16:41:33Z

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

Public API: New feature (new methods and/or new fields)
Public API: Breaking change (in general, breaking changes should only impact the next api_version)
libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
Peer protocol change (must be backward compatible or bump the peer protocol version)

Copilot

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/sanitizers to compute sanitizer compiler/linker flags and inject them into CMake via Conan toolchain extra_variables.
Simplifies cmake/XrplSanitizers.cmake to consume the injected variables, apply them to the common interface 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.

Copilot

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 cmaketoolchain to the cspell dictionary.
Rework conan/profiles/sanitizers to validate SANITIZERS, build canonical compiler/linker flags, and inject them into the CMake toolchain via extra_variables.
Simplify cmake/XrplSanitizers.cmake to apply Conan-provided flags and keep Clang’s -fsanitize-ignorelist behavior.

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.

Copilot

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/sanitizers to parse/validate SANITIZERS, construct compiler/linker flags, and inject them into the Conan CMake toolchain via extra_variables.
Simplifies cmake/XrplSanitizers.cmake to apply the injected flags and retain Clang’s ignorelist handling.
Adds cmaketoolchain to 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.

Copilot

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 toolchain extra_variables.
Simplify cmake/XrplSanitizers.cmake to 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 SANITIZERS env 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.

mathbunnyru · 2026-04-30T17:52:51Z

+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).


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

Looking good so far! I'll make another pass over it later.

Copilot

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 via CMakeToolchain extra_variables.
Simplify cmake/XrplSanitizers.cmake to apply the injected flags (and keep Clang ignorelist + GCC linker restrictions).
Update build documentation and CI workflow to reflect that SANITIZERS only 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 · 2026-04-30T18:41:35Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.1%. Comparing base (d050073) to head (c7aca84).

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #7058     +/-   ##
=========================================
- Coverage     82.1%   82.1%   -0.0%     
=========================================
  Files         1010    1010             
  Lines        76033   76033             
  Branches      7369    7369             
=========================================
- Hits         62418   62411      -7     
- Misses       13615   13622      +7

see 2 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

bthomee · 2026-05-01T15:20:04Z

    set(SANITIZERS_ENABLED FALSE)
    return()
 endif()
+set(SANITIZERS_ENABLED TRUE)


Do we actually need this flag? It's only used in XrplCompiler.cmake as:

$<$<AND:$<BOOL:${static}>,$<NOT:$<BOOL:${APPLE}>>,$<NOT:$<BOOL:${SANITIZERS_ENABLED}>>>:

Is it possible to use SANITIZERS directly without introducing SANITIZERS_ENABLED?

It is possible, but I didn't want to change the interaction between this cmake file and others.
And I don't feel it's beneficial, because it's not code duplication, so unlikely to cause any problems.

bthomee · 2026-05-02T14:56:45Z

-            tools.build:sharedlinkflags+=['{{sanitizer_flags}}']
-            tools.build:exelinkflags+=['{{sanitizer_flags}}']
-            tools.build:defines+={{defines}}
+{% if not sanitizers %}


Out of curiosity, what's the benefit of performing the sanity checks here as opposed to doing them in the cmake files?

In similar vein, why are the enable_(a|t|ub)san options set in the cmake file, as well as here?

When we do it here:

We do it earlier (Conan is run first)

Don't build not-working configurations "somehow" for dependencies to just then fail when building rippled

We don't duplicate and automatically get sanitized variables in CMake

I do set enable_-like options in cmake, because I want these to be pure cmake boolean variables, and I don't think it's possible to pass anything but strings.
So, it's a very-very minor duplication, which won't change in the future, but allows us to be sure that CMake code uses the correct types.

Copilot

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.

Copilot AI review requested due to automatic review settings April 30, 2026 16:41

Copilot started reviewing on behalf of mathbunnyru April 30, 2026 16:42 View session

Copilot AI reviewed Apr 30, 2026

View reviewed changes

Comment thread conan/profiles/sanitizers

Comment thread cmake/XrplSanitizers.cmake Outdated

Comment thread cmake/XrplSanitizers.cmake

Comment thread conan/profiles/sanitizers Outdated

mathbunnyru force-pushed the san_flags branch from df7f551 to 8eaa55d Compare April 30, 2026 16:50

Copilot AI review requested due to automatic review settings April 30, 2026 17:03

Copilot started reviewing on behalf of mathbunnyru April 30, 2026 17:04 View session

Copilot AI reviewed Apr 30, 2026

View reviewed changes

Comment thread cmake/XrplSanitizers.cmake

Comment thread cmake/XrplSanitizers.cmake

mathbunnyru force-pushed the san_flags branch 2 times, most recently from 1867200 to 9a6b2f4 Compare April 30, 2026 17:26

Copilot AI review requested due to automatic review settings April 30, 2026 17:26

Copilot started reviewing on behalf of mathbunnyru April 30, 2026 17:26 View session

Copilot AI reviewed Apr 30, 2026

View reviewed changes

Comment thread cmake/XrplSanitizers.cmake Outdated

mathbunnyru force-pushed the san_flags branch from 9a6b2f4 to 24af64b Compare April 30, 2026 17:35

chore: Do not duplicate sanitizer flags

288c224

Copilot AI review requested due to automatic review settings April 30, 2026 17:40

mathbunnyru force-pushed the san_flags branch from 24af64b to 288c224 Compare April 30, 2026 17:40

Copilot started reviewing on behalf of mathbunnyru April 30, 2026 17:40 View session

mathbunnyru requested review from bthomee and pratikmankawde April 30, 2026 17:46

Copilot AI reviewed Apr 30, 2026

View reviewed changes

Remove old parts of docs

3626d0b

bthomee reviewed Apr 30, 2026

View reviewed changes

Comment thread cmake/XrplSanitizers.cmake

Comment thread cmake/XrplSanitizers.cmake Outdated

Comment thread cmake/XrplSanitizers.cmake Outdated

Better comments

62dec4e

Copilot AI review requested due to automatic review settings April 30, 2026 18:23

Copilot started reviewing on behalf of mathbunnyru April 30, 2026 18:23 View session

Copilot AI reviewed Apr 30, 2026

View reviewed changes

Comment thread cmake/XrplSanitizers.cmake

mathbunnyru added 2 commits May 1, 2026 11:42

Merge branch 'develop' into san_flags

557548f

Even more polished version

bce47d8

bthomee approved these changes May 2, 2026

View reviewed changes

Merge branch 'develop' into san_flags

c7aca84

Copilot AI review requested due to automatic review settings May 4, 2026 14:43

Copilot started reviewing on behalf of mathbunnyru May 4, 2026 14:43 View session

Copilot AI reviewed May 4, 2026

View reviewed changes

Comment thread conan/profiles/sanitizers

Comment thread conan/profiles/sanitizers

Comment thread conan/profiles/sanitizers

Comment thread cmake/XrplSanitizers.cmake

Comment thread docs/build/sanitizers.md

		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).

Conversation

mathbunnyru commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

High Level Overview of Change

Context of Change

API Impact

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

mathbunnyru Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bthomee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

codecov Bot commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bthomee May 1, 2026

Choose a reason for hiding this comment

Uh oh!

mathbunnyru May 4, 2026

Choose a reason for hiding this comment

Uh oh!

bthomee May 2, 2026

Choose a reason for hiding this comment

Uh oh!

mathbunnyru May 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

mathbunnyru commented Apr 30, 2026 •

edited

Loading

codecov Bot commented Apr 30, 2026 •

edited

Loading