Add `go_cross_binary` rule for cross-compilation. #3261

JamesMBartlett · 2022-08-02T23:54:45Z

What type of PR is this?
Feature

What does this PR do? Why is it needed?

This PR adds a go_cross_binary rule, that allows cross compiling go_binary generated targets to different platforms and/or different versions of the go SDK. See #3202 for the desired use case this supports. Implementation details:

The go_cross_binary rule wraps a go_binary and transitions on //command_line_option:platforms and @io_bazel_rules_go//go/toolchain:sdk_version based on the platform and sdk_version attrs of the go_cross_binary rule.
Adds testing in tests/core/cross that ensures that go_cross_binary targets get built for the correct platform and sdk_version.

Which issues(s) does this PR fix?

Fixes #3202

Other notes for review
This is the second of two diffs to support the use case in #3202. The first is here #3260. So once the other one lands this will not include the first commit.

Working with my employer at the moment to sign the CLA.

google-cla · 2022-08-02T23:54:48Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

fmeum · 2022-08-03T08:33:36Z

go/private/rules/cross.bzl

+    old_default_info = ctx.attr.target[DefaultInfo]
+    old_executable = old_default_info.files_to_run.executable
+    if not old_executable:
+        fail('target must have executable')


A go_binary doesn't necessarily have executable set, e.g. when it is build with linkmode c-shared.
If there is no executable, you can just forward all providers unchanged - no need for the symlink trick.

Could you also add a test verifying that we don't break non-standard linkmodes?

Do you have ideas on how to switch between a executable and non executable version of go_cross. I see how go_binary does it in wrappers.bzl. However, it seems to me like there’s no way for a go_cross_wrapper macro to access the attributes of the go_binary specified by the target attribute. I could use native.existing_rule but as far as I can tell that would only work if the rule the target Label refers to is in the same package as the go_cross rule.

I updated the diff to have a go_cross wrapper macro that tries to find the target in the local package, but if it fails it suggests that the user explicitly call go_cross_executable or go_cross_non_executable. This feels a little gross to me, so let me know if you have a better suggestion.

Thanks for trying this out. It also feels rather invasive to me, so I would prefer a different solution.

Simple idea first: How common is it for go_cross targets to be bazel run? With platform set, it would be relatively unlikely that the target platform matches host.

Next simplest idea: Unconditionally mark the rule as executable, but write and advertise a bash/bat script or simple Go binary that prints an error message when executed in case the link mode doesn't provide you with an actual executable.

Since one can specify a go_cross rule without a platform and with just an sdk_version, I decided to go for the second option. This way users can still bazel run a go_cross rule that just changes the sdk version.

fmeum · 2022-08-03T08:36:52Z

go/private/rules/cross.bzl

