bazelbuild / rules_go

Go rules for Bazel
Apache License 2.0
1.37k stars 650 forks source link

Better support for C++ Sandwich #2176

Open Yannic opened 5 years ago

Yannic commented 5 years ago

If I understand correctly, rules_go supports C++ Sandwich s.t. go_library can have .{c,cc,h,..} sources and depend on cc_library. It would be great if that was also possible the other way around, but that currently fails because go_library doesn't return CcInfo.

I did some experiments regarding such an implementation and somehow succeeded to expose the C++ headers to other cc_library targets. However, linking the final binary failed because I could figure out how to get the correct archives (i.e. .a) for the Go targets.

Any help regarding this issue would be greatly appreciated.

jayconrod commented 5 years ago

The current support for this is to set linkmode = "c-archive" on a go_binary rule. This produces an .a file. When you build a binary like this, there's an implicitly declared cc_import target that can be depended on by C/C++ rules. That target has the name of the binary plus the suffix .cc_import, so if your binary name is "foo", you can depend on "foo.cc_import". This isn't really documented, but the details are in go_binary_c_archive_shared.

Admittedly, it's not a great interface. We should probably make go_binary return CcInfo when it's built with this link mode.

We can't do this with go_library though. I guess we actually could, but it would mean declaring a bunch of extra link actions on every library. One thing to be careful of is that a .a file created with linkmode = "c-archive" includes all packages the top-level package transitively depended on. So multiple archives like that can't be included in a cc_binary without conflicts.

Yannic commented 5 years ago

Returning CcInfo from go_binary sounds good, but I think it's also a bit confusing because all transitive dependencies are included in the archive which can lead to unexpected linking errors.

I'm not that familiar with linking in Go, but it seems like compiling with linkmode = "c-archive" is the only way to create something cc_library can depend on, which would mean that returning the .a from go_library isn't going to solve the duplicate-symbol issue.

What might work is the following: CcInfo contains a compilation context for stuff like headers or include paths, and a linking context for the .a or .so. Technically, both of them are optional. What if we return CcInfo with just the headers from go_library and require that the final binary must be a go_binary (cc_binary will fail because of missing symbols). We can use an aspect to propagate Go-providers over cc_library targets. It's still not perfect because, IIUC, main must be written in Go, but in practice that shouldn't be a limitation because the "real" main can be called from Go.

jayconrod commented 5 years ago

and require that the final binary must be a go_binary

There's not a good way to require this in Starlark. If the binary isn't a go_library, we wouldn't have any opportunity to print a good error message.

In Blaze (Google internal version of Bazel), there's some hard-coded Java logic to combine multiple Go targets before linking. This can't be implemented in Starlark though. Even if it were possibly, it's pretty hacky.

We can use an aspect to propagate Go-providers over cc_library targets.

Aspects are expensive. They limit analysis scalability, they put unnecessary restrictions on rules, and they generally add a lot of complication. I don't think this provides enough value to justify a new aspect. We currently use an aspect for cross-compilation (that doesn't follow cdeps edges), and I would very much like to remove it.

Yannic commented 5 years ago

There's not a good way to require this in Starlark.

I know. The error message will be the standard linker error say that symbols are missing. Given that the current behavior when multiple go_binary targets importing the same package results in a similar error complaining about duplicate symbols, I don't worry about that too much.

I don't think this provides enough value to justify a new aspect.

Yes, aspects are expensive and can be compilcated, but sometimes they're also necessary (e.g. Protobuf). I have to think about whether they'd be justified in this situation. This is more thinking-out-loud than a completely well reasoned concept.

We currently use an aspect for cross-compilation ... and I would very much like to remove it.

I think it should be possible to replace this aspect with the new configuration transitions, but I'm not very familiar with the implementation details here.

justincely commented 3 years ago

Is there any intention to add this capability?

steeve commented 3 years ago

I think we do now, don't we ?

steeve commented 3 years ago

See https://github.com/bazelbuild/rules_go/blob/master/tests/core/c_linkmodes/BUILD.bazel#L93-L120