Add GOARM64 platform constraints#4557
Conversation
See https://go.dev/wiki/MinimumRequirements#arm64 for which options are surfaced in this PR.
| # Optional ARM64 CPU feature extensions, orthogonal to the version above. | ||
| # These are appended to GOARM64, e.g. "v9.0,lse,crypto". | ||
| constraint_setting( | ||
| name = "lse", |
There was a problem hiding this comment.
I think that I would slightly prefer //go/constraints/lse:{on,off} for the constraints. We should also add explicit values for the defaults and declare them as the default on constraint_setting so that the selects don't need a default case.
@jayconrod @dzbarsky Do you agree or would you prefer different syntax?
There was a problem hiding this comment.
👍 I'll wait for consensus and then make the change.
There was a problem hiding this comment.
Maybe a separate question: should this be a build_setting rather than a constraint_setting? build_setting is a user-specified flag that can modify how code is generated. constraint_setting is a property of the platform that affects toolchain selection. Since I don't think we need to select different toolchains based on GOARM64 or declare constraints in platform, build_setting seems like a better fit to me. It's similar to enabling -race or debug symbols.
There was a problem hiding this comment.
If this is a build_setting, we could use a string_list_flag to let the user set it on the command line (or .bazelrc). We have a few flags already in //go/config.
There was a problem hiding this comment.
I think this is a good line of thinking. Perhaps one way to think a bit further would be to talk about usage. Here is currently how I began using this PR as-is:
platform_transition_filegroup(
name = "server_image_linux_arm64",
srcs = [":server_image"],
- target_platform = "//:linux_arm64",
+ target_platform = "//:linux_arm64_graviton4",
)Where the graviton4 platform had these ARM64 constraints set, and that kind of did all the magic downstream without having to think too much.
What might this look like with the build_setting?
Edit: we do target a couple of diff flavors of ARM64 in this one codebase, so the graviton4 flavor is not the only one.
There was a problem hiding this comment.
You can use a transition to change any build setting. It looks like platform_transition_filegroup uses an internal transition that changes "//command_line_option:platforms" to the given label. transitions.bzl is the implementation.
platform_transition_filegroup looks like it can't handle any other settings, so it wouldn't work for this. Writing a new transition and similar wrapper rule would be a pretty small bit of code though.
Do you use //:linux_arm64_graviton4 for other toolchain selection outside of Go? Whether GOARM64 should be a constraint setting or a build setting is a little ambiguous to me. I'd only lean toward build setting for simplicity because Go doesn't need it for toolchain selection.
There was a problem hiding this comment.
Sorry, didn't get a notification on this thread, just stumbled upon it. I would slightly prefer to make this a constraint. While it's true that it doesn't affect compiler resolution, constraints have the advantage that they don't propogate into the exec configuration, which is usually fairly surprising for build flags. (I know there's work in Bazel itself to fix it but we support a few versions back so it will take a while before we can rely on that). Also, making it a constraint is more aligned with how platforms_contrib is thinking about modeling more complex platforms. @cerisier has been thinking about this problem a lot, he can share more
There was a problem hiding this comment.
I agree with David here, a constraint setting would match the intended semantics better, even if it takes a bit more effort to set up in rules_go.
There was a problem hiding this comment.
Hmm, that's convincing. Let's go with a constraint setting then.
There was a problem hiding this comment.
BTW, I definitely don't want to block on this, but potentially this even belongs in @platforms_contrib so you can set it on your platform and all language rulesets can interpret it as appropriate. I think rust also has some rustc flags to set to target newer instruction sets, but it would be really nice to make it all work seamlessly.
I think once we figure that out we could potentially alias/deprecate these constraints, but not worth blocking progress here
jayconrod
left a comment
There was a problem hiding this comment.
Two additional things:
- Go build constraints like
// go:build arm64.v8.1can filter out source files. Let's make sure to apply those based on theGOARM64setting. I don't think constraints are set based onlseorcrypto, so don't worry about those. I'm not entirely sure where to add these settings, butgo/build.Context.ToolTagsmight be the right place. We setgo/build.Default.BuildTagsinenv.go. - Let's test that setting
GOARM64works. A binary should be able to callruntime/debug.ReadBuildInfo()to read how it was built. There should be a key forGOARM64in there. (Actually I'm not sure we populate that, so if we don't, we could just check that the build constraints are filtering the correct files.
|
Really interested in this getting merged. Do we have an ETA when it will land? |
|
I'm planning to write a short design doc on this topic today and include this as an example. I hope to have a clear understanding of the design space at that point. |
|
The doc is out and it looks like it will be accepted: bazelbuild/bazel#28877 If you want to move this along, you could send a PR to add these constraints to https://github.com/bazel-contrib/platforms_contrib. |
See https://go.dev/wiki/MinimumRequirements#arm64 for which options are surfaced in this PR.
What type of PR is this?
Feature
What does this PR do? Why is it needed?
Adds support for the
GOARM64environment variable via platform constraints, following the same pattern as the existingGOAMD64andGOARMsupport.New constraint settings under
//go/constraints/arm64:v8.0–v8.9,v9.0–v9.5(defaults tov8.0when unset)lseandcrypto(independent boolean constraint settings)These are combined into the
GOARM64env var at build time, e.g.GOARM64=v9.0,lse,crypto.Usage example:
Which issues(s) does this PR fix?
Fixes #4556
Other notes for review
The implementation mirrors the existing amd64/arm pattern: