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.

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

GitHub workflow with sanitizers #6746

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

GitHub workflow with sanitizers #6746

wants to merge 2 commits into from

Conversation

aitap
Copy link
Contributor

@aitap aitap commented Jan 21, 2025

There are multiple ways to write the workflow in order to detect the issues like #6733.

One is to be quite close to the reference R-hub workflow description, which is in the HEAD of the current pull request. Here's how it fails on a recent master.

The other is to write a more custom workflow, still reusing the container maintained by Gábor Csárdi and the infrastructure providing binary packages linked with libc++ for the clang+sanitizers check. It's in the parent of the current pull request or the GHA-sanitizers branch. Here's what the failure looks like.

(The supported option is to use rhub::rhub_setup().)

Comments from people experienced with GHA are very welcome; this is the third time I'm doing any CI and the first time I'm writing a non-throwaway GitHub workflow.

@aitap aitap requested a review from MichaelChirico as a code owner January 21, 2025 11:40
Copy link

codecov bot commented Jan 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.62%. Comparing base (3a7ec2d) to head (faeaab1).
Report is 8 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6746   +/-   ##
=======================================
  Coverage   98.62%   98.62%           
=======================================
  Files          79       79           
  Lines       14645    14650    +5     
=======================================
+ Hits        14444    14449    +5     
  Misses        201      201           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MichaelChirico
Copy link
Member

MichaelChirico commented Jan 21, 2025

Did you have a look at the clang-ubsan .devcontainer we have? I have used that in the past. I am not sure about performance in GHA.

https://github.com/Rdatatable/data.table/blob/7445df7473869a73103717a3f34d4e233d1c012b/.devcontainer/r-devel-clang-ubsan/Dockerfile

I'm not sure why the most recent run was cancelled, I'll close & re-open to re-trigger:

https://github.com/Rdatatable/data.table/actions/runs/12886173136

At a higher level, if performance is bad (say >=20% slower than any current PR-level action), I would recommend only running such an action on master.

To date, master-only checks have been done in GLCI -- @ben-schwen / @jangorecki do you know if we've ever had a UBSan (or MSan etc) action set up on GitLab?

@aitap
Copy link
Contributor Author

aitap commented Jan 22, 2025

Thank you for pointing me towards the devcontainer! Unlike rocker/r-devel-ubsan, rocker/r-devel-san-clang does have AddressSanitizer enabled, and we can try to additionally enable LeakSanitizer like here.

I don't think it we'll get it significantly faster than 4 minutes (or anywhere close to 40 seconds), especially if optional dependencies are introduced, so let's leave it on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants