Add configurable stamp support with --[no]stamp and stamp attrs

[robbertvanginkel]

This adds support for rules_go to make stamping of workspace variables conditional to Bazel's --nostamp flag.

Fixes #2102.

[robbertvanginkel]

@jayconrod I can't seem to reproduce the build failure locally, or find the raw non summarized logs. Can you let me know if we can retry or help me out with the logs?

[jayconrod]

This change generally looks good, but I'd prefer we not add a stamp attribute to go_binary and go_test.

stamp is useful on cc_binary, since all of the keys from --workspace_status_command may be used. We don't know which binaries need which flags, so we have a mechanism for binaries to say whether they need stamping at all.

With go_binary, we already explicitly ask for specific keys to be stamped using x_defs. Setting stamp = 0 and x_defs = {"Timestamp": "{BUILD_TIMESTAMP}"} wouldn't make sense. Likewise, setting stamp = 1 without setting x_defs wouldn't make sense.

About the test failures, everything passed except the known failures due to #2100, which we still have no work around for. Don't worry about that for this PR though.

[robbertvanginkel]

I originally went without the stamp attribute, but added it for the tests later. I couldn't figure out a way to test the stamping behavior as the go_test targets could be build with --stamp and --nostamp. I went looking for the other rules did this, and for C the docs on stamp = 1 mention

stamp = 1: ... Use this if there are tests that depend on the build information.

Happy to remove the stamp attribute, but need some guidance on what to do with the existing tests under tests/legacy/examples/stamped_bin.

[jayconrod]

Ah, I just wrote a new testing thing last weekend that would work for this: //go/tools/bazel_testing. You can write a go_bazel_test that runs in a synthetic workspace. The test itself is written in Go and uses the bazel_testing library. It can run bazel commands and verify the result. I haven't migrated all the integration tests yet, but //tests/legacy/no_prefix:no_prefix_test is an example.

So in this case, the test would define a test workspace with a binary to be stamped. It could bazel run the binary with and without the --stamp option. The binary can print something that would be stamped, and the test can look for that in stdout.

[jayconrod]

Looks good. Thanks for working on this!

[munnerz]

I've just pulled in a revision of rules_go that includes this change (upgrading from 0.18.6) - I've found that with this rule definition:

go_library(
    name = "go_default_library",
    srcs = [
        "context.go",
        "ingress.go",
        "useragent.go",
        "util.go",
        "version.go",
    ],
    importpath = "github.com/jetstack/cert-manager/pkg/util",
    visibility = ["//visibility:public"],
    x_defs = {
        "AppVersion": "{STABLE_APP_VERSION}",
        "AppGitCommit": "{STABLE_APP_GIT_COMMIT}",
        "AppGitState": "{STABLE_APP_GIT_STATE}",
    },
)

My AppVersion var is being set to {STABLE_APP_VERSION}. Presumably if I were to set --stamp on my bazel build command, this would be set appropriately.

With go_binary, we already explicitly ask for specific keys to be stamped using x_defs. Setting stamp = 0 and x_defs = {"Timestamp": "{BUILD_TIMESTAMP}"} wouldn't make sense. Likewise, setting stamp = 1 without setting x_defs wouldn't make sense.

This makes sense, however if len(x_defs) > 0, should stamp not default to True to maintain backwards compatibility? Esp. given I cannot set stamp = True on my go_library or go_binary, meaning this becomes a user facing change to how they build the project? (developers now have to specify --stamp, or it has to be added to our .bazelrc).

[munnerz]

update: even when setting --stamp=true, I'm still seeing the same behaviour: cert-manager/cert-manager@cd11389

Any insight as to what I'm doing wrong here? This seems at least like a regression or otherwise a breaking change 😅

I realised we have two different places bazel build is called, so adding --stamp=true does fix the issue. But this is definitely a change since earlier versions of rules_go (the fact this is now a required flag).

[jayconrod]

@munnerz This was intentionally a user-facing change. We were not being consistent with other languages.

I can see an argument for adding a stamp attribute on go_binary similar to cc_binary, but I think the default would have to be to observe the --stamp flag. Setting --stamp in .bazelrc seems like a better option though.