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

chore: only trigger ASAN build on C/C++ files #32143

Closed
wants to merge 1 commit into from

Conversation

nschonni
Copy link
Member

@nschonni nschonni commented Mar 8, 2020

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

@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Mar 8, 2020
@nschonni nschonni mentioned this pull request Mar 8, 2020
2 tasks
Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@nschonni
Copy link
Member Author

nschonni commented Mar 8, 2020

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 **/*.gyp or .gypi, but I thought that was more globbing in of the C/C++ files rather than the generation of files, which is why I didn't include them.

@addaleax
Copy link
Member

addaleax commented Mar 8, 2020

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.

@richardlau
Copy link
Member

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 **/*.gyp or .gypi, but I thought that was more globbing in of the C/C++ files rather than the generation of files, which is why I didn't include them.

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

@nschonni nschonni closed this Mar 21, 2020
@nschonni nschonni deleted the asan-job-filter branch March 21, 2020 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants