Use edge transitions not self transitions #3130

illicitonion · 2022-04-25T13:59:07Z

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 · 2022-04-25T14:40:35Z

/cc @uhthomas @fmeum

achew22 · 2022-05-04T17:58:05Z

@fmeum, how much does this overlap with your other transition PRs?

fmeum · 2022-05-04T18:08:13Z

@fmeum, how much does this overlap with your other transition PRs?

It does overlap in terms of merge conflicts, but the goals are completely separate. When in doubt, merge this PR first as it is certainly more complicated than mine and would require more effort to rebase.

linzhp · 2022-05-04T19:43:14Z

@achew22 let me know when you finish your review and want me to test in Uber.

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. Fixes bazelbuild#3103.

illicitonion · 2022-05-23T14:02:15Z

@achew22 I've rebased this past #3143 - would you have some time to take a look? It's gotten smaller as a result of the rebasing :)

fmeum

Only while reviewing did I gain some much-needed clarity on the way to configurable attributes. @illicitonion Sorry for all the back and forth on this!

Currently, no attributes are configurable since we use a self transition. With this PR, all attributes would beome configurable, with potentially confusing or at least unclear semantics in the case where goos/goarch is set and there is also a select on a constraint setting.

At the same time, while technically sound, this PR adds quite a bit of complexity: A new wrapper (around a wrapper), including the need to forward providers and symlink executables.

@uhthomas @linzhp Given the fact that the alternate approach of #3116 would achieve mostly the same by deleting complex code, I would like to hear your thoughts on the following plan:

Merge Transition on edges not self #3116, but with a small addition: Keep a macro wrapper around go_binary that fails the build if goos/goarch are set and any other attribute is a select expression. Since select expressions didn't work before, this does not affect backwards compatibility - it merely restricts a new feature to an (IMO) reasonable subset of its full scope.
Think about ways to replace goos/goarch in a way that doesn't complicate go_binary. For example, we could introduce a go_switch_os_arch rule that handles the platform transition for a single go_binary in its attributes.
Deprecate and eventually remove goos/goarch as Bazel platform support matures further.

fmeum · 2022-05-26T19:32:52Z

go/private/rules/binary.bzl

-go_transition_binary = go_transition_rule(executable = True, **_go_binary_kwargs)
-go_non_executable_transition_binary = go_transition_rule(executable = False, **_go_binary_kwargs)
+go_binary = rule(executable = True, **go_binary_kwargs)
+go_non_executable_transition_binary = rule(executable = False, **go_binary_kwargs)


Suggested change

go_non_executable_transition_binary = rule(executable = False, **go_binary_kwargs)

go_non_executable_binary = rule(executable = False, **go_binary_kwargs)

fmeum · 2022-05-26T19:33:02Z

go/private/rules/binary.bzl

