-
-
Notifications
You must be signed in to change notification settings - Fork 50
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
Conversation
FYI @alexeagle @jsharpe |
Will look for a jsharpe peer review once this doesn't have unrelated changes |
@alexeagle @jsharpe I think this is ready for review now |
There was a problem hiding this 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.
Co-authored-by: James Sharpe <james.sharpe@zenotech.com>
Thanks jsharpe, I've applied two suggestions and had a question about the third. Much appreciated! |
@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. |
@jsharpe I know you're busy but we are waiting to consume these fixes, any chance you could have a quick look? |
This seems to be mostly working on linux with RBE / toolchains_llvm - 2 things I run into:
@peakschris you mentioned you ran into some issues running at scale? What specifically were the issues there? |
There was a problem hiding this 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.
@jsharpe thanks for the approval
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. |
@alexeagle please would you approve now that jsharpe has approved? |
@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. |
This PR has some minor updates that I've found are needed in for our codebase:
Changes are visible to end-users: no