build_script: move to separate dep line from deps

[durin42]

Much like proc_macro_deps, build_script needs to be built for cfg = "exec" rather than as part of what's built for the deployment target. Since there's no way to mark individual dependencies as cfg = "exec" we break the build_script dependency out onto its own line. Only one build script was ever allowed anyway, so once we deprecate putting build scripts in deps we'll get the bonus of making a specific kind of build misconfiguration impossible!

[UebelAndre]

Can you provide an example of the issue you saw without this change? To my knowledge cargo_build_script is running in the correct configuration. The rule is run in “target” but it takes a “script” attribute in exec which is what’s run to produce outputs

[scentini]

We ran into issues using Bazel's compatible_with; we currently need to set many compatible_with to cargo_build_script targets although they are not supposed to be built in target configuration.

We don't actually build everything in the right configuration - deps seems wrong here.

In my ideal world, we would be able to set cfg="exec" to the rule definition itself on both cargo_build_script and proc_macro_deps. But the Bazel configurability team tells me that is not possible.

I've been ponderring on wrapping the proc_macro_deps rule in a macro that does the "exec" transition so that we don't have to have the separation of deps and proc_macro_deps. We could do the same for cargo_build_script, but it seems different enough that it warrants its own attribute?

[durin42]

@UebelAndre @scentini this is ready for review

[UebelAndre]

Can you provide an example of the issue you saw without this change? To my knowledge cargo_build_script is running in the correct configuration. The rule is run in “target” but it takes a “script” attribute in exec which is what’s run to produce outputs

Was this comment addressed?

[scentini]

Hey @UebelAndre, I tried to address your comment above - we are forced to use compatible_with on many cargo_build_script targets although they are not supposed to be relevant to target configuration.

[UebelAndre]

Hey @UebelAndre, I tried to address your comment above - we are forced to use compatible_with on many cargo_build_script targets although they are not supposed to be relevant to target configuration.

Ah sorry, seemed like kinda an aside to the ticket itself 😅

First, would you have some detailed documentation on compatible_with? I don't think I understand it's use in comparison to target_compatible_with or exec_compatible_with.

Second, I agree cargo_build_script.deps seems wrong. I thought build scripts were built for the execution platform, then used to generate things for the target platform. Doesn't this mean that conceptually, having a rust_binary as cargo_build_script.runner in the exec configuration solve for this? Is deps the only thing that needs to be fixed? (that's likely not true and feel learning more about compatible_with would help me realize that).

[scentini]

Sorry for the delayed response!

Unfortunately I couldn't find much documentation of compatible_with, except that the underlying mechanism is not documented: https://github.com/bazelbuild/bazel/issues/10849. Looks like it might be on its way out.

The issue ended up being in only the deps being built in target mode: The compatible_with propagated through them just as target_compatible_with would do, so if we have a cargo_build_script that has a compatible_with values, all the deps need to set compatible_with too. I am preparing a PR for cfg = "exec" on deps over at https://github.com/bazelbuild/rules_rust/pull/1561, which would unblock us.

I believe that this PR still makes sense though; build scripts are a special type of dependency; one can have only one of them and having an attribute for them would make it more explicit. WDYT?

[UebelAndre]

I believe that this PR still makes sense though; build scripts are a special type of dependency; one can have only one of them and having an attribute for them would make it more explicit. WDYT?

Sounds reasonable but I think we should get more buy in if cargo_build_script is going to become more a part of the core rules. cc @illicitonion @krasimirgg

[krasimirgg]

cargo_build_script

[krasimirgg]

(sorry accidentally closed the pr when trying to post a comment)

I think this PR makes sense for the reasons @scentini points out -- they are just too different than standard dependencies and separating them out makes this more clear.

[illicitonion]

Works for me 👍

[UebelAndre]

I think this PR makes sense for the reasons @scentini points out -- they are just too different than standard dependencies and separating them out makes this more clear.

I'm not sure my question was clear. There have been conversations in the past around whether or not cargo_build_script should be a part of the //rust package, which to me translates to, "should the Bazel rules for building Rust code need to re-implement this build script feature from Cargo". I had thought the consensus was "no" and that the Bazel rules should only concern itself with wiring up calls to rustc to compile code. This change makes changes to the core rust rules, which grow the footprint of cargo_build_script which is the opposite direction I expected. My question isn't "do we think the change makes sense" but "does everyone agree that we should be changing the core rust rules to account for this re-implementation of a Cargo feature?"

Is that clear to everyone?

[illicitonion]

My take is that BuildInfo is already encoding that cargo-specific logic in the core of rules_rust, so the real questions are:

1. Are we happy making BuildInfo public? Right now it only lives in rust/private/providers.bzl and isn't "public". I guess we could add the new attribute but leave the provider private, it's a bit weird, but is effectively the status quo.
2. Do we want to name our new attribute in a cargo-centric way or a more neutral way? build_script seems generic enough to me, but cargo_build_script would lean into the cargo-centricity, I'm sure we could come up with something even more generic if we wanted to.
3. Do we want to encode the assumption from cargo that there's never more than one BuildInfo-providing target per target? (i.e. should build_script take a label or a label_list). I don't really mind.

[krasimirgg]

Chatted a bit with @scentini:

• the BuildInfo feels like part of core rules_rust; it's important at least for integration with third party dependencies / the crate universe / etc
• looks like BuildInfo is at an abstraction that can support additional mechanisms beyond cargo that provide addtional build information. It's of course heavily inspired by cargo and right now in rules_rust we only have cargo as example, but we can "keep the door open" for other build info providers via this info and the rest of rules_rust should "just work" with it then.
• it would make sense to have the name of the new attribute be more neutral build_script
• there's already a rustc check that we only support at most 1 of these here: https://github.com/bazelbuild/rules_rust/blob/f0cdcedc2a68ffdb3048a8510b380e64eb9a1d72/rust/private/rustc.bzl#L269 I think it would make most sense to keep this assumption of at most 1 for these, it seems like a simple model to think about; if we ever have a compelling reason to allow multiple we can relax it then.