rules_go icon indicating copy to clipboard operation
rules_go copied to clipboard

Add `go_cross_test` rule for cross-compiling tests.

Open JamesMBartlett opened this issue 2 years ago • 8 comments

What type of PR is this? Feature

What does this PR do? Why is it needed?

  • Adds go_cross_test rule which reuses the implementation of the go_cross_binary rule from #3261.
  • Adds tests that the go_cross_test rule can be used to change the SDK version of a go_test.
  • Due to an issue in bazel: the go_cross_test rules don't currently copy the env from the go_test rule. This seems like it will be fixed in bazel 5.3.0 but as far as I can tell there will be no way to fix it here until 5.3.0 is the minimum bazel version. Which issues(s) does this PR fix? Continuation from #3202

JamesMBartlett avatar Aug 22 '22 00:08 JamesMBartlett

I'm going to hold off reviewing this one until the other one is merged and the diff gets a lot simpler. Please ping if you don't see a review shortly after the other one merges

achew22 avatar Aug 22 '22 00:08 achew22

Thanks for the PR. I am also looking to adopt this functionality. As I am testing locally, I noticed that usage isn't supported. Here's an example of go_test where the arguments need to be passed into test_setup.sh upon invoking. With this diff, the arguments are not being passed in during runtime while I run bazel test :cross_plan_test

go_test(
    name = "plan_test",
    srcs = [
        "controller_test.go",
    ],
    args = [
        "-kubebuilder_assets_dir",
        "$(rootpaths //bazel:kubebuilder_assets)",
    ],
    data = [
        "//bazel:kubebuilder_assets",
    ],
)

go_cross_test(
      name = "cross_plan_test",
      target = ":plan_test"
)

tingilee avatar Sep 07 '22 16:09 tingilee

@tingilee args is a magic attribute, its value currently can't be propagated to targets depending on a test. This needs to be fixed in Bazel first, see https://github.com/bazelbuild/bazel/issues/16076.

fmeum avatar Sep 07 '22 16:09 fmeum

@JamesMBartlett Any updates to this diff? We would like to incorporate this as part of our monorepo.

tingilee avatar Oct 19 '22 04:10 tingilee

@tingilee I don't think the necessary changes have landed in bazel yet. And I think given the fact it will ignore env and args from the test target, it will probably cause some confusion if landed in its current state. But if the maintainers are happy landing it without support for env and args, then I can rebase and bring this branch up to date.

JamesMBartlett avatar Oct 19 '22 04:10 JamesMBartlett

@fmeum Are there updates on https://github.com/bazelbuild/bazel/pull/16430 ? Would you suggest waiting for the upstream Bazel support before landing go_cross_test with limited functionalities?

tingilee avatar Oct 20 '22 05:10 tingilee

@tingilee Can you use a fork of rules_go or patch this PR in for now?

After Bazel 6 has been released, we can probably raise our minimum version of Bazel again and implement env/env_inherit support. We could then consider landing this with an error message shown if the original test uses args and the cross-compiled one doesn't, which should limit the potential for confusion without having to wait for the upstream PR to land.

fmeum avatar Oct 20 '22 07:10 fmeum

@fmeum just wanted to check on this. I read the linked PR -- seems like there's quite a bit of discussion around whether or not they even want to support this. Do you think we might be able to convince the Bazel maintainers to accept that PR?

skevy avatar May 11 '24 19:05 skevy