Use edge transitions not self transitions

[illicitonion]

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

As per bazelbuild/bazel#15157 currently we
can't configure any transition-relevant attributes using select. This is
because we use self-transitions on targets, and when this happens,
configurable attributes aren't passed to the transition, so we make
incorrect transition decisions.

Instead, insert a dummy rule which exists only to transition its
dependencies using an edge transition, which means the appropriate
attributes are resolved before the transition is called.

To achieve this, we symlink the actual built binaries into our
transitioning wrapper rules, and forward any providers needed.

Which issues(s) does this PR fix?

Fixes #3103

Other notes for review

The first commit just extracts variables for attr dicts without modifying them, the second contains the meat of the change.

[illicitonion]

Note that previously we didn't add the new executable to data_runfiles and now we do - this felt like a positive change, assuming not doing so before was simply oversight, but if there was an important reason for not doing this before, I can revert and add a comment.

[fmeum]

@illicitonion I may be missing something, but I think that this PR can be simplified: Instead of using a wrapper to transition the entire go_binary, the go_transition could be applied only to the deps and _go_context_data attribute. This would get rid of the macro and transition wrapper as well as fix some forms of bazelbuild/bazel#14626 (see 089f13e for the very much related complete fix for this issue).

[illicitonion]

Instead of using a wrapper to transition the entire go_binary, the go_transition could be applied only to the deps and _go_context_data attribute.

As far as I can tell, that can't change how the binary is actually linked?

My use-case is using a musl toolchain to cross-compile Linux binaries to put in docker images from a macOS host. We want to statically link, and so want to set static = "on", and want our custom C++ toolchain to be used for the linking, so set pure = "off", but we do so via a select; effectively:

static = select({
  "@io_bazel_rules_go//go/platform:linux": "on",
  "//conditions:default": "auto",
}),
pure = select({
  "@io_bazel_rules_go//go/platform:linux": "off",
  "//conditions:default": "auto",
}),

I tried patching in your linked change instead of mine, and didn't end up with the static binary I'd expect. I think this makes sense, because something needs to be able to apply a transition to the go_binary itself, which necessitates something depending on it (the transition wrapper), or the current approach (a self transition), except the self-transition approach can't see select-ed values.

[fmeum]

To clarify what I think may be a misunderstanding: The commit 089f13e, which is part of #3108, is not meant to fix the issue you reported as #3103. Rather, it is meant to fix bazelbuild/bazel#14626, which at its root is caused by rules_go applying transitions on Go settings to all its attributes, even those that do not reference Go targets (e.g. srcs or cdeps).

What I suggested - if it turns out to work - is a way to simultaneously 1) simplify the current PR and 2) reduce, but not completely remove the need for transitions resetting Go settings on non-Go dependency edges such as srcs and cdeps.

I think this makes sense, because something needs to be able to apply a transition to the go_binary itself...

I think that this part is not necessarily true: If I understand the code correctly, the only way in which rules_go accesses the values of the settings set in the transition is through _go_context_data. Thus, it should be sufficient if go_binary and all its transitive go_library dependencies depend on _go_context_data in the correct configuration. I think that this is guaranteed as long as both deps and _go_context_data itself carry cfg = go_transition - there should be no need for the go_binary itself to be built in this configuration. The only thing that would change if the transitions were moved to the relevant attributes would be the output path of the go_binary, but since all Go settings values are hidden away in the opaque ST hash suffix anyway, that shouldn't matter.

Sorry for the confusion I caused by bringing up the other PR. I hope that the above managed to clarify the situation and my proposal. Please let me know if I should elaborate on something.

[illicitonion]

Aha, that makes a lot more sense, apologies for the confusion!

That sounds great, and definitely a lot cleaner - I gave it a try and it seemed to work for my test-cases. Would you have any interest in pulling those cfg changes into your PR, or should one of us (if so, got a preference?) put together a PR on top of that for once it's merged?

[fmeum]

Glad to hear it worked! Since my PR becomes simpler due to yours and yours should be easier to review, I would suggest that we should aim for getting yours merged first. Since I already have two PRs based on each other (#3005 and #3108) and blocked on a third one (#3113), I would just wait until it got merged and rebase.

[illicitonion]

Closing in favour of the simpler #3116 - thanks @fmeum for the suggestion!