Support expanding locations in rustc_env and build_script_env #2 #468
Conversation
There was a problem hiding this comment.
Works for me!
One implementation detail worth specifically calling out: This expands to an absolute, rather than relative, path for cargo build scripts, because we don't run them in the execroot. I think this is fine, but it's a bit weird.
I will leave a bit of time for others to chime in before merging :)
cargo/cargo_build_script.bzl
Outdated
| @@ -4,6 +4,19 @@ load("@io_bazel_rules_rust//rust:private/rustc.bzl", "BuildInfo", "DepInfo", "ge | |||
| load("@io_bazel_rules_rust//rust:private/utils.bzl", "find_toolchain") | |||
| load("@io_bazel_rules_rust//rust:rust.bzl", "rust_binary") | |||
|
|
|||
| def _expand_location(ctx, env, data): | |||
| if env.startswith("$(execpath ") or env.startswith("$(location "): | |||
There was a problem hiding this comment.
This only does substitution if the start of the env var is a $(...) expression, whereas other rules do so anywhere. I think this is ok, but at some point may bite someone... It would be a shame to write our own parser for the replacement, but maybe doing a contains rather than startswith check, and an env.replace("$(location", "${pwd}/$(location" or similar could be worthwhile
There was a problem hiding this comment.
I suspect non-prefixed usage won't be common, but it was easy enough to change. Also added a note about it being an absolute path to the docstring.
dae
force-pushed
the
location-expansion-tidied
branch
from
d9e4f85 to
fa9a732
Compare
dae
force-pushed
the
location-expansion-tidied
branch
from
fa9a732 to
31f096e
Compare
|
Latest push just corrects some |
| # Optional environment variables passed during build.rs execution. | ||
| # Note that as the build script's working directory is not execroot, | ||
| # execpath/location will return an absolute path, instead of a relative | ||
| # one. |
There was a problem hiding this comment.
Does this work under remote-execution? Is it tested by the ci rbe_ubuntu1604 target?
There was a problem hiding this comment.
Yes, it should work under remote execution - the expansion is done in the cargo_build_script runner within the action.
The build script could opt to do bad things with the path passing it to dependees (though we have some logic in cargo_build_script to try to normalise that too), but within the action this is valid.
- rustc_env now expands $(rootpath //package:target), for passing the location of files that are needed at compile time - build_script_env now expands $(execpath //package:target), for passing the location of files or tools that are needed at build script runtime - cargo_script_build() now passes the data argument to the build script runner, so the data is available at runtime
dae
force-pushed
the
location-expansion-tidied
branch
from
2e3f50f to
10007a8
Compare
|
Rebased on latest master; if there's anything else that needs changing, please let me know. |
This removes the `do_nothing_cargo_build_script()` helper Bazel macro. We now use new support for [`$(execpath)` expansion in the `rustc_env` attribute][1] to help the `include_bytes!()` Rust macro find the right Bazel-generated files. --- [1]: bazelbuild/rules_rust#468
Summary: We pull in bazelbuild/rules_rust#468 to make it easier to build protos, while not pulling in bazelbuild/rules_rust#463, because that would require us to update Bazel to 3.0.0. Happily, the commit that we need merged immediately before the commit that we don’t want! (Yes, this is a wee bit precarious; we’re working on the Bazel upgrade: #4277.) Test Plan: With both Bazel 2.1.0 and Bazel 3.7.0, everything builds, all tests pass, and TensorBoard looks okay enough. wchargin-branch: rules-rust-20201105a wchargin-source: bb3180a3a2a415d800d6a2a925fef0cb4e376d2f
Summary: We pull in bazelbuild/rules_rust#468 to make it easier to build protos, while not pulling in bazelbuild/rules_rust#463, because that would require us to update Bazel to 3.0.0. Happily, the commit that we need merged immediately before the commit that we don’t want! (Yes, this is a wee bit precarious; we’re working on the Bazel upgrade: #4277.) Test Plan: With both Bazel 2.1.0 and Bazel 3.7.0, everything builds, all tests pass, and TensorBoard looks okay enough. wchargin-branch: rules-rust-20201105a
This is #461 squashed into fewer commits - please see that PR for previous discussion.
This potentially closes #459, closes #454, and closes #79.
The docs seem to indicate there's precedent for this in rules_cc:
https://docs.bazel.build/versions/master/be/make-variables.html#predefined_label_variables