Add support for C dependencies with alwayslink=1 by steeve · Pull Request #1879 · bazel-contrib/rules_go

steeve · 2019-01-02T14:46:19Z

This is done via an aspect that walks the C deps graph and builds a
dict between files and alwayslink properties, which is then used when
building the link command line.

Also, the command line is OS dependant, so add support for that too.

This is done via an aspect that walks the C deps graph and builds a dict between files and alwayslink properties, which is then used when building the link command line. Also, the command line is OS dependant, so add support for that too. Signed-off-by: Steeve Morin <steeve@zen.ly>

jayconrod · 2019-01-02T21:43:32Z

go/private/rules/cgo.bzl

 _CgoInfo = provider()
+_CcAlwaysLinkInfo = provider()
+
+_WHOLE_ARCHIVE_FLAGS_BY_GOOS = {


I don't think the {} + {} operator is supported anymore in Starlark. If it is, it's deprecated.

I think it would be simpler to just define a function that returns the args though.

I just tested on Bazel 0.21 and it worked without warning. Where did you see that?

I don't think it will work with --all_incompatible_changes.

See https://docs.bazel.build/versions/master/skylark/backward-compatibility.html#dictionary-concatenation

gotcha, thank you

jayconrod · 2019-01-02T21:44:19Z

go/private/rules/cgo.bzl

+} + {
+    "darwin": ("-Wl,-all_load", ""),
+    "js": ("", ""),
+    "windows": ("", ""),


Out of curiosity, why aren't the flags needed on Windows? Might be good to have a comment explaining it.

They are needed, but I'm not sure what they are

I meant depending on MSVC or MSYS

cgo doesn't support MSVC, so don't worry about that. I think MSYS should accept gcc flags, but I don't know if there's a slightly different variant on Windows. Maybe just take out the case for Windows, and I can fix it later if it's a problem.

alright i'll add windows to the list, then

jayconrod · 2019-01-02T21:49:53Z

go/private/rules/cgo.bzl

+    "windows": ("", ""),
+}
+
+def _always_link_aspect_impl(target, ctx):


An aspect will be too expensive for this. It gets applied not only to the cc_library we depend on through cdeps, but to everything it depends on, which could be a lot of things. Aspects consume a lot of time and memory, and I'd love to get rid of the ones we already have.

It's unfortunate that CcSkylarkApiProvider doesn't expose this information. Do you know if there's an open issue to add it? If not, maybe we can file one. It seems like a reasonable thing to add.

Until then, I think it would be reasonable to make -Wl,--whole-archive the default for all libraries. This would probably fix #1486 (looks like you've already seen that one).

Is there a way to measure this? We build a pretty complex codebase and we didn't feel it was slow?

I've opened bazelbuild/bazel#7033

You can build with --profile=file and then bazel analyze-profile file. It will tell you how much time Bazel spends in loading, analysis, execution, etc, but it won't say which lines of code cost the most. I don't think there's a way to do that yet.

cgo currently adds a huge amount of loading / analysis overhead because it declares cc_library rules for each configuration that could affect build files. That's separate from the cdeps libraries, but in any case, it's quite a bit slower than go build. An aspect would slow it further.

I'm not sure using -Wl,--whole-archive would be the best way really, it may bloat the binaries significantly.

That said, if we could find a way to get the go.o file (generated prior to external linking), and use a normal cc_binary to link it against the rest (bypassing go link), we might get all the niceties for free.

That may be possible, but it would interfere with tooling. bazel query would show your target as a cc_binary with a go_binary dependency. We have some hacks like this already (cgo), but Bazel API changes have made this less necessary.

Thanks for filing bazelbuild/bazel#7033. I think that's the right path forward. I'll comment there.

Thanks for being so reactive.
For now we'll keep using this patch until a better solution arises.

Also, it provides at least a solution for people who are stuck.

jayconrod · 2019-01-11T20:09:07Z

Just to recap, Bazel 0.23.0 will introduce a new Starlark provider that will contain an alwayslink field. It was submitted in 94e5016. The new provider is not documented yet, so I don't know exactly how it will work. When rules_go migrates to that API (which we'll need to do because the old one will be deprecated), we'll observe that field.

I'll leave this PR open for now, but I may close it after there's a more concrete plan.

steeve · 2019-03-27T17:48:25Z

Going back to this. Would you be open to bump the minimum bazel version to 0.23 ?

