Add tools attr with exec transition for cargo_build_script

[djmarcin]

If tools are passed in data, they must be built in the exec configuration in order to run on the host machine when the target != host.

I think this should be a fairly safe change, unless someone is doing something with the tools passed to the build script that would require them to run on the target configuration, but short of copying these tools into the out_dir I don't know what that would be. And -- that would be better handled by depending directly on these tools in the data attr of the library or binary anyway.

[UebelAndre]

This is actually a very interesting find. Lets consider the case of openssl-sys. Which we currently have an example of that builds openssl and passes it to the build script:

rules_rust/examples/sys/complex/raze/remote/BUILD.openssl-sys-0.9.60.bazel

Lines 39 to 76 in fe657ed

	cargo_build_script(
	name = "openssl_sys_build_script",
	srcs = glob(["**/*.rs"]),
	build_script_env = {
	"OPENSSL_DIR": "../openssl/openssl",
	},
	crate_features = [
	],
	crate_root = "build/main.rs",
	data = glob(["**"]) + [
	"@openssl",
	],
	edition = "2015",
	links = "openssl",
	rustc_flags = [
	"--cap-lints=allow",
	],
	tags = [
	"cargo-raze",
	"manual",
	],
	version = "0.9.60",
	visibility = ["//visibility:private"],
	deps = [
	"@rules_rust_examples_complex_sys__autocfg__1_0_1//:autocfg",
	"@rules_rust_examples_complex_sys__cc__1_0_66//:cc",
	"@rules_rust_examples_complex_sys__pkg_config__0_3_19//:pkg_config",
	] + selects.with_or({
	# cfg(target_env = "msvc")
	(
	"@rules_rust//rust/platform:i686-pc-windows-msvc",
	"@rules_rust//rust/platform:x86_64-pc-windows-msvc",
	): [
	"@rules_rust_examples_complex_sys__vcpkg__0_2_11//:vcpkg",
	],
	"//conditions:default": [],
	}),
	)

Assuming rules_foreign_cc supports cross compilation, would this crate expect openssl to be built in the target or exec configuration?

[djmarcin]

I suppose the question is what is the openssl-sys build script doing with the openssl binary? For what it's worth, we do use openssl-sys in our build and it doesn't seem broken to me (yet...)

[djmarcin]

This (or some other mechanism for passing tools in the exec configuration) would solve #884

[illicitonion]

How about if we added a compile_data attribute to cargo_build_script as per #516 ?

[djmarcin]

I believe `tools` is a more idiomatic naming for “things that need to execute during this rule” if another attribute is preferred, e.g. as on genrule https://docs.bazel.build/versions/main/be/general.html#genrule.tools

…

On Wed, Aug 11, 2021 at 2:37 AM Daniel Wagner-Hall ***@***.***> wrote: How about if we added a compile_data attribute to cargo_build_script as per #516 <#516> ? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#885 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA5AF4ODVDOHGW6LJZACQDT4JAF7ANCNFSM5B5BCDDQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email> .

[illicitonion]

Yeah, tools works for me!

[hlopko]

tools sgtm too :)

[djmarcin]

Updated to add a tools attr instead. This is potentially more fixup work in other tools such as cargo raze if they are using data to pass tools that should be built for the exec platform, but certainly better from the perspective of ensuring backwards compatibility.

[illicitonion]

Fantastic - LGTM

One last piece - any chance of a test? I know that in our current execution environments on CI we won't see a difference, but it'd be handy for the future. I imagine adding a one-line rust_binary as a tools to a target and running it in the build script? @hlopko I'm not sure if the unit testing stuff is set up well to test this kind of thing?

[UebelAndre]

Looks good to me! I wonder if tools should be wired into compile_data for the rust_binary? Leaning towards not but would like some other opinions as well 😄

Either way, thank you!

[hlopko]

The PR looks good to me, and as for the test, I guess it would be enough to have a genrule that touches a file (using for example this rule: https://github.com/bazelbuild/bazel-skylib/blob/main/rules/write_file.bzl), and then in the unit test we would check the exec path of the file passed in data and in tools. Like you do in a different PR, exec path of the file in tools should have "exec" in it.

[djmarcin]

Test added, merged main, and documentation re-updated.

[UebelAndre]

@hlopko do these tests look good to you?

[illicitonion]

LGTM, thanks! Over to @hlopko

[hlopko]

Looks good, thank you!

[hlopko]

(please rebase and somebody will merge this right away, thank you!)