gotags in go_binary rules is not handled correctly for the standard library

[toby-jn]

In rules_go v0.38.1 (specifically after 8d309d5) building a binary with gotags=["timetzdata"] no longer includes the timezone database embedded in the build. Can see that it does trigger a rebuild of the stdlib when adding/removing the tag, but it seems it doesn't link with the right thing at the end.

If an anonymous import (import _ "time/tzdata") is added, or if adding the cmdline flag --define=gotags=timetzdata then the build does correctly embed the tzdata.

This is a regression from v0.37.0 where this worked as expected.

What version of rules_go are you using?

v0.38.1

What version of gazelle are you using?

n/a

What version of Bazel are you using?

5.4.0

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

yes

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?

Reproducer

WORKSPACE

workspace(name = "com_example_tztest")

load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

http_archive(
    name = "io_bazel_rules_go",
    sha256 = "dd926a88a564a9246713a9c00b35315f54cbd46b31a26d5d8fb264c07045f05d",
    urls = [
        "https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.38.1/rules_go-v0.38.1.zip",
        "https://github.com/bazelbuild/rules_go/releases/download/v0.38.1/rules_go-v0.38.1.zip",
    ],
)

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

go_rules_dependencies()

go_register_toolchains(version = "1.19.5")

cmd/tztest/main.go

package main

import "time"

func main() {
	_, err := time.LoadLocation("America/New_York")
	if err != nil {
		panic("tzdata is missing")
	}
}

cmd/tztest/BUILD

load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library")

go_library(
    name = "go_default_library",
    srcs = ["main.go"],
    importpath = "example.com/cmd/tztest",
    visibility = ["//visibility:private"],
)

go_binary(
    name = "tztest",
    embed = [":go_default_library"],
    gotags = ["timetzdata"],
    pure = "on",
    visibility = ["//visibility:public"],
)

test.sh

#!/bin/sh

bazel build //cmd/tztest:tztest_embed

echo "Checking if result contains embedded tzdata..."
if go tool nm "$(bazel info execution_root 2> /dev/null)/$(bazel cquery --output=files //cmd/tztest:tztest 2> /dev/null)" | grep -q tzdata; then
    echo "Build contains tzdata!"
    exit 0
else
    echo "Build is missing tzdata :("
    exit 1
fi

What did you expect to see?

When the above script is run with v0.37.0 the artefact correctly contains timezone data and the script prints Build contains tzdata!

What did you see instead?

With v0.38.0 or v0.38.1 the artefact does not contain the embedded tzdata and the script prints Build is missing tzdata :(

[illicitonion]

Coincidentally, I was just filing #3457 with a repro for the exact same problem!

[fmeum]

Okay, I know why this is happening, but fixing it without hurting build times too much requires having a list of tags that influence the standard library. @illicitonion @toby-jn Do you happen to know whether there are any other relevant tags?

[fmeum]

@matloob Could you help us here?

[illicitonion]

Do you happen to know whether there are any other relevant tags?

This is the only one I've happened across - I think in general keeping a list which may be out of date, which we can update as we discover them seems reasonable.

How expensive are we talking here? Building a whole new copy of the stdlib per tag-combination?

[fmeum]

How expensive are we talking here? Building a whole new copy of the stdlib per tag-combination?

Yes, exactly. That's why just not resetting the build tags isn't really an option.

The fix would be to filter instead of blindly reset the gotags in

rules_go/go/private/rules/transition.bzl

Line 277 in afbd9c4

def _go_stdlib_transition_impl(settings, _attr):

. Happy to review PRs for that, but I'm not sure I'll get to it myself before I'm OOO for a while.

[toby-jn]

There are quite a few other tags that can influence the build (looking through https://cs.opensource.google/search?q=%2F%2Fgo:build&sq=&ss=go%2Fgo). Making the filter an allow list (i.e. pick tags which don't rebuild stdlib) instead of trying to ensure that rules_go captures all relevant tags and stays in sync seems a safer option. It does however push more responsibility onto the end user if they want avoid the rebuild across different combinations of tags.

[fmeum]

Most of the search results are in test data and of the remaining ones most are related to platforms which I think are handled differently in rules_go. I did my own search and couldn't find one that doesn't fall in either of those categories, but I haven't gone through all the results.

Assuming that tags affecting the SDK are extremely rare compared to how many tags an average Go monorepo uses, I would very much prefer not to cause excessive rebuilds due to it. We have some support form the Go team, so @matloob may be able to tell us whether enumerating all tags that have an effect is feasible.

[toby-jn]

A few that jumped out to me that might be important: netgo, nethttpomithttp2, gccgo, gc, debuglog, internal_pie, purego, gofuzz. There are also a few goexperiment.* ones, suspect anything with that prefix could be included.

[fmeum]

Contacted the Go team and got this reply:

There isn't a curated list of tags in the standard library, although you could scrape them pretty straightforwardly using the go/build/constraint package, by ingesting the source files in the standard library and recursively walking to find the TagExprs.

(The build-constraint lines “must appear near the top of the file, preceded only by blank lines and other line comments”, so there might be some overhead from just opening the files but hopefully not too much from actually reading them.)

Also bear in mind that there are implicit constraints for GOOS and GOARCH values in filenames, and for "cgo" if the file imports "C", and the values from the CPU environment variable (GOAMD64, GOARM, etc.) and GOEXPERIMENT variable also end up setting implicit build tags.

Ideally we would have a small tool that scans the SDKs of recent Go versions for tags with the approach mentioned above.

Contributions adding that (or just a hard-coded list of tags for now) would be very welcome.

[connyay]

Thanks for the hints @fmeum. I was able to get this patch to enable working with netgo, osusergo, and goexperiment.boringcrypto.

From 3349270cd22224180d456467e805cfa4f1aff284 Mon Sep 17 00:00:00 2001
From: Connor Hindley <connor.hindley@tanium.com>
Date: Wed, 8 Mar 2023 11:17:46 -0700
Subject: [PATCH] Pass through relevant stdlib buildtags.

Before this patch any `tags` provided to `go_binary` were not passed
down to the stdlib to avoid a new stdlib build for every combination of
buildtags. This is an issue for tags such as netgo, osusergo, and
goexperiment.boringcrypto which impact how the stdlib operates.

https://github.com/bazelbuild/rules_go/issues/3456
---
 go/private/rules/transition.bzl | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/go/private/rules/transition.bzl b/go/private/rules/transition.bzl
index a2db6101..f7dcc5ed 100644
--- a/go/private/rules/transition.bzl
+++ b/go/private/rules/transition.bzl
@@ -205,6 +205,12 @@ _stdlib_keep_keys = sorted([
     "//go/config:linkmode",
 ])

+_stdlib_keep_tags = sorted([
+    "netgo",
+    "osusergo",
+    "goexperiment.boringcrypto",
+])
+
 def _go_tool_transition_impl(settings, _attr):
     """Sets most Go settings to default values (use for external Go tools).

@@ -256,9 +262,19 @@ def _go_stdlib_transition_impl(settings, _attr):
     the number built.
     """
     settings = dict(settings)
+    tags_label = "//go/config:tags"
+
+    # Capture build tags that are relevant to stdlib
+    stdlib_tags = []
+    for tag in settings.get(tags_label, []):
+        if tag in _stdlib_keep_tags:
+            stdlib_tags.append(tag)
     for label, value in _reset_transition_dict.items():
         if label not in _stdlib_keep_keys:
             settings[label] = value
+
+    # Force filtered stdlib tags after reset
+    settings[tags_label] = stdlib_tags
     settings["//go/private:bootstrap_nogo"] = False
     return settings

--
2.37.0

I am testing this out in a fairly large codebase. Will look for build time/cache issues before contributing this back.

[malt3]

@connyay hitting the same issue. Any news regarding your tests?

[connyay]

My patch has been a bandaid in the codebase i'm working in. Haven't noticed any large build time increases, but had some unusual net behavior (different dns resolution??) in previously working services that I am still debugging.

[fmeum]

I created #3488 to fix this, please take a look and check whether it fixes your build.

[illicitonion]

Confirmed it fixes my repro - thanks so much!