-
Notifications
You must be signed in to change notification settings - Fork 248
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
RBE make #548
RBE make #548
Conversation
3819ab1
to
00f83ff
Compare
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.
NICE!!! The build passed! This is super awesome.
Just a few comments/questions.
foreign_cc/cmake.bzl
Outdated
@@ -197,6 +195,13 @@ def _get_generator_target(ctx): | |||
else: | |||
fail("No generator set and no default is known. Please set the cmake `generator` attribute") | |||
|
|||
if "Unix Makefiles" == generator: |
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 feel like this is unnecessary and you just append the generator args in the _cmake_impl
macro. Either that or move allow this macro to also return tool_deps
or something so there's only one where we check generator info. I think I prefer the latter
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.
Good shout that's a better idea than this, it didn't seem right when I added it.
Also, I think this would warrant a patch release for the CMake stuff. |
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.
Awesome work!
No description provided.