Make go_binary expose CcInfo #2445
Conversation
|
it could be possible to use |
go/private/rules/binary.bzl
Outdated
| @@ -81,6 +88,34 @@ def _go_binary_impl(ctx): | |||
| ), | |||
| ] | |||
|
|
|||
| if go.cgo_tools and go.mode.link in (LINKMODE_C_ARCHIVE, LINKMODE_C_SHARED): | |||
| cgo_exports = ctx.actions.declare_file("%s.h" % name) | |||
| concat_args = ctx.actions.args() | |||
There was a problem hiding this comment.
I think it's a bug that we currently concatenate header files (#2131). The go command doesn't do this. We should just generate one header from the main package.
We probably need to gather other indirectly included headers from cgo packages and cdeps though.
There was a problem hiding this comment.
this is a very good point regarding transitive cc dependencies. i'll take a look.
There was a problem hiding this comment.
i think iterating on cdeps and merging ccinfo should be enough, i'll add a test case.
|
Thank you for reviewing! Do you know who to ping regarding the bazel crash? Also, thinking about it this weekend, I think it's best to leave linkmode being set by the user and act accordinly in |
|
About the Bazel crash, probably open a new issue and tag hlopko and katre. If you can reproduce it to a small case that stands alone (doesn't require rules_go) that will make it more likely to get fixed (I believe the Bazel team is very understaffed at the moment). About |
|
Quick update, apparently this is a bug in bazel. Will update the PR once I get more information. |
|
According to bazelbuild/bazel#11885, the fix is expected to land in Bazel 3.5.0, will resume once released. |
|
Going through inactive PRs. It looks like this was blocked on Bazel 3.5.0, which is out now. Please update if you want to move forward with this. |
|
Thanks, how would you advise for this to work on earlier supported bazel versions? |
|
I think it would be reasonable to bump the minimum Bazel version to 3.5.0 at this point. 3.x.x has been out for a while without incompatible changes, and people have had time to upgrade from 2.x.x. You can bump the minimum version by editing .bazelci/presubmit.yml (only ubuntu1604 is tested with 2.2.0, the current minimum) and README.rst. That's probably best done as a separate PR. |
|
@steeve Is there any update on this? :) |
|
Not yet because we are blocked from upgrading rules_go because of some technicalities. Soon I hope! |
|
I have rebased this PR, things remaining:
|
This was due to bazelbuild/bazel#12304, fixed by adding a
Used a symlink to the first header from
No need, copied the idea from #2433 and put it in the rule, instead of the action. |
|
I also chose to remove the cc @Nickersoft |
|
Now all tests are passing, exempt on RBE for some reason: Help appreciated |
| @@ -212,56 +205,3 @@ def _include_unique(opts, flag, include, seen): | |||
| return | |||
| seen[include] = True | |||
| opts.extend([flag, include]) | |||
|
|
|||
There was a problem hiding this comment.
A handful of people are probably using this, but it's undocumented, so it's probably okay to remove it.
I think users will just need to depend directly on the go_binary target instead of the .cc target, right?
There was a problem hiding this comment.
Yeah. Actually I'm going to add an alias that's created regardless (so that it doesn't have to use linkmode as a macro).
There was a problem hiding this comment.
I have added a .cc alias to all go_binary with a manual tag and a deprecation message just in case. I can remove it but I felt it was harmless.
There was a problem hiding this comment.
That sounds like a good approach. Let's also mention this in https://github.com/bazelbuild/rules_go/wiki/Deprecation-schedule and say that it will be removed in v0.27.0.
|
Actually I was able to reproduce the test failure without RBE on my Linux VM (Debian based). Not sure why it didn't show up on the Ubuntu 18.04 builder. Looking at the binary, it does link against that library, but I'm not sure the runpath is getting set correctly. It's ldd output: readelf output: |
|
Indeed, I am reproducing on Here is the runfile sandbox: |
|
I am not sure where to look since this rpath is generated by the cc_binary link action. Any idea? |
Signed-off-by: Steeve Morin <steeve@zen.ly>
Signed-off-by: Steeve Morin <steeve@zen.ly>
Deprecates "cat $files > $out". Signed-off-by: Steeve Morin <steeve@zen.ly>
No shell required, and works on platforms without bash. Signed-off-by: Steeve Morin <steeve@zen.ly>
…ation_context They won't accept a None, only empty values. Signed-off-by: Steeve Morin <steeve@zen.ly>
Signed-off-by: Steeve Morin <steeve@zen.ly>
Also, expose only the first cgo header via a symlink. Shamelessly copied from #2682 Signed-off-by: Steeve Morin <steeve@zen.ly>
|
Alright so it seems I have opened a can of worms with 86ff842. For some reason, in The current sandbox is: Whereas it could/should be: |
|
I have not yet figured out what triggers the "good" behaviour. |
|
I mean, I could make the CI green, but I feel there's something more to this that's worth investigating. |
|
In my digging in the rabbit hole, I think I found a way to rewrite the cgo flag dance in a cleaner way: This bit of code has all the final_cc_info = cc_common.merge_cc_infos(
direct_cc_infos = [
d[CcInfo]
for d in cdeps
],
)
cpp_vars = cc_common.create_compile_variables(
cc_toolchain = go.cgo_tools.cc_toolchain,
feature_configuration = go.cgo_tools.feature_configuration,
preprocessor_defines = final_cc_info.compilation_context.defines,
include_directories = final_cc_info.compilation_context.includes,
quote_include_directories = final_cc_info.compilation_context.quote_includes,
system_include_directories = final_cc_info.compilation_context.system_includes,
user_compile_flags = cppopts,
use_pic = go.mode != LINKMODE_NORMAL,
)
cppopts = cc_common.get_memory_inefficient_command_line(
feature_configuration = go.cgo_tools.feature_configuration,
action_name = C_COMPILE_ACTION_NAME,
variables = cpp_vars,
)
c_vars = cc_common.create_compile_variables(
cc_toolchain = go.cgo_tools.cc_toolchain,
feature_configuration = go.cgo_tools.feature_configuration,
preprocessor_defines = final_cc_info.compilation_context.defines,
include_directories = final_cc_info.compilation_context.includes,
quote_include_directories = final_cc_info.compilation_context.quote_includes,
system_include_directories = final_cc_info.compilation_context.system_includes,
user_compile_flags = copts,
use_pic = go.mode != LINKMODE_NORMAL,
)
copts = cc_common.get_memory_inefficient_command_line(
feature_configuration = go.cgo_tools.feature_configuration,
action_name = C_COMPILE_ACTION_NAME,
variables = c_vars,
)
cxx_vars = cc_common.create_compile_variables(
cc_toolchain = go.cgo_tools.cc_toolchain,
feature_configuration = go.cgo_tools.feature_configuration,
preprocessor_defines = final_cc_info.compilation_context.defines,
include_directories = final_cc_info.compilation_context.includes,
quote_include_directories = final_cc_info.compilation_context.quote_includes,
system_include_directories = final_cc_info.compilation_context.system_includes,
user_compile_flags = cxxopts,
use_pic = go.mode != LINKMODE_NORMAL,
)
cxxopts = cc_common.get_memory_inefficient_command_line(
feature_configuration = go.cgo_tools.feature_configuration,
action_name = CPP_COMPILE_ACTION_NAME,
variables = cxx_vars,
) |
|
What I'm going to do is revert the rpath stuff, so that this PR is green, and perhaps open a separate PR for leveraging |
|
Alright, after some digging, indeed, here are the The |
|
Ok so I figured it out. We should be using the |
|
Hum so I got it working by running the test in |
Adapted from #2682 Signed-off-by: Steeve Morin <steeve@zen.ly>
Since we now symlink to the first cgo header. Signed-off-by: Steeve Morin <steeve@zen.ly>
Signed-off-by: Steeve Morin <steeve@zen.ly>
Signed-off-by: Steeve Morin <steeve@zen.ly>
Use the vanity resolved_symlink_{dynamic,interface}_library. Then, move rpath
computation in its own file.
Simplify the relative walking from $ORIGIN a bit.
Signed-off-by: Steeve Morin <steeve@zen.ly>
Signed-off-by: Steeve Morin <steeve@zen.ly>
This wasn't documented, and so it will be removed in the near future. In the mean time, let's be nice. Signed-off-by: Steeve Morin <steeve@zen.ly>
Signed-off-by: Steeve Morin <steeve@zen.ly>
|
And we are green! Greener, I might add. |
| @@ -212,56 +205,3 @@ def _include_unique(opts, flag, include, seen): | |||
| return | |||
| seen[include] = True | |||
| opts.extend([flag, include]) | |||
|
|
|||
There was a problem hiding this comment.
That sounds like a good approach. Let's also mention this in https://github.com/bazelbuild/rules_go/wiki/Deprecation-schedule and say that it will be removed in v0.27.0.
Signed-off-by: Steeve Morin <steeve@zen.ly>
|
All set! |
This PR makes
go_binarypropagateCcInfo.This is an early draft to get the discussion going.
I'm quite happy so far it's pretty easy.
That said, I thought I would be able to have one
CcInfothat exports both static and shared binaries, but I think that's not possible since it needs a transition.Also, don't mind the
cfg = "host"commit, I need it to test it for android rules.At this moment, using
c-sharedmakes Bazel 3.0.0 crash:Expand
What would be really cool is for thego_binaryto be eitherc-sharedorc-archivedepending on the target which depends on it. The only way I think on how to do that would be to have a scaffold wrapper that would forward the original providers and add 2 splits that would switch toc-archiveorc-shared.UPDATED: Just tested, "forwarding" the providers doesn't work. I don't see how to do this without using another target + macro. Or perhaps making all outgoing attrs transition to c-shared/c-archive to properly fill
CcInfo. That may dramatically increase the analysis time, though, since srcs and deps would exist 3 times: orignal, c-archive, c-shared.Perhaps it's best to leave linkmode exposed and enable CcInfo accordingly.
Ideally, even, it'sgo_librarywhich would exposeCcInfo. That of course creates the question of the sandwich (go -> C -> go), but I guess that's less of an issue.Update: not possible since if one gives multiple
go_librarytocc_*then things won't go well.All in all, I gotta say it's pretty nice to write this and it just works:
Refs #2433