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

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

Closed
JamesMBartlett opened this issue Jun 15, 2022 · 10 comments · Fixed by #3261
Closed

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

JamesMBartlett opened this issue Jun 15, 2022 · 10 comments · Fixed by #3261

Comments

@JamesMBartlett
Copy link
Contributor

I didn't see an FR issue template and the bug template didn't seem relevant, so sorry for not using the template.

Use Case

Pixie would like to be able to build go libraries/binaries with different versions of the go SDK. Pixie has to work with multiple versions of user's go binaries, so we use go binaries built by different versions of go as data inputs to our tests to ensure our code can interact with go binaries regardless of what version built them.

Proposal

I've created a patch of rules_go, that allows specifying gosdk (as a label to the GoSDK provider) on any go_binary or go_test target. Then using a bazel transition all of that targets dependencies will be built with the given SDK. By default the first go_download_sdk/go_host_sdk/go_.*_sdk rule will become the default SDK to use for all rules that don't explicitly specify gosdk.

I've attached a link to the patch below, if you are OK with this sort of change I can add tests, open a PR and continue review there, but I figured I should start a discussion here first since its a non-trivial change. (Note that this patch is based off of v0.32, but I can easily rebase for a PR)

Patch

You can also see this patch in action in our build files here

Also, I'm happy to work on a different solution to building with multiple SDK versions, if people are unhappy with this solution.

Related Issues

This feature request is similar to #2527.

@fmeum
Copy link
Member

fmeum commented Jun 20, 2022

Being able to select between Go SDK versions sounds very useful. Thanks for the suggestion and willingness to implement it!

I would prefer a slightly more general approach. What do you think about the following? Also @linzhp @achew22.

  1. Mimic Bazel's --java_runtime_version with a Starlark string setting such as //go/toolchain:sdk_version (see https://github.com/bazelbuild/bazel/tree/master/tools/jdk for how this is done by the Java rules).

  2. Add appropriate version constraints to the toolchains using config_setting_group. E.g., if the SDK is 1.18.3, it should match the values "" and "1.18" and "1.18.3". As a result, if the flag is at its default, any Go SDK is matched. If the flag is at some non-empty value, only the SDKs with that version are matched.

  3. Offer a way for users to select a per-target SDK. I would probably prefer a separate rule for that rather than a new attribute on go_binary (we already want to get rid of goos/goarch at some point), but that can be discussed.

I can offer assistance along the way.

@achew22
Copy link
Member

achew22 commented Jun 20, 2022

+1 to @fmeum. I think moving to a "more standard" configuration would probably be a good path forward. If we were using toolchains in the more traditional sense, it would be possible to add the toolchain as a transition in a rule external to go_binary (go_distributable_binary?). Then we aren't managing our own targeting independent of transitions, which have proven to be complex to maintain and debug.

@JamesMBartlett
Copy link
Contributor Author

JamesMBartlett commented Jun 21, 2022

@fmeum That makes sense to me. I understand # 1 and # 2. I'm a little confused what you mean by a separate rule for # 3.

@fmeum
Copy link
Member

fmeum commented Jun 22, 2022

I'm thinking of a rule that allows the following:

go_binary(
    name = "foo",
    ...
)

go_cross(
    name = "special_foo",
    target = "foo",
    platform = "//:my_custom_platform",
    sdk_version = "1.17",
)

The name and attributes are of course TBD, but the general concept should be a simple wrapper around a go_binary target with the ability to transition on platform and toolchain.

@kmicklas
Copy link
Contributor

kmicklas commented Aug 1, 2022

I'm planning on implementing this soon, if there are no other attempts yet.

@fmeum
Copy link
Member

fmeum commented Aug 1, 2022

@kmicklas Thanks! Let me know in case you get stuck, happy to provide guidance.

@JamesMBartlett
Copy link
Contributor Author

I have an attempt, that I can clean up and put out in the next couple days, unless your timeline is faster

@JamesMBartlett
Copy link
Contributor Author

Created 2 PRs (#3260, #3261) to support this use case. The first adds the sdk_version build flag. The second adds go_cross, using the added sdk_version flag to transition on sdk version.

@kmicklas
Copy link
Contributor

kmicklas commented Aug 3, 2022

Thanks @JamesMBartlett! You beat me to it.

@mashail
Copy link

mashail commented Aug 12, 2022

thank you @JamesMBartlett we are looking for it

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 a pull request may close this issue.

5 participants