Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't configure static differently for different platforms #3103

Closed
illicitonion opened this issue Apr 5, 2022 · 6 comments · Fixed by #3116
Closed

Can't configure static differently for different platforms #3103

illicitonion opened this issue Apr 5, 2022 · 6 comments · Fixed by #3116

Comments

@illicitonion
Copy link
Contributor

What version of rules_go are you using?

0.31.0

What version of Bazel are you using?

5.1.0

Does this issue reproduce with the latest releases of all the above?

Yes

What operating system and processor architecture are you using?

macOS x86-64

Any other potentially useful information about your toolchain?

Using musl to cross-compile to Linux x86-64.

What did you do?

Trying to ensure that binaries built for Linux are statically linked, and for Darwin are not.

Writing code along the lines of:

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

What did you expect to see?

Expect this to lead to a static binary on Linux and a dynamic one on macOS.

What did you see instead?

A dynamic binary is produced on both platforms. This appears to be because of bazelbuild/bazel#15157 - this transition inspects attr.static to determine whether to statically link, but because the attribute is a configurable one, the transition doesn't see the value for the attribute at all, so defaults to "auto". It's explicitly not supported to consult configurable attributes in a transition (and should probably become an error), but it feels like being able to have platform-configurable static-ness is a pretty reasonable feature request.

It feels like there should be a way to do this attribute resolution which doesn't involve a transition, but I'm not familiar enough with the internals of rules_go to know what that would look like. Do folks have ideas for how we could make this static attribute stickier?

I'm happy to put in the work if folks have ideas for what this may look like...

@fmeum
Copy link
Member

fmeum commented Apr 5, 2022

As bazelbuild/bazel#15157 only applies to incoming edge transitions, maybe the solution is to move rules_go's transitions to the relevant outgoing edges (e.g. deps)?

@illicitonion
Copy link
Contributor Author

To make sure I understand, that means that rather than using go_transition_wrapper to conditionally generate a go_binary or go_transition_binary, we'd need to unconditionally generate a wrapper target which had an unconditional dep on a go_transition_binary, and that transition would be where the current go_transition_impl logic would have to happen?

@fmeum
Copy link
Member

fmeum commented Apr 7, 2022

To make sure I understand, that means that rather than using go_transition_wrapper to conditionally generate a go_binary or go_transition_binary, we'd need to unconditionally generate a wrapper target which had an unconditional dep on a go_transition_binary, and that transition would be where the current go_transition_impl logic would have to happen?

I'm not intimately familiar with the internal rules_go setup, but I would have thought of the following: In go_binary, unconditionally set cfg on deps (and possibly the go_context if that's needed by the framework) to a transition that essentially does the same logic as the incoming transition on a go_transition_binary does today. Since the transition is now applied on an outgoing edge, it should be able to read the attributes of the go_binary just fine. This would also get rid of the transition wrapper, which shouldn't be needed: Transitions that do not cause any settings to change their values are true no-ops.

illicitonion added a commit to illicitonion/rules_go that referenced this issue Apr 11, 2022
As per bazel-contrib#3103 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.
illicitonion added a commit to illicitonion/rules_go that referenced this issue Apr 11, 2022
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 bazel-contrib#3103.
illicitonion added a commit to illicitonion/rules_go that referenced this issue Apr 25, 2022
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 bazel-contrib#3103.
@fmeum
Copy link
Member

fmeum commented May 7, 2022

Thinking a bit more about this: What would you expect to happen if the go_binary with a select on the platform also has goarch/goos set? I'm asking because the answer is most likely "evaluate the select based on the result of that platform change", but then that means we would like Bazel to resolve the select after some parts of an incoming transition have been applied.

@illicitonion
Copy link
Contributor Author

What would you expect to happen if the go_binary with a select on the platform also has goarch/goos set?

I don't think it makes sense to have selects based on platform if you're hard-coding a platform.

As a concrete example, I don't think it makes sense to write:

go_binary(
    name = "bin",
    goos = "linux",
    goarch = "amd64",
    static = select({
        "@io_bazel_rules_go//go/platform:linux": "on",
        "//conditions:default": "off",
    }),
)

If you're hard-coding goos, it doesn't make sense to pretend that other attrs are dynamic.

Stepping back into the bigger picture, the goos and goarch attributes probably shouldn't actually exist; Bazel's platform support should be used instead, but it's currently not very ergonomic to do so and so they make sense for now. But I don't think we should give them much weight when considering other platform/toolchain-related features.

@fmeum
Copy link
Member

fmeum commented May 9, 2022

Stepping back into the bigger picture, the goos and goarch attributes probably shouldn't actually exist; Bazel's platform support should be used instead, but it's currently not very ergonomic to do so and so they make sense for now. But I don't think we should give them much weight when considering other platform/toolchain-related features.

Fully agree here. I was just asking a "provocative" question so that we can pin down the intended semantics. goos and goarch should not be factored in there as they should become redundant as platform support matures.

illicitonion added a commit to illicitonion/rules_go that referenced this issue May 23, 2022
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 bazel-contrib#3103.
illicitonion added a commit to illicitonion/rules_go that referenced this issue May 23, 2022
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 bazel-contrib#3103.
illicitonion added a commit to illicitonion/rules_go that referenced this issue May 23, 2022
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 bazel-contrib#3103.
illicitonion added a commit to illicitonion/rules_go that referenced this issue May 23, 2022
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 bazel-contrib#3103.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants