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

Provide a platform to the go_binary rule (via skylark) #1030

Closed
pcj opened this issue Nov 17, 2017 · 5 comments
Closed

Provide a platform to the go_binary rule (via skylark) #1030

pcj opened this issue Nov 17, 2017 · 5 comments

Comments

@pcj
Copy link
Contributor

pcj commented Nov 17, 2017

I'd like to {have | be able to write} a skylark build rule that can assemble a set of cross-compiled go binaries.

My first issue is caching of outputs:

In my previous cross-compilation implementation for this repo, there was a rule xgo_binary that took an os_arch attribute, walked down the go_library deps via a parameterized aspect, and placed cross-compiled outputs under a subdirectory scoped to the os_arch attribute bazel-bin/.../${os_arch}/foo.a. The benefit of this that bazel could cache all those outputs and not have to rebuild them every time you built a different "platform". The design of this was from the go stdlib itself (cross-compiling the stdlib places it in a subdirectory like windows_386/).

From what I can tell thus far, every time I cross-compile a new platform now, bazel rebuilds everything down the aspect graph to that platform, eliminating the possibility of caching cross-compiled outputs.

My second issue is that I can't specify the platform (like @io_bazel_rules_go//go/toolchain:linux_amd64) to the rule go_binary rule itself. This may or may not be possible by adding a 'platform(s)' attr to the go_binary rule.

This means that I have to script the assembly of those multiple cross-compiled outputs outside of bazel, and try to duplicate bazel's logic to figure out if any of the upstream deps have changed and rebuild.

It's a little frustrating because that rule did everything except cgo, and now we're back after a year-long loop with platforms and we still can't do cgo and the cache performance is much lower.

@ianthehat
Copy link
Contributor

So would I...

The current design assumes that eventually we will get platform transitions, which are required for things like android, and desired for lots of other use cases.
I believe the plan is to specify those transitions using constraints rather than platforms.
I have no idea when they will arrive though.

Cross compiling with cgo is on it's way, and because it's strictly required and there are no workarounds, it has been one of the primary drivers of the design. Unfortunately this puts us at the mercy of the cc system, which means we have to use cpu/platforms to drive the target platform.

As far as caching, we kind of assumed that people would be using --cpu to drive the target platform, rather than --experimental_platforms, and thus they already have caching (as the primary output directory varies with cpu). I was assuming that when experimental_platforms becomes just platforms it will have a similar effect on the output directory, but we should check that.
It would be easy to add a goos_goarch segment to the path if we need to. Although I would like to see a solid design for when to add it, should we always have it in the path (making it annoying for people that only ever build host builds and want to run the binaries easily) or just when cross compiling (which means special cases and possibly annoyance when trying to write scripts that assemble multi-platform builds but have to work differently depending on the host platform)

Depending on how long it is going to take platform transitions to arrive, it might be fairly easy to add something that could build multiple target architectures for pure go code, it's not any different to the systems we already have for building pure/cgo/race etc variants.

@pcj
Copy link
Contributor Author

pcj commented Nov 17, 2017

So from what I took away from @katre's answer in linked issue, we may be able to specify a platform attribute for all/most build rules, which sounds good.

Still not clear whether it is up to the rule implementor to partition build artifacts into unique subdirectories (or whether bazel itself would handle that), but at least for the short term the answer seems to be yes.

@ianthehat
Copy link
Contributor

Okay, so now we just need to decide what form the partitioning should take (always, or sometime include the goos/goarch, in a directory or directly in the filename etc)

@pcj
Copy link
Contributor Author

pcj commented Nov 25, 2017

Confirming that #1055 improves cache performance significantly 🛩️

@ianthehat
Copy link
Contributor

https://github.com/ianthehat/rules_go/tree/exe will make your life even better.
It requires the current release candidate of bazel to work, but it renames the executables based on the build mode as well.
This means although you still need the external script, you can just build all the modes in a loop knowing they will be fully cached, and then collect the results at the end (because they will be accumulated rather than overwritten)
I was also going to experiment with allowing you to override the goos and goarch on a go_binary rule, and have the toolchain merely specify the defaults if not set. It would not work for anything that needed the c compiler, but for pure go it would let you build all the cross compilations you want in a single pass.

ianthehat added a commit to ianthehat/rules_go that referenced this issue Dec 7, 2017
This adds goos and goarch attributes to a go_binary
If specified they force the binary to cross compile to the specified
architecture.
It also adds a test that uses this to cross compile and verify the binary was
correctly cross compiled.
This also required multiple rules for the same binary in different modes, which
meant we needed control over the binary file name (so not using the rule name)

Fixes bazel-contrib#1104
Fixes bazel-contrib#1030
ianthehat added a commit to ianthehat/rules_go that referenced this issue Dec 7, 2017
This adds goos and goarch attributes to a go_binary
If specified they force the binary to cross compile to the specified
architecture.
It also adds a test that uses this to cross compile and verify the binary was
correctly cross compiled.
This also required multiple rules for the same binary in different modes, which
meant we needed control over the binary file name (so not using the rule name)

Fixes bazel-contrib#1104
Fixes bazel-contrib#1030
ianthehat added a commit that referenced this issue Dec 7, 2017
This adds goos and goarch attributes to a go_binary
If specified they force the binary to cross compile to the specified
architecture.
It also adds a test that uses this to cross compile and verify the binary was
correctly cross compiled.
This also required multiple rules for the same binary in different modes, which
meant we needed control over the binary file name (so not using the rule name)

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

No branches or pull requests

2 participants