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

Allow go sdk selection to factor in GOEXPERIMENT values #3582

Open
ddelnano opened this issue Jun 8, 2023 · 9 comments
Open

Allow go sdk selection to factor in GOEXPERIMENT values #3582

ddelnano opened this issue Jun 8, 2023 · 9 comments

Comments

@ddelnano
Copy link

ddelnano commented Jun 8, 2023

What version of rules_go are you using?

v0.38.1

What version of gazelle are you using?

v0.29.0

What version of Bazel are you using?

6.2.0

Does this issue reproduce with the latest releases of all the above?

Yes since this issue is a request for new functionality.

What operating system and processor architecture are you using?

linux/amd64

Any other potentially useful information about your toolchain?

N/A

What did you do?

The go_cross_binary rule supports specifying the sdk_version. This attribute only permits the go version string, which means that if you want to compile the same binary with and without boringcrypto the two sdks must have a different go version. This is awkward to use since it means we must maintain a heuristic for having two go versions for each minor version we want to test against.

The example below shows how this must be done today:

load("@io_bazel_rules_go//go:deps.bzl", "go_download_sdk", "go_register_toolchains", "go_rules_dependencies")
load("@io_bazel_rules_go//go:def.bzl", "go_cross_binary")

go_download_sdk(
    name = "go_sdk_1_20",
    version = "1.20.5",
)

go_download_sdk(
    name = "go_sdk_boringcrypto",
    version = "1.20.4",
    experiments = ["boringcrypto"],
)

go_register_toolchains()
go_rules_dependencies()

go_cross_binary(
  name = "standard_crypto",
  sdk_version = "1.20.5",
  target = ":application",
)

# Ideally the following binary would be able to be compiled with go 1.20.5
go_cross_binary(
  name = "boringcrypto",
  sdk_version = "1.20.4",
  target = ":application",
)

What did you expect to see?

N/A since this is new functionality.

What did you see instead?

I would like for go_cross_binary to select the correct sdk for the same version of go depending on what GOEXPERIMENTs should be enabled for the given binary. I'm not well versed on how Bazel works internally, but I believe this means that toolchain selection needs to take into account the GOEXPERIMENT values in addition to the go version string.

This is something I'm interested in contributing back to rules_go, but I will likely need some direction on where this could be implemented and a high level of what needs to be accomplished.

@fmeum
Copy link
Member

fmeum commented Jun 10, 2023

boringcrypto may actually be a special case. As far as I understand it is mostly used for its FIPS certification, which means that a target platform constraint could be a good fit.

Since Go experiments can affect the SDK in essentially arbitrary ways and can be combined, I find it hard to come up with a way to match them directly.

What do you think of allowing users to add constraints and target_settings to the SDKs' toolchain definitions? This would allow for great flexibility and keep the complexity out of rules_go at the same time.

@linzhp What do you think of modelling boringcrypto as a target platform constraint?

@ddelnano
Copy link
Author

What do you think of allowing users to add constraints and target_settings to the SDKs' toolchain definitions? This would allow for great flexibility and keep the complexity out of rules_go at the same time.

I'm still trying to wrap my head around these concepts, but I believe my current attempt it's trying to accomplish what you described (commit). I was essentially trying to model how the sdk_version_setting works, which is one of the current target_settings.

Can you explain more on the target platform constraint idea? It's not clear to me if that is an implementation detail of the target_settings option or if it is another possible solution.

@linzhp
Copy link
Contributor

linzhp commented Jun 12, 2023

Can we model all experiments as target platform constraints? This allows users to use pre-compiled stdlib with experiments and remove this logic:

https://github.com/bazelbuild/rules_go/blob/286e96454a739681639ef3289d92c23060fcb5af/go/private/actions/stdlib.bzl#L54

@fmeum
Copy link
Member

fmeum commented Jun 18, 2023

@linzhp and I had an offline discussion about this. While it may make sense to model boringcrypto differently, there is still a need to select to select SDKs with arbitrary sets of experiments.

@ddelnano An approach based on build settings similar to sdk_version_setting seems reasonable and a contribution would be appreciated. There are some details that warrant taking a closer look (e.g. whether we should sort/canonicalize the list of experiments that we match on), but we can have those on a PR.

@ddelnano
Copy link
Author

ddelnano commented Jun 21, 2023

@fmeum here is where I've gotten so far. Apologies for the rough shape it's in. I primarily print'ed my way to its current form while running the //tests/core/boringcrypto:boringcrypto_test with toolchain resolution enabled (inside and outside of the test since bazel is launching another bazel as best as I can tell).

It's currently failing to find a toolchain that matches the new goexperiments config_setting_group, but I wasn't making much progress digging into this further myself. I added TODO comment placeholders for things I am fairly confident I'm not handling but need to, but any guidance you can provide on the change and those comments would be appreciated.

I'm also in the #go Bazel slack. So if it's easier for more real time communication there, I'm happy to talk there as well.

@fmeum
Copy link
Member

fmeum commented Jun 27, 2023

Matching individual experiments seems difficult and the semantics aren't super clear: Most experiments have a nofoo form to disable them, so matching them one-by-one could have surprising results.

Could you try matching the value of a single string_list_setting instead? I could see that simplifying the implementation as well.

@sluongng
Copy link
Contributor

If I understand this issue correctly, then it's about implementing a Minimal Version Selection for Go SDK based on the matching experiments. BzlMod and Go Modules have implemented similar techniques to manage external dependencies.

The main problems I can see with this issue are:

  1. There is not a clear standard (on Go project side) on which version is supporting which experiment. That information, ideally, should be available programmatically so that we could query it to select the right version. And no, using https://pkg.go.dev/internal/goexperiment or keeping a version of it inside rules_go does not count.

  2. There would need to be a lock file support implemented for this. We don't want the user to be added in a new go_download_sdk with a newer version in the future and suddenly everything rebuilt randomly. That might be acceptable when there is only a single Go SDK version in the repo, but with multiple versions in play, deterministic would be much appreciated. A separate process/tool could be provided to help maintain such a lock file though.

  3. I don't know if there would be a right UX for this. Or if the problem is too niche to solve right now as part of the build graph construction. We could always urge the user to create a BUILD file editing tool that would query up all these targets and do version resolve outside of rules_go and run bulldozer to edit the targets accordingly. Such a temporary solution would give us more time for the problem to mature a bit more, perhaps with more support APIs from Go project.

@fmeum
Copy link
Member

fmeum commented Jul 4, 2023

I would like to avoid the complexity that full support for (partially) matching experiments would entail, as @sluongng described. If we restrict ourselves to matching the exact list of strings, we would still allow users to select between otherwise identical SDKs, but avoid essentially all of these difficult questions.

@pjjw
Copy link
Contributor

pjjw commented Jul 12, 2023

sorry, ignore previous comment, misunderstood the discussion for a second.

fmeum pushed a commit that referenced this issue Jan 31, 2025
…pecific Go toolchain (#4242)

**What type of PR is this?**

Feature

**What does this PR do? Why is it needed?**

This commit adds a new setting
--@io_bazel_rules_go//go/toolchain:sdk_name which can be used to select
a registered Go toolchain via the name provided in go_download_sdk,
go_wrap_sdk, etc.

This fixes the problem of selecting a toolchain where the same Go
version is used, but different experiements are enabled or patches
applied.

**Which issues(s) does this PR fix?**

Fixes #4240

**Other notes for review**

Works also as a workaround for #3582
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

5 participants