-
Notifications
You must be signed in to change notification settings - Fork 4k
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
No way to use toolchains correctly in executable rule output #19645
Comments
Any updates on this? |
I'm very confused about your use case, hopefully we can figure out what makes sense. Typically, the intent of toolchains is that they are used during build actions: a c++ compiler is called, an analyzer is run, whatever. It sounds like you want some files from a toolchain, but you want your build rule to bundle those into the outputs? We've used toolchains for that in a few cases (the Python runtime is probably the best example), but it's always been a tricky fit, because it's using the tools as an output to be passed along. Maybe you don't want a toolchain, you want a direct (target configured) dependency on the tools, so that they can be bundled into your rule's output? |
I agree, but see my numerous linked examples in the footnote of the opening post. This mis-pattern is quite widespread, because folks (apparently) want to use toolchains in tests or bazel-run scripts (ie: the output of executable rules). This is a feature request for an improvement to bazel toolchains/exec_groups to make it possible to resolve toolchain for a rule's target/parent. Or alternatively we loudly declare this will 'never' be possible, and (as a community), we go and rewrite these examples to avoid toolchains. I'm ok with either path to address this issue - right now I'm kind of stuck waiting for a decision. Folks "expect" this to work, but it doesn't - and if I'm going to delete their toolchain then I'd at least like to be able to link to this PR (or some other doc) as evidence for why that was the correct change to their rules. |
Toolchains (uniquely) have two options for their dependencies:
So part of the solution is probably to be more careful of the target/exec distinction, here. It's unfortunately easy to get this wrong in the default case, since the base case is that the host platform is used as both the only exec platform and as the default target platform. I agree that there is probably more we can be doing to make this cleaner. I'm going to add this item to our backlog for tracking purposes. If you have any specific features you'd like to request, we'd be happy to see a proposal or further discussion to move this along. |
I think we're talking past each other. I (believe I am) familiar with cfg=target/cfg=exec and when to use them. This does not help with the problem described in this issue. I'll explain one example here in detail, to see if that helps: rules_oci has a target that can be used similar to This executable rule in this case is the following: def _impl(ctx):
crane = ctx.toolchains["@rules_oci//oci:crane_toolchain_type"]
yq = ctx.toolchains["@aspect_bazel_lib//lib:yq_toolchain_type"]
if not ctx.file.image.is_directory:
fail("image attribute must be a oci_image or oci_image_index")
_, _, _, maybe_digest, maybe_tag = util.parse_image(ctx.attr.repository)
if maybe_digest or maybe_tag:
fail("`repository` attribute should not contain digest or tag. got: {}".format(ctx.attr.repository))
executable = ctx.actions.declare_file("push_%s.sh" % ctx.label.name)
files = [ctx.file.image]
substitutions = {
"{{crane_path}}": crane.crane_info.binary.short_path,
"{{yq_path}}": yq.yqinfo.bin.short_path,
"{{image_dir}}": ctx.file.image.short_path,
"{{fixed_args}}": " ".join(_quote_args(["--repository", ctx.attr.repository])),
}
if ctx.attr.remote_tags:
files.append(ctx.file.remote_tags)
substitutions["{{tags}}"] = ctx.file.remote_tags.short_path
ctx.actions.expand_template(
template = ctx.file._push_sh_tpl,
output = executable,
is_executable = True,
substitutions = substitutions,
)
runfiles = ctx.runfiles(files = files)
runfiles = runfiles.merge(yq.default.default_runfiles)
runfiles = runfiles.merge(crane.default.default_runfiles)
return DefaultInfo(executable = executable, runfiles = runfiles)
# Refactored slightly from the original for clarity
oci_push = rule(
implementation = _impl,
attrs = _attrs,
toolchains = [
"@rules_oci//oci:crane_toolchain_type",
"@aspect_bazel_lib//lib:yq_toolchain_type",
],
executable = True,
) To summarise the relevant parts: this rule gets the paths for two tools ( But this use of toolchains is incorrect when cross-compiling! Specifically: When bazel host platform != exec platform (eg: run bazel on macos with a linux RBE cluster), the rule implementation above will run on the exec (linux) platform, but the output executable needs to work on the host (macos) platform. The toolchains will incorrectly resolve to the exec (linux) platform, and the executable output will be a script that points to linux tools that will fail when run on macos. The correct But there is NO WAY to resolve the toolchain for the target of a rule! ^^This is the bazel limitation described in this github issue. If there is anything I can do to expand or clarify the above explanation, please suggest it because it seems to be subtle/difficult for folks to recognise the issue here and I'd like to point folks to this comment for enlightenment. Afaik cfg=target is not relevant here, since there is nowhere to put that that will affect toolchain resolution - the Afaics this incorrect use (or misunderstanding) of toolchains is moderately common (see my examples in widely used rules), and can impact both bazel-run and bazel tests (both executable rules). Ideally, we improve bazel so that toolchains can be used in these important use cases. |
This looks very related to #21805, where I suggest the |
Related, but not the same - just to prevent an overzealous close-as-duplicate. I agree |
Hey @anguslees, I did some experimenting, and it looks like what you want is possible by doing the following:
The outgoing edge dependency can make use of the following transition: def _transition_exec_to_target_impl(settings, attr):
return {
"//command_line_option:extra_execution_platforms": settings["//command_line_option:platforms"],
}
transition_exec_to_target = transition(
implementation = _transition_exec_to_target_impl,
inputs = ["//command_line_option:platforms"],
outputs = ["//command_line_option:extra_execution_platforms"],
) I will send out a PR against rules_oci soon to get That said, I do think that there is a bit of voodoo magic going on. Transitioning on command line flags is a bit funky. Especially because we're doing it on |
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 move the toolchains into a small helper rule named oci_push_tools(). We can use an outgoing edge transition on the oci_push() side to set --extra_execution_platforms equal to --platforms, thereby causing 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.
Oh wow - I remember considering this, but I think I must have just incorrectly concluded that transitioning Fwiw on bazel 6.4.0, I needed to force the //command_line_option:platforms labels to strings first: def _transition_exec_to_target_impl(settings, attr):
return {
"//command_line_option:extra_execution_platforms": [str(p) for p in settings["//command_line_option:platforms"]],
} Combined with the |
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.
Instead of using a transition to add new execution platforms, consider using a flag to activate them (once I sit down and implement the proposal, hopefully this week). |
Hey @katre, Even though that proposal you linked looks interesting, I don’t really understand how it’s relevant in this context. In this specific case we want to force toolchain resolution so that the resulting toolchain can be executed on whatever the current target platform is. I don’t think that can be solved by conditionally enabling registration of platforms. |
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.
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.
You're right, I misunderstood your workaround, sorry about that. |
This can be solved by defining two toolchain_type for the toolchain. |
Are those the toolchains that intentionally have exec/target_compatible_with set the wrong way around? I’ve never been a fan of that approach. |
I don't know how Python does this, but I'm pretty sure that the Java setup is correct (although a bit complicated): Line 11 in 2fb8564
|
@EdSchouten there is no hack. Using transition for this is more hacky to me. Basically, toolchain_type(name="tool")
toolchain_type(name="runtime")
runtime_rule = rule(binary = label(cfg = "target"))
tool_rule = rule(binary = label(cfg = "exec"))
runtime_rule(name="jq_amd64", binary = "@jq_linux_amd64")
toolchain(name="jq_amd64", toolchain = "jq_amd64", target_compatible_with = linux_amd64, toolchain_type=runtime)
tool_rule(name="tool_jq_amd64", binary = "@jq_linux_amd64")
toolchain(name="tool_jq_amd64", toolchain = "tool_jq_amd64", exec_compatible_with = linux_amd64, toolchain_type=tool) You can then use runtime for outputs and tool for build actions. |
The above is also what I proposed in aspect-build/bazel-lib#747 which is to solve the exactly the same issue you want to solve in bazel-contrib/rules_oci#590 |
The way
Furthermore, how would you deal with the case where you actually care about a true target platform? What if I'm on a Linux machine, and I would like to I also don't think it's reasonable to request that authors of toolchains declare all of their toolchains in twofold, just to distinguish the case where it's called as part of a build action, or as part of Note that I'm not saying that the transition that I wrote is ideal. All I'm saying is that it's at least more correct and more flexible than having so-called 'runtime toolchains'. |
For the runtime toolchain, the binary is only supposed to be used in the
output and should not be used to run actions. Unless the output is used as
exec cfg by other rules. In this case, the exec cfg of the other rule will
be the target cfg of this runtime toolchain.
Sent from https://boleyn.su/phone
…On Sat, Jun 1, 2024, 01:37 Ed Schouten ***@***.***> wrote:
@BoleynSu <https://github.com/BoleynSu>
runtime_rule(name="jq_amd64", binary = ***@***.***_linux_amd64")
toolchain(name="jq_amd64", toolchain = "jq_amd64", target_compatible_with = linux_amd64, toolchain_type=runtime)
The way toolchain() is called, this is basically saying: "Behold! Here is
a copy of jq that can be executed on any platform, and is capable of
targeting Linux." This is, in my opinion, incorrect for two reasons:
- That copy of jq can't be executed on any platform. For example, my
Mac is unable to run it.
- jq doesn't support the notion of a target platform. It is a tool
that is intended to perform queries against JSON data. It is not capable of
emitting any machine code for Linux amd64.
Furthermore, how would you deal with the case where you actually care
about a true target platform? What if I'm on a Linux machine, and I would
like to bazel build a shell script to be run on OpenBSD that ships with a
C/C++ compiler that targets Sun Solaris? There is no
target_of_target_compatible_with that we can set.
I also don't think it's reasonable to request that authors of toolchains
declare all of their toolchains in twofold, just to distinguish the case
where it's called as part of a build action, or as part of bazel run.
That's why I'm calling it a hack.
—
Reply to this email directly, view it on GitHub
<#19645 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXVSKGC6DSFQBKRUFIFDADZFCYOXAVCNFSM6AAAAAA5IRL5TSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBSG4YDKNBUG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@EdSchouten jq_binary = rule(use jq_runtime_toolchain's binary as output)
jq_binary(name="jq") # if you build :jq, you will get a binary for your target platform
genrule( # if you build :run_jq, it will use a binary for you exec platform
name="run_jq",
tools=["jq"]
)
# for bazel rules impl
read_jq_file = rule(use jq_tool_toolchain's binary to read ".data" from src and write to out)
read_jq_file(name="read", src="input", out="output") # This will correctly use the right binary as well. |
Thus, this is correct, because the toolchain (runtime one) is supposed to be (exec-)usable on any platform. The toolchain does not execute any action at all, it only produce a binary that is compatible with linux_amd64. You can think of it as there is a magic binary that works on any platfrom and it will always output a binary of jq that is compatible with linux_adm64. |
Yes. I tried to respond to this solution in my original issue description:
I think those objections still stand. You're basically suggesting that every "tool" toolchain should declare/register two toolchains, since the toolchain providers can't anticipate whether a consumer is going to want it as an "exec" or a "target" toolchain. I agree that works; in composing this feature request, I felt this was unacceptably onerous on the entire ecosystem - or at least requires a "decision" that this is how we want to use toolchains, that we can point to and rally behind. Note: An aside for others casually skimming this discussion:
|
The toolchain_files() and current_rust_toolchain() rules return files for the current 'exec' platform. This is perfecly reasonable if you want to use these tools as part of ctx.actions.run(). But if you want to use these as part of 'data = []' (runfiles), they must be built for the target. This change adds a helper rule, toolchain_files_for_target(), which can take the aforementioned rules as an input, transitioning it so that it yields files for the target platform. I'm not happy with how this rule is implemented, but there is little we can do about that until bazelbuild/bazel#19645 is addressed.
The toolchain_files() and current_rust_toolchain() rules return files for the current 'exec' platform. This is perfecly reasonable if you want to use these tools as part of ctx.actions.run(). But if you want to use these as part of 'data = []' (runfiles), they must be built for the target. This change adds a helper rule, toolchain_files_for_target(), which can take the aforementioned rules as an input, transitioning it so that it yields files for the target platform. I'm not happy with how this rule is implemented, but there is little we can do about that until bazelbuild/bazel#19645 is addressed.
The toolchain_files() and current_rust_toolchain() rules return files for the current 'exec' platform. This is perfecly reasonable if you want to use these tools as part of ctx.actions.run(). But if you want to use these as part of 'data = []' (runfiles), they must be built for the target. This change adds a helper rule, toolchain_files_for_target(), which can take the aforementioned rules as an input, transitioning it so that it yields files for the target platform. I'm not happy with how this rule is implemented, but there is little we can do about that until bazelbuild/bazel#19645 is addressed.
The toolchain_files() and current_rust_toolchain() rules return files for the current 'exec' platform. This is perfecly reasonable if you want to use these tools as part of ctx.actions.run(). But if you want to use these as part of 'data = []' (runfiles), they must be built for the target. This change adds a helper rule, toolchain_files_for_target(), which can take the aforementioned rules as an input, transitioning it so that it yields files for the target platform. I'm not happy with how this rule is implemented, but there is little we can do about that until bazelbuild/bazel#19645 is addressed.
The toolchain_files() and current_rust_toolchain() rules return files for the current 'exec' platform. This is perfecly reasonable if you want to use these tools as part of ctx.actions.run(). But if you want to use these as part of 'data = []' (runfiles), they must be built for the target. This change adds a helper rule, toolchain_files_for_target(), which can take the aforementioned rules as an input, transitioning it so that it yields files for the target platform. I'm not happy with how this rule is implemented, but there is little we can do about that until bazelbuild/bazel#19645 is addressed.
Description of the feature request:
A common pattern1 I see in rules is toolchain declarations that effectively capture a tool path, and templated executable/test rules that resolve the toolchain and substitute the tool path into the executable file output.
Unfortunately this is incorrect! When the exec and target platforms differ, the toolchain will be resolved for the exec platform, but the executable/test output needs to be for the target platform.
As far as I can see, there is no way to make toolchain resolution / platform transitions do the correct thing here. Since executable rules are how bazel tests work, this is a pretty major gap in the toolchains/platforms story. I can't file bugs against rules, because there is no reasonable solution to suggest. This is a feature request for a solution that is (a) correct, and also ideally (b) easy/obvious.
One onerous current workaround is to declare two toolchains for all such cases. One that uses exec_compatible_with/target_compatible_with as usual, and is used when the toolchain is invoked by an action directly. And a second toolchain that uses target_compatible_with only, which can be used in executable rule output. This is moderately terrible, since this duplicates work for everybody, confuses the meaning of exec_compatible_with vs target_compatible_with, and toolchain authors can't anticipate when their toolchain needs to be used in this way - undoing the loose coupling between author and consumer that is kind of the point of toolchains.
An incorrect (afaics) version of the two-toolchains approach is to only declare target_compatible_with toolchain, then create a "current_foo_toolchain" rule that resolves the toolchain for a particular target, and returns the toolchain provider through a rule attribute - see eg python toolchain. This is incorrect in general because another toolchain that depends on this toolchain cannot be resolved at the same time for the same exec+target combination (ie:
toolchains=[foo]
with a dependency on another rule withtoolchains=[bar]
, is not the same astoolchains=[foo, bar]
).As a possible implementation, I'd love to be able to do
exec_group(cfg='target')
, since I thinkctx.exec_groups["target"].toolchains
is an intuitive and 'obvious' way to address the above.Another option is to not use toolchains for what are effectively artifacts. Rule authors obviously like the loose coupling between toolchain declaration and use however, so this potential path would require widespread advocacy/documentation, and perhaps a canonical meta-library for an "artifact toolchain".
See also slack thread: https://bazelbuild.slack.com/archives/CA31HN1T3/p1690176577746239
Which category does this issue belong to?
Core
What underlying problem are you trying to solve with this feature?
It should be possible to resolve and refer to the correct toolchain for an executable rule output.
Which operating system are you running Bazel on?
macos and Linux
What is the output of
bazel info release
?6.3.2
If
bazel info release
returnsdevelopment version
or(@non-git)
, tell us how you built Bazel.No response
What's the output of
git remote get-url origin; git rev-parse master; git rev-parse HEAD
?No response
Have you found anything relevant by searching the web?
No response
Any other information, logs, or outputs that you want to share?
No response
Footnotes
Examples of (incorrect) toolchains used for executable rules in rules_js, rules_docker, rules_oci, gazelle ↩
The text was updated successfully, but these errors were encountered: