chore: only trigger ASAN build on C/C++ files#32143
chore: only trigger ASAN build on C/C++ files#32143nschonni wants to merge 1 commit intonodejs:masterfrom
Conversation
There was a problem hiding this comment.
I'm broadly in favour, since the ASAN build takes 3 hours to run, but what about things like compiler options in the gyp files (or for that matter changes to gyp itself)? Also some parts of the build (e.g. bits of V8, the inspector protocol) are generated via scripts and if the scripts change that would also lead to a difference in the generated binary.
Perhaps this would be better using paths-ignore with **.md?
|
I'd probably suggest expanding the globs to cover any of the generated files rather than limiting to just the non-MD changes, but it probably needs someone with more understanding of the internals to point them out. It's easy to add |
|
ASAN is a tool to look for bugs in native C/C++ code, but that doesn’t mean that changes to JS files can’t affect its outcome. JS code and test changes can still have a big impact on what C++ code gets run and how. I don’t have strong feelings about this PR itself, but it seems worth pointing this out. |
My understanding is that it is globbing the files changed by the push/pull request so generated files are not considered (because they are not part of the source tree). In theory it could be possible to expand the glob but I don't have a full picture of what would need to be included either. I'd suggest if we want ASAN checking it would be easier to codify "this is definitely a doc-only change" (i.e. only markdown files have changed) so don't trigger the build rather than "this change has affected the compiled code" (source files? tooling? scripts? e.g. changing the yaml file for the ASAN build itself should trigger the build). |
Since the Address Sanitizer is only used on C/C++ files, this prevents the extra build from triggering unless those type of files are touched by a Pull Request or Push.
Checklist