Repository navigation
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
1364de0 to
f2d4c87
Compare
|
🎉 All checks passed! This PR is ready for review. |
geomin12
left a comment
geomin12
left a comment
There was a problem hiding this comment.
please just use labels for presubmit for now. we are slowly rolling out postsubmit then presubmit for ASAN / host-ASAN
https://github.com/ROCm/TheRock/blob/main/docs/development/ci_behavior_manipulation.md#pull-request
Callers that set asan_presubmit=true can run ASAN builds and tests as a first-class presubmit gate without requiring 'ci:asan'/'ci:host-asan' PR labels. When enabled: - ASAN CI runs on push/pull_request without opt-in labels - Sandbox test runners are allocated for ASAN tests on all triggers Intended for workflows that narrow the build via build_stages to keep ASAN presubmit costs manageable.
f2d4c87 to
c43f2c9
Compare
ScottTodd
left a comment
ScottTodd
left a comment
There was a problem hiding this comment.
+1 to Geo's comments and I'm not sure about the implementation approach here. We're going to have multiple build variants (ASan, TSan, Debug, Coverage, etc.) and I wouldn't want to add individual options like *_presubmit for each of them.
Let's see the full plan in context with other work and then review the available implementation options.
|
The PR was created as part of splitting the original PR: #7311 I had created ROCm/rocm-systems#10558 and |
|
Closing this in favor of #7780 |
| has_asan_label = ( | ||
| "ci:asan" in ci_inputs.pr_labels or "ci:host-asan" in ci_inputs.pr_labels | ||
| ) | ||
| if ( | ||
| ci_inputs.is_pull_request | ||
| and ci_inputs.build_variant == "asan" | ||
| and not has_asan_label |
There was a problem hiding this comment.
We're going to have multiple build variants (ASan, TSan, Debug, Coverage, etc.) and I wouldn't want to add individual options like
*_presubmitfor each of them.
A few other ideas:
explicit opt-in label per entry point workflow
We could also generalize the ci:asan label by adding a pull_request_opt_in_labels input to .github/workflows/setup_multi_arch.yml then...
- Set to
ci:asanin https://github.com/ROCm/TheRock/blob/main/.github/workflows/multi_arch_ci_asan.yml - Set to
ci:tsanin a futuremulti_arch_ci_tsan.yml - Leave empty in https://github.com/ROCm/TheRock/blob/main/.github/workflows/multi_arch_ci.yml
- Leave empty in https://github.com/ROCm/rocm-systems/blob/develop/.github/workflows/therock-multi-arch-ci-asan.yml
Dynamic enable/disable behavior
We could add an extension point to setup_multi_arch.yml where python ./build_tools/github_actions/configure_multi_arch_ci.py imports another python module that can inject its own logic. That would allow repositories like rocm-systems and rocm-libraries to customize the behavior without needing to fully fork the configure script (see the changes in ROCm/rocm-libraries#10752 for example)
3 participants
Callers that set asan_presubmit=true can run ASAN builds and tests as
a first-class presubmit gate without requiring 'ci:asan'/'ci:host-asan'
PR labels.
When enabled:
Intended for workflows that narrow the build via build_stages to keep
ASAN presubmit costs manageable.
ISSUE ID: #7202