+
+    new_default_info = DefaultInfo(
+        files = depset([new_executable]),
+        runfiles = old_default_info.default_runfiles,


For better or worse, you should probably copy this ugly workaround here: https://github.com/bazelbuild/rules_go/blob/47676dfc37a012c109e036b551c07a7e2da3d68b/go/private/rules/binary.bzl#L133

go/private/rules/cross.bzl

fmeum · 2022-08-03T08:39:18Z

go/private/rules/cross.bzl

+            """,
+        ),
+        "sdk_version": attr.string(
+            doc = """GoSDK version to transition the given target to.


Suggested change

doc = """GoSDK version to transition the given target to.

doc = """Go SDK version to transition the given target to.

go/private/rules/cross.bzl

fmeum · 2022-08-03T08:42:11Z

go/private/rules/cross.bzl

+    "implementation": _go_cross_impl,
+    "attrs": {
+        "target": attr.label(
+            doc = """Go binary to transition to the given platform and/or sdk_version.


We may want to use providers to limit the rule to Go targets.

fmeum · 2022-08-03T08:42:50Z

go/private/rules/cross.bzl

+        ),
+    },
+    "cfg": go_cross_transition,
+    "doc": """    """,


sluongng · 2022-08-04T09:46:16Z

go/private/rules/cross.bzl

+    new_executable = ctx.actions.declare_file(ctx.attr.name)
+    ctx.actions.symlink(output=new_executable, target_file=old_executable)


Im a bit confused by this as it reads as:

declare a new binary executable file

the new binary executable file is a symlink to the old executable

If you don't mind, I would love to get 1-2 line of comments on top of this explaining what's happening. 🙏

tests/core/cross/BUILD.bazel

fmeum · 2022-08-16T09:52:07Z

go/private/rules/cross.bzl

+        # the underlying `go_binary` target is executable.
+        error_script = _error_script(ctx)
+
+        # See the implementaion of `go_binary` for an explanation of the need for default vs data runfiles here.


Suggested change

# See the implementaion of `go_binary` for an explanation of the need for default vs data runfiles here.

# See the implementation of `go_binary` for an explanation of the need for default vs data runfiles here.

go/private/rules/cross.bzl

- Adds a `go_cross` rule that wraps a `go_binary` to generate a cross-compiled version of the binary. - Supports compiling for a different platform, and/or a different golang SDK version. - Adds docs for the new `go_cross` rule. - Adds testing in `tests/core/cross` for the new `go_cross` rule. Signed-off-by: James Bartlett <jamesbartlett@newrelic.com>

fmeum · 2022-08-20T10:21:37Z

@achew22 This generally looks good to me. Are you also fine with the new API?

achew22

One really dumb high level question. Is go_cross always going to wrap a go_binary or can it wrap a go_test (or other)? If it's really only ever intended to be used to wrap a go_binary what would we think about calling it go_cross_binary?

achew22 · 2022-08-20T20:28:23Z

Other than that question, this looks more than good. Thanks so much for putting in so much hard work @JamesMBartlett!

joeljeske · 2022-08-20T20:45:48Z

One really dumb high level question. Is go_cross always going to wrap a go_binary or can it wrap a go_test (or other)?

Does this PR include a strategy for setting the sdk for a go_tests? I would really love to see that, especially for testing a specific sdk version.

sluongng · 2022-08-21T06:30:39Z

Does this PR include a strategy for setting the sdk for a go_tests? I would really love to see that, especially for testing a specific sdk version.

@joeljeske See bc60909

joeljeske · 2022-08-21T13:59:41Z

Thanks! From what I can tell, running a go_test with a different SDK version requires passing different bazel cli flags, right?

Given that go_cross enables transitioning a go_binary the SDK using attrs, do you think we should enable a similar behavior for go_test? It seems like it would be important to be able to run tests under the non-default sdk in the same invocation as building binaries.

It would be important that one could use go_cross with a target of a test, or if one could set sdk version for a go_test directly.

What do you think?

achew22 · 2022-08-21T19:22:50Z

I love the excitement around this feature, but let's leave the feature requests for different conversations. This PR is useful as it stands right now, we're just haggling over naming. Besides, I think the bikeshed should be blue -- it's obviously the best color.

joeljeske · 2022-08-21T21:00:40Z

Very good, thank you!

Signed-off-by: James Bartlett <jamesbartlett@newrelic.com>

achew22

I ❤️ this PR. Thanks @JamesMBartlett. Leaving it for @fmeum to take one last look since he has a knack for the subtle.

JamesMBartlett · 2022-08-22T00:44:36Z

I changed the name to go_cross_binary since separate rules are necessary to support go_binary and go_test crossing. And I opened another PR for the go_cross_test rule #3274

tests/core/cross/README.rst

go/private/rules/cross.bzl

Signed-off-by: James Bartlett <jamesbartlett@newrelic.com>

fmeum · 2022-08-25T18:25:20Z

Thanks again for the great contribution, @JamesMBartlett!

jfirebaugh · 2022-09-23T23:40:27Z

Just curious, what's the advantage of go_cross_binary versus using the --platforms flag? The latter seems like the officially recommended way to do cross compiling with Bazel. (Although --platforms doesn't seem to be very well supported by rules_go.)

JamesMBartlett · 2022-09-24T00:09:49Z

go_cross_binary does use Bazel's --platforms flag. The purpose of go_cross_binary is to (in Bazel lingo) transition a go_binary rule from the default value of the --platforms flag to the specified platform in the rule. More about transitions here. This can be useful, when managing many different platforms. For instance, at Pixie our use case was to have multiple go binaries built with different platforms/sdk version all as data dependencies to a test, something like:

go_binary(
  name = "binary",
  ...
)
go_cross_binary(
  name = "binary_amd64",
  target = “:binary”,
  platform = "//path/to/amd/platform",
)
go_cross_binary(
  name = "binary_arm",
  target = “:binary”,
  platform = "//path/to/arm/platform",
)

some_test_rule(
  name = "test using binaries",
  data = [":binary_amd64", ":binary_arm"],
  ...
)

Passing the --platforms flag on the command line causes all rules (without transitions) to use that platforms flag, which would make this use case difficult.

Also, correct me if I'm wrong @fmeum but I believe rules_go supports the --platforms flag, with some caveats around setting up crosstool files if you're using cgo. See: https://github.com/bazelbuild/rules_go#how-do-i-cross-compile

jfirebaugh · 2022-09-24T00:37:13Z

Thank you for the explanation, that makes sense.

I encountered two issues with rules_go's support of the --platforms flag:

I was trying to use a self-defined "linux_amd64" platform rather than --platforms=@io_bazel_rules_go//go/toolchain:linux_amd64 (I was unaware of the latter at the time). This leads to a "No matching toolchains" error. Possibly related to Regression in v0.34.0 breaks go_image on M1 macs #3269.
Cross-compiling should use the appropriate SDK's stdlib rather than rebuilding it #3305

JamesMBartlett · 2022-09-27T00:32:06Z

Don't really have context to help with 2, but for your first issue I think you just need to specify a cgo constraint on your custom platform ("@io_bazel_rules_go//go/toolchain:{cgo_off,cgo_on}"). I don't know the details of how to do it with cgo on, but with cgo_off I think it should Just Work:tm:

EDIT: on second thought, I don't think the cgo constraints are actually added to the toolchain so I don't think the above is the issue you're running into

JamesMBartlett mentioned this pull request Aug 2, 2022

[FR/Proposal] Support use of multiple SDK versions in the same bazel workspace #3202

Closed

JamesMBartlett force-pushed the james/go_cross branch from 4c1d00c to b9f5ead Compare August 3, 2022 00:10

fmeum self-requested a review August 3, 2022 08:32

fmeum reviewed Aug 3, 2022

View reviewed changes

sluongng reviewed Aug 4, 2022

View reviewed changes

JamesMBartlett force-pushed the james/go_cross branch 4 times, most recently from 54f8c9e to ea53726 Compare August 4, 2022 20:29

JamesMBartlett force-pushed the james/go_cross branch 2 times, most recently from 3dba311 to dbae7c9 Compare August 16, 2022 03:25

fmeum reviewed Aug 16, 2022

View reviewed changes

achew22 reviewed Aug 20, 2022

View reviewed changes

go_cross->go_cross_binary

18992d5

Signed-off-by: James Bartlett <jamesbartlett@newrelic.com>

JamesMBartlett force-pushed the james/go_cross branch from dbae7c9 to 18992d5 Compare August 22, 2022 00:37

JamesMBartlett changed the title ~~Add go_cross rule for cross-compilation.~~ Add go_cross_binary rule for cross-compilation. Aug 22, 2022

achew22 approved these changes Aug 22, 2022

View reviewed changes

JamesMBartlett mentioned this pull request Aug 22, 2022

Add go_cross_test rule for cross-compiling tests. #3274

Open

fmeum approved these changes Aug 22, 2022

View reviewed changes

tests/core/cross/README.rst Outdated Show resolved Hide resolved

go/private/rules/cross.bzl Outdated Show resolved Hide resolved

nits

46e8395

Signed-off-by: James Bartlett <jamesbartlett@newrelic.com>

fmeum merged commit 55ca35a into bazel-contrib:master Aug 25, 2022

jfirebaugh mentioned this pull request Sep 24, 2022

Cross-compiling should use the appropriate SDK's stdlib rather than rebuilding it #3305

Closed

schwehr mentioned this pull request Dec 1, 2022

no matching toolchains found for types bazel-contrib/rules_jsonnet#164

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `go_cross_binary` rule for cross-compilation. #3261

Add `go_cross_binary` rule for cross-compilation. #3261

JamesMBartlett commented Aug 2, 2022 •

edited

Loading

google-cla bot commented Aug 2, 2022

fmeum Aug 3, 2022

JamesMBartlett Aug 4, 2022 •

edited

Loading

JamesMBartlett Aug 4, 2022

fmeum Aug 5, 2022

JamesMBartlett Aug 16, 2022

fmeum Aug 3, 2022

fmeum Aug 3, 2022

fmeum Aug 3, 2022

fmeum Aug 3, 2022

sluongng Aug 4, 2022

fmeum Aug 16, 2022

fmeum commented Aug 20, 2022

achew22 left a comment

achew22 commented Aug 20, 2022

joeljeske commented Aug 20, 2022

sluongng commented Aug 21, 2022

joeljeske commented Aug 21, 2022

achew22 commented Aug 21, 2022

joeljeske commented Aug 21, 2022

achew22 left a comment

JamesMBartlett commented Aug 22, 2022

fmeum commented Aug 25, 2022

jfirebaugh commented Sep 23, 2022

JamesMBartlett commented Sep 24, 2022 •

edited

Loading

jfirebaugh commented Sep 24, 2022

JamesMBartlett commented Sep 27, 2022 •

edited

Loading

	doc = """GoSDK version to transition the given target to.
	doc = """Go SDK version to transition the given target to.

		new_executable = ctx.actions.declare_file(ctx.attr.name)
		ctx.actions.symlink(output=new_executable, target_file=old_executable)

	# See the implementaion of `go_binary` for an explanation of the need for default vs data runfiles here.
	# See the implementation of `go_binary` for an explanation of the need for default vs data runfiles here.

Add go_cross_binary rule for cross-compilation. #3261

Add go_cross_binary rule for cross-compilation. #3261

Conversation

JamesMBartlett commented Aug 2, 2022 • edited Loading

google-cla bot commented Aug 2, 2022

Choose a reason for hiding this comment

JamesMBartlett Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmeum commented Aug 20, 2022

achew22 left a comment

Choose a reason for hiding this comment

achew22 commented Aug 20, 2022

joeljeske commented Aug 20, 2022

sluongng commented Aug 21, 2022

joeljeske commented Aug 21, 2022

achew22 commented Aug 21, 2022

joeljeske commented Aug 21, 2022

achew22 left a comment

Choose a reason for hiding this comment

JamesMBartlett commented Aug 22, 2022

fmeum commented Aug 25, 2022

jfirebaugh commented Sep 23, 2022

JamesMBartlett commented Sep 24, 2022 • edited Loading

jfirebaugh commented Sep 24, 2022

JamesMBartlett commented Sep 27, 2022 • edited Loading

Add `go_cross_binary` rule for cross-compilation. #3261

Add `go_cross_binary` rule for cross-compilation. #3261

JamesMBartlett commented Aug 2, 2022 •

edited

Loading

JamesMBartlett Aug 4, 2022 •

edited

Loading

JamesMBartlett commented Sep 24, 2022 •

edited

Loading

JamesMBartlett commented Sep 27, 2022 •

edited

Loading