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

Add go_cross_test rule for cross-compiling tests. #3274

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

JamesMBartlett
Copy link
Contributor

What type of PR is this?
Feature

What does this PR do? Why is it needed?

- 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>
Signed-off-by: James Bartlett <jamesbartlett@newrelic.com>
Works the same as the `go_cross_binary` rule.
Because of [this](bazelbuild/bazel#15224) bazel issue,
there's no way to access the underlying `go_test` rule's `env`
info provider. So until bazel 5.3.0 becomes the `rules_go` minimum bazel
version, I see no way of passing through the `go_test` rule's `env` to
the `go_cross_test`. This means any `env` set will not be used when the
`go_cross_test` test is run, which is a little unfortunate. However, I
think `go_cross_test` is still useful in the meantime despite this
limitation.

Signed-off-by: James Bartlett <jamesbartlett@newrelic.com>
@achew22
Copy link
Member

achew22 commented Aug 22, 2022

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

@tingilee
Copy link
Contributor

tingilee commented Sep 7, 2022

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"
)

@fmeum
Copy link
Member

fmeum commented Sep 7, 2022

@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 bazelbuild/bazel#16076.

@tingilee
Copy link
Contributor

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

@JamesMBartlett
Copy link
Contributor Author

@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.

@tingilee
Copy link
Contributor

@fmeum Are there updates on bazelbuild/bazel#16430 ? Would you suggest waiting for the upstream Bazel support before landing go_cross_test with limited functionalities?

@fmeum
Copy link
Member

fmeum commented Oct 20, 2022

@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.

@skevy
Copy link

skevy commented May 11, 2024

@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?

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

Successfully merging this pull request may close these issues.

5 participants