chore: add sanitizers to ci workflow #4462

kostasrim · 2025-01-15T11:49:16Z

Sanitizers used to run once per day. However with the recent changes and failures we decided to run them as part of each PR. This PR removes the daily run and migrates it as part of the CI/PR workflow.

remove daily sanitizers workflow
add sanitizers to ci workflow

Signed-off-by: kostas <[email protected]>

kostasrim · 2025-01-15T12:56:07Z

https://github.com/dragonflydb/dragonfly/actions/runs/12788087085/job/35648645177

adiholden · 2025-01-15T14:00:08Z

.github/workflows/ci.yml

        build-type: [Debug, Release]
        compiler: [{ cxx: g++, c: gcc }]
        cxx_flags: ["-Werror"]
+        sanitizers: ["Plain"]


I would rename Plain to NoSanitizers.
Its easier to understand when looking on the actions this way

Actually I would use "ON" / "OFF" this way you can use matrix.sanitizers

I don't want to use ON/OFF because it has no meaning when you read it https://github.com/dragonflydb/dragonfly/actions/runs/12788815584/job/35650888951

I used "Plain" for "Plain Build" but "NoSanitizers" is better so I will use that

adiholden · 2025-01-15T19:01:19Z

.github/workflows/ci.yml

+            -DCMAKE_CXX_COMPILER="${{matrix.compiler.cxx}}" \
+            -DCMAKE_C_COMPILER_LAUNCHER=sccache \
+            -DCMAKE_CXX_COMPILER_LAUNCHER=sccache \
+            -DCMAKE_CXX_FLAGS="${{matrix.cxx_flags}}" \


this line appears twice

adiholden · 2025-01-15T19:03:05Z

.github/workflows/ci.yml

+        run: |
+          apt -y update
+          apt -y upgrade
+          apt install -y clang


why do we need this step for sanitizers but not for alpine build that uses clang?

lets make this a separate step to just install clang

because alpine comes prebundled with clang

lets make this a separate step to just install clang

IMO this is bad as well. It should be prebundled and should go under container foundtry and not as part of this workflow file. See: https://github.com/romange/container-foundry/blob/main/u24.04-dev.Dockerfile

romange/container-foundry#14

adiholden · 2025-01-15T19:04:42Z

.github/workflows/ci.yml

+        if: matrix.sanitizers == 'Sanitizers'
+        run: |
+            cd ${GITHUB_WORKSPACE}/build
+            ctest -V -L DFLY


what is the reason for different unit test run for sanitizers and the "plain"?

We run unit tests with epoll and without and also some more other manually (for the non sanitizers build) 🤷

you are stating what we are doing. I can see that and therefor I ask what is the reason for this?
Why not run the same for both?

Note that if you unite the step and run the same tests for both you will not need the matrix.sanitizers at all

Helio is a third party lib, not part of dragonfly. We are using sanitizers to test dragonfly and not helio's proactor (iouring or epoll). Furthermore, you are trying to to bring everything under the same umbrella. I don't think this is a good thing. The end goal would be to have separate actions, one for normal builds and one for sanitizers. That way, as we change things, workflows are separate and self contained (similar to what we do with regression tests, that we have action which we parametarize).

Anyway, these are not important for now. I will unify them for now

I believe that unifying the steps fro different builds (similarly to code quadruplication )

removes rows and simplifies

if we need to do changes we dont need to remember to fix both

Unlsess there is a reason to not unify them I dont see why not.
We want the sanitizers to run on different flows, there is a reason we have in the build running tests with different flags for example cluster emulated / lock on hash tag. If we run the sanitizers on this flows as well we might catch different failures.

Similar to unifying the bulild step, you see that we added some params to the build f.e HELIO_STACK_CHECK that we forgot to add to the sanitizers bulid. once we unify this there is nothing to remember

adiholden · 2025-01-15T19:13:25Z

.github/workflows/ci.yml

+            -DWITH_ASAN=ON \
+            -DWITH_USAN=ON \
+            -DCMAKE_C_FLAGS=-Wno-error=unused-command-line-argument \
+            -DCMAKE_CXX_FLAGS=-Wno-error=unused-command-line-argument


this can be passed from matrix.cxx_flags

adiholden · 2025-01-15T19:14:20Z

.github/workflows/ci.yml

+          mkdir -p $GITHUB_WORKSPACE/build
+          cd $GITHUB_WORKSPACE/build
+          cmake .. \
+            -DCMAKE_BUILD_TYPE=Debug \


matrix.build-type

adiholden · 2025-01-15T19:21:07Z

.github/workflows/ci.yml

