Skip to content
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

cgo: declare rules for each supported mode, select in aspect #1595

Merged
merged 3 commits into from
Aug 30, 2018

Conversation

jayconrod
Copy link
Contributor

go_binary and go_test allow developers to set mode attributes that
affect build tags, such as goos, goarch, race, and msan. These
attributes are propagated with an aspected over "deps" and "embed"
edges. Each rule the aspect visits must return a GoLibrary provider.

Unfortunately, our cgo implementation declares cc_library,
objc_library, and cc_binary rules that depend on sources generated by
_cgo_codegen. _cgo_codegen performs source filtering using build
tags. The native rules can't depend on a platform-specific GoSource
provider created by the aspect.

This change introduces separate _cgo_codegen and cc_library rules for
each platform. A new rule, _cgo_select_embed produces GoLibrary and
GoSource providers. The resolve function called by the aspect can
produce a GoSource for alternate platforms.

This change increases load and analysis time by about 15% (measured by
building tests/integration/popular_repos, cold starting Bazel but with
a warm disk cache). However, this is necessary to build
golang.org/x/sys/unix in alternate modes.

Fixes #1592

go_binary and go_test allow developers to set mode attributes that
affect build tags, such as goos, goarch, race, and msan. These
attributes are propagated with an aspected over "deps" and "embed"
edges. Each rule the aspect visits must return a GoLibrary provider.

Unfortunately, our cgo implementation declares cc_library,
objc_library, and cc_binary rules that depend on sources generated by
_cgo_codegen. _cgo_codegen performs source filtering using build
tags. The native rules can't depend on a platform-specific GoSource
provider created by the aspect.

This change introduces separate _cgo_codegen and cc_library rules for
each platform. A new rule, _cgo_select_embed produces GoLibrary and
GoSource providers. The resolve function called by the aspect can
produce a GoSource for alternate platforms.

This change increases load and analysis time by about 15% (measured by
building tests/integration/popular_repos, cold starting Bazel but with
a warm disk cache). However, this is necessary to build
golang.org/x/sys/unix in alternate modes.

Fixes bazelbuild#1592
@jayconrod
Copy link
Contributor Author

@ianthehat PTAL. I think this is the only way to do this without being able to perform C/C++/ObjC compile / link actions inside go_library. However, there's a significant performance penalty in loading and analysis, since there are a lot of extra rules.

@steeve
Copy link
Contributor

steeve commented Jul 26, 2018

Note that bazel master is now exposing the cc provider to Skylark \o/ ! Not sure it's in 0.16 though

@jayconrod
Copy link
Contributor Author

@steeve We get cc providers already from cc_library dependencies? Did you mean something else?

I'm not sure if they've exposed a way to create C/C++ compile / link actions. That would be really useful because we wouldn't need to use cc_library / cc_binary anymore to compile cgo code. This PR has a ~15% performance penalty in loading and analysis, but if we could create compile / link actions directly, that would go away.

@steeve
Copy link
Contributor

steeve commented Jul 26, 2018

I meant being able to generate cc_library directly from a rule (returning a cc provider) without having to link macros together.

@steeve
Copy link
Contributor

steeve commented Jul 26, 2018

Since this commit: bazelbuild/bazel@95a40be

@amckague
Copy link

amckague commented Jul 30, 2018

Running this commit rebased on master didn't appear to solve the undefined: raceenabled problems I'm seeing in my own build. I'd appreciate pointers on where I can add debugging information to see what's going on.

In my case we're invoking the protoc rules, have a copy of sys in vendor, and using -race during bazel test. (in case that context helps at all)
(# gazelle:proto disable_global, or forcing protoc to be statically linked might be part of the picture)

@jayconrod jayconrod merged commit 8b0118f into bazelbuild:master Aug 30, 2018
@jayconrod jayconrod deleted the cgo-mode branch August 30, 2018 04:55
jayconrod added a commit that referenced this pull request Sep 5, 2018
go_binary and go_test allow developers to set mode attributes that
affect build tags, such as goos, goarch, race, and msan. These
attributes are propagated with an aspected over "deps" and "embed"
edges. Each rule the aspect visits must return a GoLibrary provider.

Unfortunately, our cgo implementation declares cc_library,
objc_library, and cc_binary rules that depend on sources generated by
_cgo_codegen. _cgo_codegen performs source filtering using build
tags. The native rules can't depend on a platform-specific GoSource
provider created by the aspect.

This change introduces separate _cgo_codegen and cc_library rules for
each platform. A new rule, _cgo_select_embed produces GoLibrary and
GoSource providers. The resolve function called by the aspect can
produce a GoSource for alternate platforms.

This change increases load and analysis time by about 15% (measured by
building tests/integration/popular_repos, cold starting Bazel but with
a warm disk cache). However, this is necessary to build
golang.org/x/sys/unix in alternate modes.

Fixes #1592
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cgo does not propagate mode. sys/unix is broken with race = "on"
4 participants