steeve · 2019-03-27T17:49:17Z

It would remove this whole hack too: https://github.com/bazelbuild/rules_go/blob/master/go/private/rules/cgo.bzl#L239-L252

jayconrod · 2019-03-27T20:43:53Z

I'd rather not drop support for old Bazel versions, but I hope we won't have to anymore. rules_go now has a compatibility layer that can provide a consistent interface across Bazel API breaks. It's in go/private/compat. Maybe we can add something in there to retrieve this field on versions that support it.

At the moment, I'm working on moving a lot of logic into the execution layer. Which means almost a complete rewrite of cgo. So let's not change anything here just yet. I'll likely ask you to take a look at that branch when it's passing CI here. You have a more complicated use case than most folks, and I want to make sure you're not broken by that change.

steeve · 2019-03-27T23:25:40Z

So I take it you'll emit c artefacts from starlark and not via macro? That's great to hear!

steeve · 2019-03-27T23:29:53Z

I'll try to rebase that PR and make use of the new field so that we can remove the aspect.

That said, it won't work on earlier versions of bazel.

jayconrod · 2019-03-28T00:27:29Z

So I take it you'll emit c artefacts from starlark and not via macro? That's great to hear!

That's right, we'll build an entire package in one action, including cgo code generation, C compilation, assembly, packing, coverage, and Go compilation.

Big caveat though: it won't work for Objective C because Bazel doesn't expose the Objective C toolchain to Starlark rules. So we'll still follow the old path when objc = True.

steeve · 2019-03-28T00:31:16Z

That's amazing.

For objc I think we can get pretty far with using -x objc to a cc_binary and aggregating deps as their are both compatible with each other.

steeve · 2019-04-02T15:14:50Z

@jayconrod i just remembered there is https://docs.bazel.build/versions/master/skylark/lib/apple_common.html#new_objc_provider which could perhaps do the trick?

jayconrod · 2019-04-02T15:22:48Z

I think new_objc_provider would be used if an objc_library needs to depend on a go_library or go_binary. Is there a case where we need to do that?

steeve · 2019-04-02T16:32:20Z

I'm not sure, it sure looks to me it could be used to emit an objc_library from a rule?
Which could then be put as a dep to the Cgo cc_library?

steeve · 2019-04-25T09:04:56Z

(going back to this real quick)
Would it be an option to use the aspect on older Bazel versions but the real provider on newer ones ?

steeve · 2019-04-25T09:05:57Z

Also, obviously this PR will change massively related to the changes in GoCompilePkg, not sure how much yet.

steeve · 2019-08-09T10:58:55Z

Closing this, will reopen when the need arises.

steeve requested review from ianthehat and jayconrod as code owners January 2, 2019 14:46

googlebot added the cla: yes label Jan 2, 2019

steeve mentioned this pull request Jan 2, 2019

cgo: C++ static initialization not working #1486

Open

jayconrod reviewed Jan 2, 2019

View reviewed changes

steeve mentioned this pull request Jan 3, 2019

CcSkylarkApiProvider does not expose alwayslink libraries bazelbuild/bazel#7033

Closed

steeve closed this Aug 9, 2019

steeve mentioned this pull request Nov 26, 2019

Add support for cc_library.always_link #2294

Closed

cstrahan mentioned this pull request Jul 24, 2020

Fix static linking with alwayslink #2584

Open

Uh oh!

Conversation

steeve commented Jan 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steeve Jan 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayconrod commented Jan 11, 2019

Uh oh!

steeve commented Mar 27, 2019

Uh oh!

steeve commented Mar 27, 2019

Uh oh!

jayconrod commented Mar 27, 2019

Uh oh!

steeve commented Mar 27, 2019

Uh oh!

steeve commented Mar 27, 2019

Uh oh!

jayconrod commented Mar 28, 2019

Uh oh!

steeve commented Mar 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steeve commented Apr 2, 2019

Uh oh!

jayconrod commented Apr 2, 2019

Uh oh!

steeve commented Apr 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steeve commented Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steeve commented Apr 25, 2019

Uh oh!

steeve commented Aug 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

steeve Jan 3, 2019 •

edited

Loading

steeve commented Mar 28, 2019 •

edited

Loading

steeve commented Apr 2, 2019 •

edited

Loading

steeve commented Apr 25, 2019 •

edited

Loading