Skip to content

Commit

Permalink
Generalize boringcrypto to GOEXPERIMENT support (bazel-contrib#3401)
Browse files Browse the repository at this point in the history
The new `experiments` parameter on all SDK rules can now be used to
enable Go experiments. This subsumes the previously special-cased
`boringcrypto` support and allows us to push the duty to match the Go
SDK version on the user.
  • Loading branch information
fmeum authored and healthy-pod committed Feb 22, 2023
1 parent 3db9c6e commit f6a5c7d
Show file tree
Hide file tree
Showing 12 changed files with 59 additions and 63 deletions.
2 changes: 1 addition & 1 deletion go/private/BUILD.sdk.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ go_sdk(
libs = [":libs"],
package_list = ":package_list",
root_file = "ROOT",
boringcrypto = {boringcrypto},
experiments = {experiments},
tools = [":tools"],
)

Expand Down
3 changes: 1 addition & 2 deletions go/private/actions/compilepkg.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,7 @@ def emit_compilepkg(
outputs.append(out_cgo_export_h)
if testfilter:
args.add("-testfilter", testfilter)
if go.sdk.boringcrypto:
args.add("-boringcrypto")
args.add_all(go.sdk.experiments, before_each = "-experiment")

gc_flags = list(gc_goopts)
gc_flags.extend(go.mode.gc_goopts)
Expand Down
3 changes: 1 addition & 2 deletions go/private/actions/link.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,7 @@ def emit_link(
builder_args.add("-p", archive.data.importmap)
tool_args.add_all(gc_linkopts)
tool_args.add_all(go.toolchain.flags.link)
if go.sdk.boringcrypto:
builder_args.add("-boringcrypto")
builder_args.add_all(go.sdk.experiments, before_each = "-experiment")

# Do not remove, somehow this is needed when building for darwin/arm only.
tool_args.add("-buildid=redacted")
Expand Down
5 changes: 2 additions & 3 deletions go/private/actions/stdlib.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ def _should_use_sdk_stdlib(go):
not go.mode.race and # TODO(jayconrod): use precompiled race
not go.mode.msan and
not go.mode.pure and
not go.sdk.boringcrypto and
not go.sdk.experiments and
go.mode.link == LINKMODE_NORMAL)

def _build_stdlib_list_json(go):
Expand Down Expand Up @@ -80,8 +80,7 @@ def _build_stdlib(go):
args.add("-out", pkg.dirname)
if go.mode.race:
args.add("-race")
if go.sdk.boringcrypto:
args.add("-boringcrypto")
args.add_all(go.sdk.experiments, before_each = "-experiment")
args.add_all(link_mode_args(go.mode))
env = go.env
if go.mode.pure:
Expand Down
2 changes: 1 addition & 1 deletion go/private/providers.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ GoSDK = provider(
fields = {
"goos": "The host OS the SDK was built for.",
"goarch": "The host architecture the SDK was built for.",
"boringcrypto": "Whether to build for boringcrypto",
"experiments": "Go experiments to enable via GOEXPERIMENT.",
"root_file": "A file in the SDK root directory",
"libs": ("List of pre-compiled .a files for the standard library " +
"built for the execution platform."),
Expand Down
6 changes: 3 additions & 3 deletions go/private/rules/sdk.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ def _go_sdk_impl(ctx):
return [GoSDK(
goos = ctx.attr.goos,
goarch = ctx.attr.goarch,
boringcrypto = ctx.attr.boringcrypto,
experiments = ctx.attr.experiments,
root_file = ctx.file.root_file,
package_list = package_list,
libs = ctx.files.libs,
Expand All @@ -46,9 +46,9 @@ go_sdk = rule(
mandatory = True,
doc = "The host architecture the SDK was built for",
),
"boringcrypto": attr.bool(
"experiments": attr.string_list(
mandatory = False,
doc = "Whether the toolchain should be built with boringcrypto support enabled",
doc = "Go experiments to enable via GOEXPERIMENT",
),
"root_file": attr.label(
mandatory = True,
Expand Down
37 changes: 21 additions & 16 deletions go/private/sdk.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -26,21 +26,22 @@ load(
)

MIN_SUPPORTED_VERSION = (1, 14, 0)
MIN_BORINGCRYPTO_VERSION = (1, 19, 0)

def _go_host_sdk_impl(ctx):
goroot = _detect_host_sdk(ctx)
platform = _detect_sdk_platform(ctx, goroot)
version = _detect_sdk_version(ctx, goroot)
_sdk_build_file(ctx, platform, version, ctx.attr.boringcrypto)
_sdk_build_file(ctx, platform, version, experiments = ctx.attr.experiments)
_local_sdk(ctx, goroot)

_go_host_sdk = repository_rule(
implementation = _go_host_sdk_impl,
environ = ["GOROOT"],
attrs = {
"boringcrypto": attr.bool(),
"version": attr.string(),
"experiments": attr.string_list(
doc = "Go experiments to enable via GOEXPERIMENT",
),
},
)

Expand Down Expand Up @@ -114,7 +115,7 @@ def _go_download_sdk_impl(ctx):
_remote_sdk(ctx, [url.format(filename) for url in ctx.attr.urls], ctx.attr.strip_prefix, sha256)

detected_version = _detect_sdk_version(ctx, ".")
_sdk_build_file(ctx, platform, detected_version, ctx.attr.boringcrypto)
_sdk_build_file(ctx, platform, detected_version, experiments = ctx.attr.experiments)

if not ctx.attr.sdks and not ctx.attr.version:
# Returning this makes Bazel print a message that 'version' must be
Expand All @@ -136,7 +137,9 @@ _go_download_sdk = repository_rule(
"goos": attr.string(),
"goarch": attr.string(),
"sdks": attr.string_list_dict(),
"boringcrypto": attr.bool(),
"experiments": attr.string_list(
doc = "Go experiments to enable via GOEXPERIMENT",
),
"urls": attr.string_list(default = ["https://dl.google.com/go/{}"]),
"version": attr.string(),
"strip_prefix": attr.string(default = "go"),
Expand Down Expand Up @@ -226,15 +229,17 @@ def _go_local_sdk_impl(ctx):
goroot = ctx.attr.path
platform = _detect_sdk_platform(ctx, goroot)
version = _detect_sdk_version(ctx, goroot)
_sdk_build_file(ctx, platform, version, ctx.attr.boringcrypto)
_sdk_build_file(ctx, platform, version, ctx.attr.experiments)
_local_sdk(ctx, goroot)

_go_local_sdk = repository_rule(
implementation = _go_local_sdk_impl,
attrs = {
"path": attr.string(),
"version": attr.string(),
"boringcrypto": attr.bool(),
"experiments": attr.string_list(
doc = "Go experiments to enable via GOEXPERIMENT",
),
},
)

Expand Down Expand Up @@ -267,7 +272,7 @@ def _go_wrap_sdk_impl(ctx):
goroot = str(ctx.path(root_file).dirname)
platform = _detect_sdk_platform(ctx, goroot)
version = _detect_sdk_version(ctx, goroot)
_sdk_build_file(ctx, platform, version, ctx.attr.boringcrypto)
_sdk_build_file(ctx, platform, version, ctx.attr.experiments)
_local_sdk(ctx, goroot)

_go_wrap_sdk = repository_rule(
Expand All @@ -282,7 +287,9 @@ _go_wrap_sdk = repository_rule(
doc = "A set of mappings from the host platform to a file in the SDK's root directory",
),
"version": attr.string(),
"boringcrypto": attr.bool(),
"experiments": attr.string_list(
doc = "Go experiments to enable via GOEXPERIMENT",
),
},
)

Expand Down Expand Up @@ -339,7 +346,7 @@ def _local_sdk(ctx, path):
for entry in ["src", "pkg", "bin", "lib", "misc"]:
ctx.symlink(path + "/" + entry, entry)

def _sdk_build_file(ctx, platform, version, boringcrypto):
def _sdk_build_file(ctx, platform, version, experiments):
ctx.file("ROOT")
goos, _, goarch = platform.partition("_")

Expand All @@ -352,7 +359,7 @@ def _sdk_build_file(ctx, platform, version, boringcrypto):
"{goarch}": goarch,
"{exe}": ".exe" if goos == "windows" else "",
"{rules_go_repo_name}": "io_bazel_rules_go",
"{boringcrypto}": str(boringcrypto),
"{experiments}": repr(experiments),
},
)

Expand Down Expand Up @@ -525,7 +532,7 @@ def _version_string(v):
v = v[:-1]
return ".".join([str(n) for n in v]) + suffix

def go_register_toolchains(version = None, nogo = None, go_version = None, boringcrypto = None):
def go_register_toolchains(version = None, nogo = None, go_version = None, experiments = None):
"""See /go/toolchains.rst#go-register-toolchains for full documentation."""
if not version:
version = go_version # old name
Expand All @@ -543,19 +550,17 @@ def go_register_toolchains(version = None, nogo = None, go_version = None, borin
if not version:
fail('go_register_toolchains: version must be a string like "1.15.5" or "host"')
elif version == "host":
go_host_sdk(name = "go_sdk", boringcrypto = boringcrypto)
go_host_sdk(name = "go_sdk", experiments = experiments)
else:
pv = _parse_version(version)
if not pv:
fail('go_register_toolchains: version must be a string like "1.15.5" or "host"')
if _version_less(pv, MIN_SUPPORTED_VERSION):
print("DEPRECATED: Go versions before {} are not supported and may not work".format(_version_string(MIN_SUPPORTED_VERSION)))
if boringcrypto and _version_less(pv, MIN_BORINGCRYPTO_VERSION):
fail("go_register_toolchains: boringcrypto is only supported for versions 1.19.0 and above")
go_download_sdk(
name = "go_sdk",
version = version,
boringcrypto = boringcrypto,
experiments = experiments,
)

if nogo:
Expand Down
16 changes: 16 additions & 0 deletions go/toolchains.rst
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,10 @@ SDK version to use (for example, :value:`"1.15.5"`).
| used for static analysis. The ``nogo`` binary will be used alongside the |
| Go compiler when building packages. |
+--------------------------------+-----------------------------+-----------------------------------+
| :param:`experiments` | :type:`string_list` | :value:`[]` |
+--------------------------------+-----------------------------+-----------------------------------+
| Go experiments to enable via `GOEXPERIMENT`. |
+--------------------------------+-----------------------------+-----------------------------------+

go_download_sdk
~~~~~~~~~~~~~~~
Expand Down Expand Up @@ -367,6 +371,10 @@ used. Otherwise, ``go env GOROOT`` is used.
| only when the build uses a Go toolchain and `toolchain resolution`_ results in this SDK being |
| chosen. Otherwise it will be created unconditionally. |
+--------------------------------+-----------------------------+-----------------------------------+
| :param:`experiments` | :type:`string_list` | :value:`[]` |
+--------------------------------+-----------------------------+-----------------------------------+
| Go experiments to enable via `GOEXPERIMENT`. |
+--------------------------------+-----------------------------+-----------------------------------+

go_local_sdk
~~~~~~~~~~~~
Expand All @@ -392,6 +400,10 @@ This prepares a local path to use as the Go SDK in toolchains.
| build uses a Go toolchain and `toolchain resolution`_ results in this SDK being chosen. |
| Otherwise it will be created unconditionally. |
+--------------------------------+-----------------------------+-----------------------------------+
| :param:`experiments` | :type:`string_list` | :value:`[]` |
+--------------------------------+-----------------------------+-----------------------------------+
| Go experiments to enable via `GOEXPERIMENT`. |
+--------------------------------+-----------------------------+-----------------------------------+


go_wrap_sdk
Expand Down Expand Up @@ -424,6 +436,10 @@ rule.
| build uses a Go toolchain and `toolchain resolution`_ results in this SDK being chosen. |
| Otherwise it will be created unconditionally. |
+--------------------------------+-----------------------------+-----------------------------------+
| :param:`experiments` | :type:`string_list` | :value:`[]` |
+--------------------------------+-----------------------------+-----------------------------------+
| Go experiments to enable via `GOEXPERIMENT`. |
+--------------------------------+-----------------------------+-----------------------------------+


**Example:**
Expand Down
8 changes: 4 additions & 4 deletions go/tools/builders/compilepkg.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func compilePkg(args []string) error {
var testFilter string
var gcFlags, asmFlags, cppFlags, cFlags, cxxFlags, objcFlags, objcxxFlags, ldFlags quoteMultiFlag
var coverFormat string
var boringcrypto bool
var experiments multiFlag
fs.Var(&unfilteredSrcs, "src", ".go, .c, .cc, .m, .mm, .s, or .S file to be filtered and compiled")
fs.Var(&coverSrcs, "cover", ".go file that should be instrumented for coverage (must also be a -src)")
fs.Var(&embedSrcs, "embedsrc", "file that may be compiled into the package with a //go:embed directive")
Expand All @@ -72,7 +72,7 @@ func compilePkg(args []string) error {
fs.Var(&cxxFlags, "cxxflags", "C++ compiler flags")
fs.Var(&objcFlags, "objcflags", "Objective-C compiler flags")
fs.Var(&objcxxFlags, "objcxxflags", "Objective-C++ compiler flags")
fs.BoolVar(&boringcrypto, "boringcrypto", false, "Build stdlib with boringcrypto")
fs.Var(&experiments, "experiment", "Go experiments to enable via GOEXPERIMENT")
fs.Var(&ldFlags, "ldflags", "C linker flags")
fs.StringVar(&nogoPath, "nogo", "", "The nogo binary. If unset, nogo will not be run.")
fs.StringVar(&packageListPath, "package_list", "", "The file containing the list of standard library packages")
Expand Down Expand Up @@ -132,8 +132,8 @@ func compilePkg(args []string) error {
return fmt.Errorf("invalid test filter %q", testFilter)
}

if boringcrypto {
os.Setenv("GOEXPERIMENT", "boringcrypto")
if len(experiments) > 0 {
os.Setenv("GOEXPERIMENT", strings.Join(experiments, ","))
}

return compileArchive(
Expand Down
7 changes: 4 additions & 3 deletions go/tools/builders/link.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ func link(args []string) error {
builderArgs, toolArgs := splitArgs(args)
stamps := multiFlag{}
xdefs := multiFlag{}
experiments := multiFlag{}
archives := archiveMultiFlag{}
flags := flag.NewFlagSet("link", flag.ExitOnError)
goenv := envFlags(flags)
Expand All @@ -62,9 +63,9 @@ func link(args []string) error {
flags.Var(&archives, "arc", "Label, package path, and file name of a dependency, separated by '='")
packageList := flags.String("package_list", "", "The file containing the list of standard library packages")
buildmode := flags.String("buildmode", "", "Build mode used.")
boringcrypto := flags.Bool("boringcrypto", false, "set boringcrypto GOEXPERIMENT")
flags.Var(&xdefs, "X", "A string variable to replace in the linked binary (repeated).")
flags.Var(&stamps, "stamp", "The name of a file with stamping values.")
flags.Var(&experiments, "experiment", "Go experiments to enable via GOEXPERIMENT")
conflictErrMsg := flags.String("conflict_err", "", "Error message about conflicts to report if there's a link error.")
if err := flags.Parse(builderArgs); err != nil {
return err
Expand Down Expand Up @@ -161,8 +162,8 @@ func link(args []string) error {
}
goargs = append(goargs, "-o", *outFile)

if *boringcrypto {
os.Setenv("GOEXPERIMENT", "boringcrypto")
if len(experiments) > 0 {
os.Setenv("GOEXPERIMENT", strings.Join(experiments, ","))
}

// add in the unprocess pass through options
Expand Down
7 changes: 4 additions & 3 deletions go/tools/builders/stdlib.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ func stdlib(args []string) error {
race := flags.Bool("race", false, "Build in race mode")
shared := flags.Bool("shared", false, "Build in shared mode")
dynlink := flags.Bool("dynlink", false, "Build in dynlink mode")
boringcrypto := flags.Bool("boringcrypto", false, "Build stdlib with boringcrypto")
var experiments multiFlag
flags.Var(&experiments, "experiment", "Go experiments to enable via GOEXPERIMENT")
if err := flags.Parse(args); err != nil {
return err
}
Expand Down Expand Up @@ -110,8 +111,8 @@ You may need to use the flags --cpu=x64_windows --compiler=mingw-gcc.`)
}
os.Setenv("CGO_LDFLAGS_ALLOW", b.String())

if *boringcrypto {
os.Setenv("GOEXPERIMENT", "boringcrypto")
if len(experiments) > 0 {
os.Setenv("GOEXPERIMENT", strings.Join(experiments, ","))
}

// Build the commands needed to build the std library in the right mode
Expand Down
26 changes: 1 addition & 25 deletions tests/core/boringcrypto/boringcrypto_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ const origWrapSDK = `go_wrap_sdk(
const wrapSDKBoringcrypto = `go_wrap_sdk(
name = "go_sdk",
root_file = "@local_go_sdk//:ROOT",
boringcrypto = True,
experiments = ["boringcrypto"],
)`

func TestBoringcryptoExperimentPresent(t *testing.T) {
Expand Down Expand Up @@ -94,30 +94,6 @@ func TestBoringcryptoExperimentPresent(t *testing.T) {
}
}

func TestGoRegisterToolchainsChecksVersion(t *testing.T) {
const (
from = `go_wrap_sdk(
name = "go_sdk",
root_file = "@local_go_sdk//:ROOT",
)
go_register_toolchains()`
to = `go_register_toolchains(version = "1.18.0", boringcrypto = True)`
)
mustReplaceInFile(t, "WORKSPACE", from, to)
defer mustReplaceInFile(t, "WORKSPACE", to, from)

out, err := bazel_testing.BazelCmd("build", "//:program").CombinedOutput()
if err == nil {
t.Fatal("bazel build succeeded; expected command failure\n output:", string(out))
}

wantMsg := "go_register_toolchains: boringcrypto is only supported for versions 1.19.0 and above"
if !strings.Contains(string(out), wantMsg) {
t.Fatalf("output of bazel build: expected to contain %q\ngot %v", wantMsg, string(out))
}
}

func mustReplaceInFile(t *testing.T, path, old, new string) {
t.Helper()
if old == new {
Expand Down

0 comments on commit f6a5c7d

Please sign in to comment.