+          echo "-----------------------------"
+          mkdir -p $GITHUB_WORKSPACE/build
+          cd $GITHUB_WORKSPACE/build
+          cmake .. \


I believe we can make only one build step for sanitizers and non sanitizers and control the configure all the flags in under the matrix

Another downside of this: [build (alpine-dev:latest, Release, g++, gcc, -Werror, NoSanitizers, OFF, OFF)](https://github.com/dragonflydb/dragonfly/actions/runs/12804933055/job/35700274090?pr=4462#logs)

See https://github.com/dragonflydb/dragonfly/actions/runs/12804933055/job/35700274090?pr=4462

(I will figure out something 😄 -- need to expiriment )

ok I suggest use https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/running-variations-of-jobs-in-a-workflow to understand what is the correct why to do this

adiholden · 2025-01-15T19:28:32Z

.github/workflows/ci.yml

+            compiler: { cxx: clang++, c: clang }
+            cxx_flags: ""
+            sanitizers: "Sanitizers"
+        exclude:


why do you add it to the matrix.containers list if you exclude it here?

Because we would generate a bunch more job. exclude is filter on the matrix level. GH action syntax is missleading here :(

You basically "exclude" all the jobs that run on ubuntu-24 container and are not sanitizers. Why? Because otherwise we would have a release ubuntu-24, debug ubuntu-24, etc. That way you get only one job sanitizers + ubuntu 24 with clang

include above is the opposite. it acts as a "decorator" overwritting the matrix value

From what I understand include adds and exclude removes. here you remove all the ubuntu-dev:24 defined in the matrix above therefor I think that if you remove it from the matrix it will be enough we will just run them from the include

kostasrim added 3 commits January 15, 2025 13:47

chore: add sanitizers to ci workflow

5eb421a

Signed-off-by: kostas <[email protected]>

add missing run

d44a95e

Signed-off-by: kostas <[email protected]>

try qith single quote

3ca3d41

kostasrim self-assigned this Jan 15, 2025

kostasrim added 2 commits January 15, 2025 14:05

add missing double quotes

5dd131f

try

7648050

small fix

079d1af

kostasrim requested a review from adiholden January 15, 2025 12:57

adiholden reviewed Jan 15, 2025

View reviewed changes

kostasrim added 3 commits January 16, 2025 10:29

comments

b3d657e

clean up

9c29cf7

try

5e409e3

kostasrim force-pushed the kpr6 branch from d9d52ff to 5e409e3 Compare January 16, 2025 11:04

remove exclude

9753d98

kostasrim force-pushed the kpr6 branch 2 times, most recently from 43b58cd to a5f49a8 Compare January 16, 2025 12:04

comment out sscache

f10bd94

kostasrim force-pushed the kpr6 branch from a5f49a8 to f10bd94 Compare January 16, 2025 12:07

kostasrim added 2 commits January 16, 2025 14:51

comment out alpine

41e2fb4

comment out alpine

536478c

kostasrim force-pushed the kpr6 branch 2 times, most recently from 0de668e to bcb0377 Compare January 16, 2025 14:17

kostasrim force-pushed the kpr6 branch from bcb0377 to ce2db82 Compare January 16, 2025 14:21

fix

b6f346b

kostasrim force-pushed the kpr6 branch from ce2db82 to b6f346b Compare January 16, 2025 14:26

alpine

0d98ac2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add sanitizers to ci workflow #4462

chore: add sanitizers to ci workflow #4462

kostasrim commented Jan 15, 2025

kostasrim commented Jan 15, 2025

adiholden Jan 15, 2025

adiholden Jan 15, 2025

kostasrim Jan 16, 2025

adiholden Jan 15, 2025

adiholden Jan 15, 2025

adiholden Jan 15, 2025

kostasrim Jan 16, 2025

kostasrim Jan 16, 2025

adiholden Jan 15, 2025

kostasrim Jan 16, 2025 •

edited

Loading

adiholden Jan 16, 2025

adiholden Jan 16, 2025

kostasrim Jan 16, 2025

adiholden Jan 16, 2025

adiholden Jan 15, 2025

adiholden Jan 15, 2025

kostasrim Jan 16, 2025

adiholden Jan 15, 2025

kostasrim Jan 16, 2025

adiholden Jan 16, 2025

adiholden Jan 15, 2025

kostasrim Jan 16, 2025

kostasrim Jan 16, 2025

kostasrim Jan 16, 2025

adiholden Jan 16, 2025

kostasrim Jan 16, 2025

chore: add sanitizers to ci workflow #4462

Are you sure you want to change the base?

chore: add sanitizers to ci workflow #4462

Conversation

kostasrim commented Jan 15, 2025

kostasrim commented Jan 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim Jan 16, 2025 •

edited

Loading