+    }),
+    executable = True,
+)
+go_non_executable_transition_transition_binary = rule(


Suggested change

go_non_executable_transition_transition_binary = rule(

go_non_executable_transition_binary = rule(

fmeum · 2022-05-26T19:34:08Z

go/private/rules/binary.bzl

+)
+go_non_executable_transition_transition_binary = rule(
+    implementation = forward_through_transition_impl,
+    attrs = dicts.add(transition_attrs, {


implementation and attrs could be extracted into e.g. _go_transition_binary_kwargs.

fmeum · 2022-05-26T19:39:45Z

go/private/rules/binary.bzl

@@ -165,11 +167,19 @@ def _go_binary_impl(ctx):
        ccinfo = cc_common.merge_cc_infos(
            cc_infos = [ccinfo] + [d[CcInfo] for d in source.cdeps],
        )
-        providers.append(ccinfo)
+        providers_to_forward.append(ccinfo)


Maybe leave this as providers as these providers are still forwarded only in addition to being advertised by the rule itself?

fmeum · 2022-05-26T19:40:49Z

go/private/rules/binary.bzl

+    implementation = forward_through_transition_impl,
+    attrs = dicts.add(transition_attrs, {
+        "is_windows": attr.bool(),
+        "transition_dep": attr.label(cfg = go_transition),


Given that this is only an internal attribute, we could help the IntelliJ plugin here by giving it a name recognized by its aspect, see fmeum/rules_meta@8d57be3.

fmeum · 2022-05-26T19:41:21Z

go/private/rules/test.bzl

@@ -168,13 +191,8 @@ def _go_test_impl(ctx):
    # source file is present, Bazel will set the COVERAGE_OUTPUT_FILE
    # environment variable during tests and will save that file to the build
    # events + test outputs.
-    return [
+    providers_to_forward = [


See above, could stay as providers.

fmeum · 2022-05-26T19:52:38Z

tests/core/go_binary/configurable_attribute_test.go

+go_binary(
+    name = "configurable_srcs",
+    srcs = select({
+        "@io_bazel_rules_go//go/platform:darwin": ["main_darwin.go"],


This example contains exactly what worries me a bit about this PR: The use of goos changes the platform, but then another attribute is set based on a select on the platform. It is not immediately obvious in which "order" these are evaluated. Do you see a concrete use case for this pattern? If not, maybe we should make goos/goarch explicitly non-configurable by failing in the wrapper macro if they are set to a type that equals type(select({"//conditions:default": True})).

fmeum · 2022-05-26T20:06:09Z

go/private/rules/transition.bzl

@@ -456,3 +497,38 @@ def _set_ternary(settings, attr, name):
        label = filter_transition_label("@io_bazel_rules_go//go/config:{}".format(name))
        settings[label] = value == "on"
    return value
+
+def _symlink_file_to_rule_name(ctx, src):
+    output_file_name = ctx.label.name + (".exe" if ctx.attr.is_windows else "")


AFAIK go_binary has an out attribute that we should probably take into account here.

fmeum · 2022-05-26T20:06:55Z

go/private/rules/transition.bzl

+
+def _symlink_file_to_rule_name(ctx, src):
+    output_file_name = ctx.label.name + (".exe" if ctx.attr.is_windows else "")
+    dst = ctx.actions.declare_file(paths.join(ctx.label.name, output_file_name))


Is this the same logic that go_binary uses today? Ideally, the new executable would have the same name is that used go_binary before this PR to retain compatibility with hard-coded paths to outputs.

fmeum · 2022-05-26T21:52:46Z

go/private/rules/transition.bzl

+    if file == None:
+        return default_info
+
+    data_runfiles = default_info.data_runfiles.merge(ctx.runfiles([file]))


Is it correct that the original executable will still be in the runfiles? On Windows, this may be significant as the symlink may end up being a full copy.

fmeum · 2022-06-02T07:36:28Z

@illicitonion I discussed this with @achew22 and @linzhp and we arrived at the conclusion that merging #3116 with the small addition noted in #3130 (review) is the preferred solution for both rules_go users and maintainers: It prevents users from introducing confusing interactions with platforms and essentially implements a new feature by deleting complicated code, which is nice.

illicitonion · 2022-06-14T12:29:32Z

Closing in favour of #3116

illicitonion mentioned this pull request Apr 25, 2022

Transition on edges not self #3116

Merged

illicitonion force-pushed the edge-transitions-reviewable branch from 79da723 to 95331e7 Compare April 25, 2022 14:30

illicitonion force-pushed the edge-transitions-reviewable branch from 95331e7 to 8ba8b61 Compare April 26, 2022 09:23

achew22 requested review from achew22 and linzhp May 4, 2022 18:20

illicitonion mentioned this pull request May 9, 2022

Transitions can inspect configurable attributes, but don't see a value for them bazelbuild/bazel#15157

Closed

Add tests

cd744bd

illicitonion force-pushed the edge-transitions-reviewable branch 3 times, most recently from 56bbbb2 to cbc9a5c Compare May 23, 2022 13:30

illicitonion force-pushed the edge-transitions-reviewable branch from cbc9a5c to 4077a66 Compare May 23, 2022 13:47

fmeum reviewed May 27, 2022

View reviewed changes

fmeum mentioned this pull request Jun 3, 2022

Include go_transition_test in bazel aspect #3160

Merged

illicitonion closed this Jun 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use edge transitions not self transitions #3130

Use edge transitions not self transitions #3130

illicitonion commented Apr 25, 2022

illicitonion commented Apr 25, 2022

achew22 commented May 4, 2022

fmeum commented May 4, 2022

linzhp commented May 4, 2022

illicitonion commented May 23, 2022

fmeum left a comment •

edited

Loading

fmeum May 26, 2022

fmeum May 26, 2022

fmeum May 26, 2022

fmeum May 26, 2022

fmeum May 26, 2022

fmeum May 26, 2022

fmeum May 26, 2022

fmeum May 26, 2022

fmeum May 26, 2022

fmeum May 26, 2022

fmeum commented Jun 2, 2022

illicitonion commented Jun 14, 2022

	go_non_executable_transition_binary = rule(executable = False, **go_binary_kwargs)
	go_non_executable_binary = rule(executable = False, **go_binary_kwargs)

	go_non_executable_transition_transition_binary = rule(
	go_non_executable_transition_binary = rule(

Use edge transitions not self transitions #3130

Use edge transitions not self transitions #3130

Conversation

illicitonion commented Apr 25, 2022

illicitonion commented Apr 25, 2022

achew22 commented May 4, 2022

fmeum commented May 4, 2022

linzhp commented May 4, 2022

illicitonion commented May 23, 2022

fmeum left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmeum commented Jun 2, 2022

illicitonion commented Jun 14, 2022

fmeum left a comment •

edited

Loading