Use a transition to resolve the correct versions of crane and yq #590
Conversation
|
I'm torn by this, though i had the same problem myself a lot of times before. I have been told that the exec_groups solve this problem, any way we could use that here? |
I don’t think that’s the case. Those can only be used to control where/how stuff gets executed. But in our case we’re interested in building an action that includes binaries for the right target. |
Unfortunately, exec_groups don't support transitions afaics. |
EdSchouten
force-pushed
the
eschouten/20240523-transition
branch
from
1a587ec to
56519c4
Compare
|
PTAL! |
EdSchouten
force-pushed
the
eschouten/20240523-transition
branch
from
56519c4 to
d8eed58
Compare
|
Whoops! Forgot to add |
|
Hmmm... In the bzlmod case we're unable to access |
|
Something along the lines of Lines 27 to 30 in 6620d46 |
Bazel doesn't provide a very elegant way to specify that a toolchain should be resolved in such a way that it can be executed on a *target* platform: bazelbuild/bazel#19645 To work around this, we can pass in the toolchains through ordinary labels and apply an outgoing edge transition to set --extra_execution_platforms equal to --platforms. This causes the toolchains to be resolved the way we want. Though I think using transitions for this is pretty slick, it's a shame that this has to be done by patching up command line options.
EdSchouten
force-pushed
the
eschouten/20240523-transition
branch
from
d8eed58 to
11cf945
Compare
|
My understanding is that this should be fixed in upstream. See bazel-contrib/bazel-lib#747 and bazelbuild/bazel#19645 (comment) |
|
This doesn't cherry-pick onto 2.x so I think we'll need to take that upstream fix for our 2.0.0 release anyway. thanks for reminding us @BoleynSu |
|
bazel-contrib/bazel-lib#875 is for fixing upstream issue in bazel-lib but it is only for jq and lacks docs/tests. For rules, if jq needs to be run during the actions, we should use |
|
Again, I don’t see a reason why we should declare all toolchains twice, purely to distinguish in what context they end up getting used. |
|
Okay. Let me point out at least one drawback I can think of for your solution. If the tool, e.g., crane is not prebuilt but built by compiling source code. With the transition approache, crane may not build. For example, target=win, exec=linux. With the transition, we will have to build crane with exec=win which won't work if only exec=linux is supported. |
|
Are you sure that's correct...? Given your example, we're only forcing that a toolchain is resolved with both target & exec set to Windows. That doesn't necessarily imply that in order to build dependencies of that toolchain we also apply such a transition. The transition should influence the way the toolchain is resolved. Not the way the toolchain is built. |
|
We are forced to build crane with cfg=win and exec=win. It does not matter what crane's dependency is. A toolchain rule is built in the same way as a normal rule. For example, If you use transition extra_execution_platforms=win to use crane_toolchain_type, you will need to build ":tool" for target="value of --platforms" with extra_execution_platforms=win. I.e, you need to build ":crane" with target="selected execution_platforms for building :tool, which can only be win" (because ":crane" is of cfg=exec in ":tool") and exec="selected execution_platforms for building :crane, which can only be win tool". However, there is no cc_toolchain for that is exec compatible with win. Anyway, if you doubt it, you can try it locally pretty easily. |
Just to check: is this something you are speculating, or is that something that is actually true right now? Because to me it's not a given that this is the case. Changing Furthermore, if that does turn out the be the case, we can patch up the transition to prepend the desired platforms instead of replacing them. Or file an issue against Bazel that they fix up the way transitions work. |
|
If we prepend,
For example, I have not really tried the theory to be honest but it must be true given how transition works. A repro it pretty easy to make. |
Feel free to post one. |
# Helper rule for ensuring that the crane and yq toolchains are actually
# resolved for the architecture we are targeting.
def _transition_to_target_impl(settings, attr):
return {
# String conversion is needed to prevent a crash with Bazel 6.x.
"//command_line_option:extra_execution_platforms": [
str(platform)
for platform in settings["//command_line_option:platforms"]
],
}
_transition_to_target = transition(
implementation = _transition_to_target_impl,
inputs = ["//command_line_option:platforms"],
outputs = ["//command_line_option:extra_execution_platforms"],
)
BinaryInfo = provider(
doc = "Provide info for binary",
fields = {
"bin": "Target for the binary",
},
)
def _toolchain_impl(ctx):
binary_info = BinaryInfo(
bin = ctx.attr.bin,
)
toolchain_info = platform_common.ToolchainInfo(
binary_info = binary_info,
)
return [toolchain_info]
binary_toolchain = rule(
implementation = _toolchain_impl,
attrs = {
"bin": attr.label(
mandatory = True,
allow_single_file = True,
executable = True,
cfg = "exec",
),
},
)
def _resolved_binary_rule_impl(ctx, toolchain_type, template_variable):
bin = ctx.toolchains[toolchain_type].binary_info.bin[DefaultInfo]
out = ctx.actions.declare_file(ctx.attr.name + ".exe")
ctx.actions.symlink(
target_file = bin.files_to_run.executable,
output = out,
is_executable = True,
)
return [
DefaultInfo(
executable = out,
files = bin.files,
runfiles = bin.default_runfiles,
),
platform_common.TemplateVariableInfo({
template_variable: out.path,
} if template_variable != None else {}),
]
def resolved_binary_rule(*, toolchain_type, template_variable = None):
return rule(
implementation = lambda ctx: _resolved_binary_rule_impl(ctx, toolchain_type, template_variable),
executable = True,
toolchains = [toolchain_type],
)
resolved_binary = resolved_binary_rule(toolchain_type = "//:toolchain_type")
def _image_impl(ctx):
return DefaultInfo(
files = ctx.attr.bin[0][DefaultInfo].files,
)
image = rule(
_image_impl,
attrs = {
"_allowlist_function_transition": attr.label(
default = "@bazel_tools//tools/allowlists/function_transition_allowlist",
),
"bin": attr.label(
cfg = _transition_to_target,
default = "//:bin",
),
},
)Please try the above example. |
|
The issue with prepending will also happen with register_execution_platforms in WORKSPACE/MODULE.bazel. Again I did not try it but it should be how transition works. So basically, the current solution is broken I would say. |
|
In my case it failed because it first failed as follows: That's because it tried to run the That I could solve by constraining the registration of Go toolchains, by not registering ones where That said, I agree that this is suboptimal. But it's still not as awful as 'runtime toolchains'. We should simply work with the Bazel developers to get the right set of features added to Bazel to do resolution of toolchains the way we want. |
|
You can replace the genrule with a real file to see other failures. I was using genrule simply because I do not want to add a go file to the example. Or you can add I do not think we need any change in the Bazel core. For our specific use-case and the one posted in bazelbuild/bazel#19645, basically, all we need is X_toolchain_type which provides a binary with cfg="target" instead of "exec" and a resolved_binary where user uses it as deps with cfg="exec" or tools in genrule. I.e it does not really need to be two toolchian types for one binary. It is similar to that we have a |
|
How do we do this with one toolchain type? How will resolution know if we need it for the exec or target platform (when those are not the same)? I agree the exec platform transition propagates through the toolchain transition, and this breaks later actions. Sad face. I think we're back to "no good solution" here. Duplicate toolchains works but isn't "good" (duplicates work for everyone and confuses target vs exec in definition). Exec platform transition breaks deeper builds, and would require further work I'm bazel to prevent that (not clear exactly what, since we've lost information in the transition). |
|
@anguslees See my comments here bazel-contrib/bazel-lib#875 (comment) |
|
@EdSchouten @thesayyn @alexeagle folks be mindful of #706 this is breaking cross cpu compilation, not sure if anyone else is looking into a fix, looks like when crosscompiling from linux/x64 to linux/arm64 it also happens, so it's trivial to add a unit test to validate that things are broken, and then fix from there, I may have some capacity this week, but I can't promise much, but if someone else has capacity feel free to tackle it |
Bazel doesn't provide a very elegant way to specify that a toolchain should be resolved in such a way that it can be executed on a target platform:
bazelbuild/bazel#19645
To work around this, we can pass in the toolchains through ordinary labels and apply an outgoing edge transition to set --extra_execution_platforms equal to --platforms. This causes the toolchains to be resolved the way we want.
Though I think using transitions for this is pretty slick, it's a shame that this has to be done by patching up command line options.