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

fix: pass toolchain env, lint_target_headers #304

Merged
merged 23 commits into from
Oct 15, 2024

Conversation

peakschris
Copy link
Contributor

@peakschris peakschris commented Jun 30, 2024

This PR has some minor updates that I've found are needed in for our codebase:

  1. env-vars required by the compiler and known to the toolchain are automatically passed to clang-tidy. Avoid use of use_default_shell_env when running commands.
  2. Allow the lint_target_headers regex to match header paths with mixed \ and / separators that seem to be produced by clang-tidy.
  3. Set an envvar to prevent msys bash from mangling the path
  4. Improve flag handling

Changes are visible to end-users: no

@peakschris peakschris marked this pull request as ready for review June 30, 2024 11:25
@peakschris
Copy link
Contributor Author

FYI @alexeagle @jsharpe

lint/clang_tidy.bzl Outdated Show resolved Hide resolved
@alexeagle
Copy link
Member

Will look for a jsharpe peer review once this doesn't have unrelated changes

@peakschris peakschris changed the title feat: parameterize clang-tidy name, pass toolchain env, fix lint_target_headers fix: pass toolchain env, lint_target_headers Jul 2, 2024
@peakschris
Copy link
Contributor Author

@alexeagle @jsharpe I think this is ready for review now

Copy link
Member

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

Hey @jsharpe could you take a quick look at this? I'm out of my depth.

lint/clang_tidy.bzl Outdated Show resolved Hide resolved
lint/clang_tidy.bzl Show resolved Hide resolved
lint/clang_tidy.bzl Outdated Show resolved Hide resolved
@peakschris
Copy link
Contributor Author

Thanks jsharpe, I've applied two suggestions and had a question about the third. Much appreciated!

@peakschris
Copy link
Contributor Author

@jsharpe please would you check two latest commits, we were finding some issues when running at scale

@jsharpe
Copy link
Contributor

jsharpe commented Jul 17, 2024

@jsharpe please would you check two latest commits, we were finding some issues when running at scale

Just a quick note to say this is still on my radar to look at but I've not had much time to do so! Will try to in the next week or so.

@peakschris
Copy link
Contributor Author

@jsharpe I know you're busy but we are waiting to consume these fixes, any chance you could have a quick look?

@jsharpe
Copy link
Contributor

jsharpe commented Jul 24, 2024

This seems to be mostly working on linux with RBE / toolchains_llvm - 2 things I run into:

  1. Failure to find some C system headers float.h, stddef.h etc.. I suspect this is a toolchain bug though as its picking up the system gcc version of some of these files instead of using the hermetic files from the toolchain_llvm
  2. Headers from implementation_deps don't seem to be found.

@peakschris you mentioned you ran into some issues running at scale? What specifically were the issues there?

Copy link
Contributor

@jsharpe jsharpe 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 happy with these changes as they are - the issues I've run into with running this on linux can likely be easily fixed in follow-ups so no need to block landing this with those changes.

@peakschris
Copy link
Contributor Author

@jsharpe thanks for the approval

you mentioned you ran into some issues running at scale? What specifically were the issues there?

With large directories the command line parameter limit was being reached, so one of the changes here was to use param files as much as possible.

@peakschris
Copy link
Contributor Author

@alexeagle please would you approve now that jsharpe has approved?

@jsharpe
Copy link
Contributor

jsharpe commented Sep 4, 2024

@alexeagle another ping on this as I think you were on vacation when the original ping was done so this may have slipped through your notifications.

@alexeagle alexeagle merged commit c9cf101 into aspect-build:main Oct 15, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants