language/go: Add support for //go:build tags

[thempatel]

What type of PR is this?

Feature

What package or component does this PR mostly affect?

language/go

What does this PR do? Why is it needed?

Adds support for the new //go:build build tags syntax introduced in go1.17

Which issues(s) does this PR fix?

Fixes #1099

Other notes for review

[sluongng]

My advice is to split this PR into smaller ones for easier reviewing:

• 1 PR for refactoring test comparision and introduce com_github_google_go_cmp
• 1 PR to add support for //go:build tags

[thempatel]

@sluongng #1244

[thempatel]

@sluongng PTAL

cc: @achew22 in case you are interested and want to review

[sluongng]

@thempatel thanks so much for working on this.

I have several nits as commented. Generally I would recommend moving some of the added, mirror functionality from go/build/constraint to a new file. fileinfo.go is approaching 1000 lines status and the diff hunks were quite hard to consume.

Very glad that you were able to refactor the unit tests using the 2 new structs. Could you please also provide 1 simple test cases (which only uses //go:build and not the old // +build) in testdata as integration test for this? The testdata are used in TestGenerateRules in generate_test.go FYI.

[thempatel]

@sluongng PTAL

[sluongng]

I only have small nits left.

I think this version is a lot cleaner and easier to digest. Breaking the diff into smaller files was definitely the right way to go.

fileinfo.go diff is still quite noisy to digest through. But the test passing is additional assurance.

There are small scope creeps here and there with the usage of cmp.Diff but IMO the end result is strictly better.

Pinging @achew22 @linzhp for additional reviews

[thempatel]

@sluongng PTAL

[thempatel]

@sluongng @achew22 @linzhp ping, hoping to get this in -- we have work blocked on this particular body of code making it's way to mainline

[linzhp]

When we have

-- collector_linux.go --
//go:build linux && amd64

package cpufreqcounters

import (
	"uber.com/inventory"
	"uber.com/file"
)

-- collector_other.go --
//go:build !(linux && amd64)

package cpufreqcounters

import (
	"uber.com/file"
)

This pull request would lead to huge list of deps in go_library like:

    deps = select({
        "@io_bazel_rules_go//go/platform:aix_ppc64": [
            "//uber.com/file",
        ],
        "@io_bazel_rules_go//go/platform:android_386": [
            "//uber.com/file",
        ],
        "@io_bazel_rules_go//go/platform:android_amd64": [
            "//uber.com/file",
            "//uber.com/inventory",
        ],
        "@io_bazel_rules_go//go/platform:android_arm": [
            "//uber.com/file",
        ],
        "@io_bazel_rules_go//go/platform:android_arm64": [
            "//uber.com/file",
        ],
        "@io_bazel_rules_go//go/platform:darwin_386": [
            "//uber.com/file",
        ],
        "@io_bazel_rules_go//go/platform:darwin_amd64": [
            "//uber.com/file",
        ],
        "@io_bazel_rules_go//go/platform:darwin_arm": [
            "//uber.com/file",
        ],
        "@io_bazel_rules_go//go/platform:darwin_arm64": [
            "//uber.com/file",
        ],
        "@io_bazel_rules_go//go/platform:dragonfly_amd64": [
            "//uber.com/file",
        ],
        "@io_bazel_rules_go//go/platform:freebsd_386": [
            "//uber.com/file",
        ],
        "@io_bazel_rules_go//go/platform:freebsd_amd64": [
            "//uber.com/file",
        ],
        "@io_bazel_rules_go//go/platform:freebsd_arm": [
            "//uber.com/file",
        ],
        "@io_bazel_rules_go//go/platform:freebsd_arm64": [
            "//uber.com/file",
        ],
        "@io_bazel_rules_go//go/platform:illumos_amd64": [
            "//uber.com/file",
        ],
        "@io_bazel_rules_go//go/platform:linux_amd64": [
            "//uber.com/file",
            "//uber.com/inventory",
        ],
        "//conditions:default": [],

Can you simplify it to something like:

    deps = [
            "//uber.com/file",
    ] + select({
        "@io_bazel_rules_go//go/platform:android": [
            "//uber.com/inventory",
        ],
        "@io_bazel_rules_go//go/platform:linux": [
            "//uber.com/inventory",
        ],
        "//conditions:default": [],
    }),

or

    deps = select({
        "@io_bazel_rules_go//go/platform:android": [
            "//uber.com/file",
            "//uber.com/inventory",
        ],
        "@io_bazel_rules_go//go/platform:linux": [
            "//uber.com/file",
            "//uber.com/inventory",
        ],
        "//conditions:default":  [
            "//uber.com/file",
        ],
    }),

[linzhp]

nit: According to Uber style guide:

Functions should be sorted in rough call order.

So it's better to put readTags at the top of this file.

[thempatel]

@linzhp I'm somewhat confused by that, does that mean that the existing build tags logic is broken? How does it work today? Is the long list a problem, these are generated files so I would lean toward not introducing scope creep to make the file prettier unless the generated code is causing bugs/issues

[thempatel]

I think the only concern left is a very uncommon edge case: users would have to be using cgo and is converting a code base from normal go build to rules_go for the first time to get affected.

Could you help me understand this edge case (for my own edification), my understanding is if a new repo becomes managed by gazelle and they are using cgo, then the BUILD files should be generated correctly.

Even when that happen, we provide a work around in gazelle by using # keep to fall back to manual BUILD file configuration.

this makes sense to me!

The question here is mainly: how much interops between rules_go and go build do we want to support? at what cost?

I am not sure, to me it seems like it would be situational, maybe some existing examples would help bring clarity

Imo some of the API in build_tags.go should be provided and customizable from Go SDK and Gazelle should only call into them instead of currrent approach of fork/vendor the implementation. But it might take more time to convince Go team to implement that API.

In an ideal world I agree, but I think it is unlikely we'll be able to convince the go team within any reasonable timeframe and we have many other things to do.

@linzhp PTAL

[linzhp]

Sorry, I am getting swamped by the perf season. Would you mind rebasing on master so it's easier for me to test?

[thempatel]

@linzhp rebased and pushed, PTAL

[linzhp]

Some tests are failing

[thempatel]

@linzhp pushed some old code, apologies. Things look green now

[linzhp]

All tests in Uber passed. Thanks for the contribution.

[thempatel]

thanks for the collaboration everyone!

[pcj]

The strings.Cut part causing upgrade pain.

[linzhp]

Can you come up with a PR to fix it?

[thempatel]

I'm sorry to hear that, I will point out that bazel-gazelle's minimum supported version is go1.18

and given that strings.Cut was added in go1.18, it felt like a safe change to make. If possible, please consider upgrading your version of Go.

[pcj]

Yeah, it happens. Just one of those many cases where you think upgrading something will be a small task and it morphs into a bigger one.