`--@io_bazel_rules_go//go/config:pgoprofile` overridden unconditionally by `go_binary` default `pgoprofile`

[rickystewart]

Here, the command-line (Starlark) flag --@io_bazel_rules_go//go/config:pgoprofile is defined with a default value of --@io_bazel_rules_go//go/config:empty.

However, the go_binary pgoprofile argument also takes a label attribute that defaults to //go/config:empty. Here we check if pgoprofile is unset (via if getattr(attr, "pgoprofile", "auto") == "auto") and override the settings with the provided value if it is set. But since the pgoprofile argument to go_binary does have a default value, it is actually never going to be unset.

These two things combined mean that it is actually impossible to use the --@io_bazel_rules_go//go/config:pgoprofile command-line flag to configure a PGO profile for a go_binary as suggested by the documentation. The value of pgoprofile set on the go_binary (either explicitly or implicitly via the default value of //go/config:empty) will always override it.

This patch seems to be one possible way to address the issue (though there are presumably multiple different options -- I haven't tested this one enough to be 100% sure this works as intended):

diff --git a/go/private/rules/binary.bzl b/go/private/rules/binary.bzl
index 8bbe4622..bea22f23 100644
--- a/go/private/rules/binary.bzl
+++ b/go/private/rules/binary.bzl
@@ -400,14 +400,13 @@ _go_binary_kwargs = {
             </ul>
             """,
         ),
-        "pgoprofile": attr.label(
-            allow_files = True,
+        "pgoprofile": attr.string(
             doc = """Provides a pprof file to be used for profile guided optimization when compiling go targets.
             A pprof file can also be provided via `--@io_bazel_rules_go//go/config:pgoprofile=<label of a pprof file>`.
             Profile guided optimization is only supported on go 1.20+.
             See https://go.dev/doc/pgo for more information.
             """,
-            default = "//go/config:empty",
+            default = "auto",
         ),
         "_go_context_data": attr.label(default = "//:go_context_data", cfg = go_transition),
         "_allowlist_function_transition": attr.label(

This is similar to what is done for the other arguments, e.g. race, msan, goos, etc. -- these all have a default value of auto which, when combined with appropriate logic, allows rules_go to use the global configuration (--@io_bazel_rules_go//go/config:race, etc.) unless a value besides the default auto is supplied in the go_binary arguments.

[fmeum]

Making this attribute a string means that we might not convert labels correctly (i.e. in the context of the rules_go module, not the module containing the go_binary target).

Could we keep it an attr.label but either set the default to None or detect it by comparing Target.label to Label("//go/config:empty")?

A PR would be very welcome.