Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support expanding locations in rustc_env and build_script_env #461

Closed
wants to merge 9 commits into from
Closed

Support expanding locations in rustc_env and build_script_env #461

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f3d1552
$(location ...) expansion and runtime data support for build scripts
dae Oct 25, 2020
e37b492
rely on build script to locate bazel-out parent
dae Oct 25, 2020
6f5dbca
add test for source file access as well
dae Oct 25, 2020
41484f5
add run_in_execroot flag to cargo_build_script()
dae Oct 25, 2020
14e4b8b
Regenerate documentation
dae Oct 25, 2020
cb10fec
add note about build_script_env being preferable for build scripts
dae Oct 25, 2020
95fd967
prepend execroot automatically in build script runner
dae Oct 26, 2020
8158d14
Regenerate documentation
dae Oct 26, 2020
362b166
add back accidentally removed deps in build script run
dae Oct 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 29 additions & 7 deletions cargo/cargo_build_script.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,12 @@ 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_locations(ctx):
"Expand $(execroot ...) references in user-provided env vars."
env = ctx.attr.build_script_env
data = getattr(ctx.attr, "data", [])
return dict([(k, ctx.expand_location(v, data)) for (k, v) in env.items()])

def _build_script_impl(ctx):
"""The implementation for the `_build_script_run` rule.

Expand Down Expand Up @@ -88,14 +94,14 @@ def _build_script_impl(ctx):
for f in ctx.attr.crate_features:
env["CARGO_FEATURE_" + f.upper().replace("-", "_")] = "1"

env.update(ctx.attr.build_script_env)
env.update(_expand_locations(ctx))

tools = depset(
direct = [
script,
ctx.executable._cargo_build_script_runner,
toolchain.rustc,
],
] + ctx.files.data,
transitive = toolchain_tools,
)

Expand Down Expand Up @@ -164,6 +170,10 @@ _build_script_run = rule(
"build_script_env": attr.string_dict(
doc = "Environment variables for build scripts.",
),
"data": attr.label_list(
doc = "Data or tools required by the build script.",
allow_files = True,
),
"_cc_toolchain": attr.label(
default = Label("@bazel_tools//tools/cpp:current_cc_toolchain"),
),
Expand All @@ -188,6 +198,7 @@ def cargo_build_script(
version = None,
deps = [],
build_script_env = {},
data = [],
**kwargs):
"""Compile and execute a rust build script to generate build attributes

Expand Down Expand Up @@ -220,10 +231,19 @@ def cargo_build_script(
cargo_build_script(
name = "build_script",
srcs = ["build.rs"],
# Data are shipped during execution.
data = ["src/lib.rs"],
# Environment variables passed during build.rs execution
build_script_env = {"CARGO_PKG_VERSION": "0.1.2"},
# Optional environment variables passed during build.rs compilation
rustc_env = {
"CARGO_PKG_VERSION": "0.1.2",
},
# Optional environment variables passed during build.rs execution.
# The path will be relative to the folder above bazel-out, so your
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this general change of allowing location expansion is a really useful one, but I don't love the interactions around path lookups.

In particular, this suggestion to traverse parent directories until you happen to find the right one. The bazel-out heuristic is an internal implementation detail of how bazel lays out files, and actually only happens to be true for generated files - if you did $(execroot //some/checked/in:file) there wouldn't be a bazel-out. $(location) and friends were designed assuming actions run in the execroot (or a runfiles tree which mirrors the execroot).

I think a reasonable approach here would be for us to change build scripts so that they executed in the execroot as their working directory (which means that these injected env vars are valid relative paths), and to recommend that crates explicitly add a env!("CARGO_MANIFEST_DIR") prefix to any paths they try to read at build time. We changed where build scripts run recently in #427 so I don't feel too bad about changing it again... This approach seems fairly principled, and gives a very clear workaround for crates which face problems with this.

Any required fix pull requests may appear as slightly odd to crate authors (the author may think "the working directory is env!("CARGO_MANIFEST_DIR") - why would I need to be explicit?"), but hopefully they'd just find it odd (and understand that alternative build tools may behave differently), rather than objectionable...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it should also work on source files, as we're only referencing "bazel-out" in the build script, which is always generated. But I take your point about this being a bit of a hack, and an approach that doesn't require hunting for execroot would be both more ergonomic and less fragile.

Perhaps one way to solve this would be to add another argument to cargo_build_script() that controls whether the working dir is execroot or the manifest dir? Crates that have bought into the Bazel ecosystem could then consume the locations directly, and third-party crates that assume cargo will set the working dir to the manifest dir for them would continue to function without patching.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That feels like a reasonable compromise to me - we could perhaps also set an additional BAZEL_EXEC_ROOT env var in the manifest dir path case, to help folks who are trying to support both well to support conditional behaviour.

One challenge I've run into is crates which use prost-build, so it may be interesting to run through this as a concrete example:

With https://github.com/danburkert/prost/pull/324 (current unreleased, sadly) and this PR, you can set build_script_env to something like {"PROTOC": "$(execroot @com_google_protobuf//:protoc)"}:

In exec-root-as-working-directory mode, things should Just Work assuming all other paths (e.g. paths to the protobufs being codegen'd) are using the $CARGO_MANIFEST_DIR prefix properly.
In cargo-manifest-dir-working-directory mode, it would require the build script to prepend ${BAZEL_EXEC_ROOT} to PROTOC, which works but is a bit annoying... Alternatively, we could extend ${pwd}-substitution to the build script runner (which seems pretty reasonable), so that one could simply use:
{"PROTOC": "${pwd}/$(execroot @com_google_protobuf//:protoc)"} (or possibly {"PROTOC": "$${pwd}/$(execroot @com_google_protobuf//:protoc)"} - I always get lost in the escaping here!)
and the build script runner would do the right thing in both modes...

Actually, the more I talk it through, the more I'm convinced we don't need both modes, we just need ${pwd}-substitution everywhere that we support $(location) substitution - how does that sound to you?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, solving the prost-build problem was one of the motivations for this PR, as I'm using it in my project too.

Substituting pwd sounds good, I'll look into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of the latest push? It adds the execroot automatically rather than relying on the user to do so - the rationale being that it's consistent with the way things like genrule() behave. But I can change it to require an explicit ${pwd} reference if you'd prefer.

And if you'd like me to squash these commits to tidy things up, please let me know.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I experimented with resolving execpath/${pwd} for rust_binary/rust_test(), but it didn't work - rustc_env is only available at compile time, and if we bake in the execroot, the execution stage is running in a different sandbox, and the path is no longer valid - so they will need to continue using rootpath instead)

# build script will need to walk up the tree first - see build.rs
# in examples/env_locations for a demo.
build_script_env = {
"SOME_TOOL_OR_FILE": "$(execroot @tool//:binary)"
}
# Optional data/tool dependencies
data = ["@tool//:binary"],
)

rust_library(
Expand All @@ -245,13 +265,15 @@ def cargo_build_script(
version (str, optional): The semantic version (semver) of the crate.
deps (list, optional): The dependencies of the crate defined by `crate_name`.
build_script_env (dict, optional): Environment variables for build scripts.
data (list, optional): Files or tools needed by the build script.
**kwargs: Forwards to the underlying `rust_binary` rule.
"""
rust_binary(
name = name + "_script_",
crate_features = crate_features,
version = version,
deps = deps,
data = data,
**kwargs
)
_build_script_run(
Expand All @@ -261,5 +283,5 @@ def cargo_build_script(
crate_features = crate_features,
version = version,
build_script_env = build_script_env,
deps = deps,
data = data,
dae marked this conversation as resolved.
Show resolved Hide resolved
)
17 changes: 12 additions & 5 deletions docs/cargo_build_script.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
## cargo_build_script

<pre>
cargo_build_script(<a href="#cargo_build_script-name">name</a>, <a href="#cargo_build_script-crate_name">crate_name</a>, <a href="#cargo_build_script-crate_features">crate_features</a>, <a href="#cargo_build_script-version">version</a>, <a href="#cargo_build_script-deps">deps</a>, <a href="#cargo_build_script-build_script_env">build_script_env</a>, <a href="#cargo_build_script-kwargs">kwargs</a>)
cargo_build_script(<a href="#cargo_build_script-name">name</a>, <a href="#cargo_build_script-crate_name">crate_name</a>, <a href="#cargo_build_script-crate_features">crate_features</a>, <a href="#cargo_build_script-version">version</a>, <a href="#cargo_build_script-deps">deps</a>, <a href="#cargo_build_script-build_script_env">build_script_env</a>, <a href="#cargo_build_script-data">data</a>, <a href="#cargo_build_script-kwargs">kwargs</a>)
</pre>

Compile and execute a rust build script to generate build attributes
Expand Down Expand Up @@ -40,10 +40,16 @@ load("@io_bazel_rules_rust//cargo:cargo_build_script.bzl", "cargo_build_script")
cargo_build_script(
name = "build_script",
srcs = ["build.rs"],
# Data are shipped during execution.
data = ["src/lib.rs"],
# Environment variables passed during build.rs execution
build_script_env = {"CARGO_PKG_VERSION": "0.1.2"},
# Optional environment variables passed during build.rs compilation
rustc_env = {
"CARGO_PKG_VERSION": "0.1.2",
},
# Optional environment variables passed during build.rs execution
build_script_env = {
"SOME_TOOL_OR_FILE": "$(execroot @tool//:binary)"
}
# Optional data/tool dependencies
data = ["@tool//:binary"],
)

rust_library(
Expand All @@ -69,6 +75,7 @@ The `hello_lib` target will be build with the flags and the environment variable
| <a id="cargo_build_script-version"></a>version | The semantic version (semver) of the crate. | <code>None</code> |
| <a id="cargo_build_script-deps"></a>deps | The dependencies of the crate defined by <code>crate_name</code>. | <code>[]</code> |
| <a id="cargo_build_script-build_script_env"></a>build_script_env | Environment variables for build scripts. | <code>{}</code> |
| <a id="cargo_build_script-data"></a>data | Files or tools needed by the build script. | <code>[]</code> |
| <a id="cargo_build_script-kwargs"></a>kwargs | Forwards to the underlying <code>rust_binary</code> rule. | none |


27 changes: 17 additions & 10 deletions docs/flatten.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ Run the benchmark test using: `bazel run //fibonacci:fibonacci_bench`.
| <a id="rust_benchmark-edition"></a>edition | The rust edition to use for this crate. Defaults to the edition specified in the rust_toolchain. | String | optional | "" |
| <a id="rust_benchmark-out_dir_tar"></a>out_dir_tar | __Deprecated__, do not use, see [#cargo_build_script] instead. | <a href="https://bazel.build/docs/build-ref.html#labels">Label</a> | optional | None |
| <a id="rust_benchmark-proc_macro_deps"></a>proc_macro_deps | List of <code>rust_library</code> targets with kind <code>proc-macro</code> used to help build this library target. | <a href="https://bazel.build/docs/build-ref.html#labels">List of labels</a> | optional | [] |
| <a id="rust_benchmark-rustc_env"></a>rustc_env | Dictionary of additional <code>"key": "value"</code> environment variables to set for rustc. | <a href="https://bazel.build/docs/skylark/lib/dict.html">Dictionary: String -> String</a> | optional | {} |
| <a id="rust_benchmark-rustc_env"></a>rustc_env | Dictionary of additional <code>"key": "value"</code> environment variables to set for rustc.<br><br>Supports $(rootpath ...) expansion, so you can use this to pass in the path to a generated file or an external tool. | <a href="https://bazel.build/docs/skylark/lib/dict.html">Dictionary: String -> String</a> | optional | {} |
| <a id="rust_benchmark-rustc_flags"></a>rustc_flags | List of compiler flags passed to <code>rustc</code>. | List of strings | optional | [] |
| <a id="rust_benchmark-srcs"></a>srcs | List of Rust <code>.rs</code> source files used to build the library.<br><br>If <code>srcs</code> contains more than one file, then there must be a file either named <code>lib.rs</code>. Otherwise, <code>crate_root</code> must be set to the source file that is the root of the crate to be passed to rustc to build this crate. | <a href="https://bazel.build/docs/build-ref.html#labels">List of labels</a> | optional | [] |
| <a id="rust_benchmark-version"></a>version | A version to inject in the cargo environment variable. | String | optional | "0.0.0" |
Expand Down Expand Up @@ -239,7 +239,7 @@ Hello world
| <a id="rust_binary-out_binary"></a>out_binary | - | Boolean | optional | False |
| <a id="rust_binary-out_dir_tar"></a>out_dir_tar | __Deprecated__, do not use, see [#cargo_build_script] instead. | <a href="https://bazel.build/docs/build-ref.html#labels">Label</a> | optional | None |
| <a id="rust_binary-proc_macro_deps"></a>proc_macro_deps | List of <code>rust_library</code> targets with kind <code>proc-macro</code> used to help build this library target. | <a href="https://bazel.build/docs/build-ref.html#labels">List of labels</a> | optional | [] |
| <a id="rust_binary-rustc_env"></a>rustc_env | Dictionary of additional <code>"key": "value"</code> environment variables to set for rustc. | <a href="https://bazel.build/docs/skylark/lib/dict.html">Dictionary: String -> String</a> | optional | {} |
| <a id="rust_binary-rustc_env"></a>rustc_env | Dictionary of additional <code>"key": "value"</code> environment variables to set for rustc.<br><br>Supports $(rootpath ...) expansion, so you can use this to pass in the path to a generated file or an external tool. | <a href="https://bazel.build/docs/skylark/lib/dict.html">Dictionary: String -> String</a> | optional | {} |
| <a id="rust_binary-rustc_flags"></a>rustc_flags | List of compiler flags passed to <code>rustc</code>. | List of strings | optional | [] |
| <a id="rust_binary-srcs"></a>srcs | List of Rust <code>.rs</code> source files used to build the library.<br><br>If <code>srcs</code> contains more than one file, then there must be a file either named <code>lib.rs</code>. Otherwise, <code>crate_root</code> must be set to the source file that is the root of the crate to be passed to rustc to build this crate. | <a href="https://bazel.build/docs/build-ref.html#labels">List of labels</a> | optional | [] |
| <a id="rust_binary-version"></a>version | A version to inject in the cargo environment variable. | String | optional | "0.0.0" |
Expand Down Expand Up @@ -535,7 +535,7 @@ INFO: Elapsed time: 1.245s, Critical Path: 1.01s
| <a id="rust_library-edition"></a>edition | The rust edition to use for this crate. Defaults to the edition specified in the rust_toolchain. | String | optional | "" |
| <a id="rust_library-out_dir_tar"></a>out_dir_tar | __Deprecated__, do not use, see [#cargo_build_script] instead. | <a href="https://bazel.build/docs/build-ref.html#labels">Label</a> | optional | None |
| <a id="rust_library-proc_macro_deps"></a>proc_macro_deps | List of <code>rust_library</code> targets with kind <code>proc-macro</code> used to help build this library target. | <a href="https://bazel.build/docs/build-ref.html#labels">List of labels</a> | optional | [] |
| <a id="rust_library-rustc_env"></a>rustc_env | Dictionary of additional <code>"key": "value"</code> environment variables to set for rustc. | <a href="https://bazel.build/docs/skylark/lib/dict.html">Dictionary: String -> String</a> | optional | {} |
| <a id="rust_library-rustc_env"></a>rustc_env | Dictionary of additional <code>"key": "value"</code> environment variables to set for rustc.<br><br>Supports $(rootpath ...) expansion, so you can use this to pass in the path to a generated file or an external tool. | <a href="https://bazel.build/docs/skylark/lib/dict.html">Dictionary: String -> String</a> | optional | {} |
| <a id="rust_library-rustc_flags"></a>rustc_flags | List of compiler flags passed to <code>rustc</code>. | List of strings | optional | [] |
| <a id="rust_library-srcs"></a>srcs | List of Rust <code>.rs</code> source files used to build the library.<br><br>If <code>srcs</code> contains more than one file, then there must be a file either named <code>lib.rs</code>. Otherwise, <code>crate_root</code> must be set to the source file that is the root of the crate to be passed to rustc to build this crate. | <a href="https://bazel.build/docs/build-ref.html#labels">List of labels</a> | optional | [] |
| <a id="rust_library-version"></a>version | A version to inject in the cargo environment variable. | String | optional | "0.0.0" |
Expand Down Expand Up @@ -792,7 +792,7 @@ Run the test with `bazel build //hello_lib:hello_lib_test`.
| <a id="rust_test-edition"></a>edition | The rust edition to use for this crate. Defaults to the edition specified in the rust_toolchain. | String | optional | "" |
| <a id="rust_test-out_dir_tar"></a>out_dir_tar | __Deprecated__, do not use, see [#cargo_build_script] instead. | <a href="https://bazel.build/docs/build-ref.html#labels">Label</a> | optional | None |
| <a id="rust_test-proc_macro_deps"></a>proc_macro_deps | List of <code>rust_library</code> targets with kind <code>proc-macro</code> used to help build this library target. | <a href="https://bazel.build/docs/build-ref.html#labels">List of labels</a> | optional | [] |
| <a id="rust_test-rustc_env"></a>rustc_env | Dictionary of additional <code>"key": "value"</code> environment variables to set for rustc. | <a href="https://bazel.build/docs/skylark/lib/dict.html">Dictionary: String -> String</a> | optional | {} |
| <a id="rust_test-rustc_env"></a>rustc_env | Dictionary of additional <code>"key": "value"</code> environment variables to set for rustc.<br><br>Supports $(rootpath ...) expansion, so you can use this to pass in the path to a generated file or an external tool. | <a href="https://bazel.build/docs/skylark/lib/dict.html">Dictionary: String -> String</a> | optional | {} |
| <a id="rust_test-rustc_flags"></a>rustc_flags | List of compiler flags passed to <code>rustc</code>. | List of strings | optional | [] |
| <a id="rust_test-srcs"></a>srcs | List of Rust <code>.rs</code> source files used to build the library.<br><br>If <code>srcs</code> contains more than one file, then there must be a file either named <code>lib.rs</code>. Otherwise, <code>crate_root</code> must be set to the source file that is the root of the crate to be passed to rustc to build this crate. | <a href="https://bazel.build/docs/build-ref.html#labels">List of labels</a> | optional | [] |
| <a id="rust_test-version"></a>version | A version to inject in the cargo environment variable. | String | optional | "0.0.0" |
Expand Down Expand Up @@ -963,15 +963,15 @@ The tools required for the `rust_wasm_bindgen` rule.
| Name | Description | Type | Mandatory | Default |
| :------------- | :------------- | :------------- | :------------- | :------------- |
| <a id="rust_wasm_bindgen_toolchain-name"></a>name | A unique name for this target. | <a href="https://bazel.build/docs/build-ref.html#name">Name</a> | required | |
| <a id="rust_wasm_bindgen_toolchain-bindgen"></a>bindgen | The label of a <code>bindgen</code> executable. | <a href="https://bazel.build/docs/build-ref.html#labels">Label</a> | optional | None |
| <a id="rust_wasm_bindgen_toolchain-bindgen"></a>bindgen | The label of a <code>wasm-bindgen</code> executable. | <a href="https://bazel.build/docs/build-ref.html#labels">Label</a> | optional | None |


<a id="#cargo_build_script"></a>

## cargo_build_script

<pre>
cargo_build_script(<a href="#cargo_build_script-name">name</a>, <a href="#cargo_build_script-crate_name">crate_name</a>, <a href="#cargo_build_script-crate_features">crate_features</a>, <a href="#cargo_build_script-version">version</a>, <a href="#cargo_build_script-deps">deps</a>, <a href="#cargo_build_script-build_script_env">build_script_env</a>, <a href="#cargo_build_script-kwargs">kwargs</a>)
cargo_build_script(<a href="#cargo_build_script-name">name</a>, <a href="#cargo_build_script-crate_name">crate_name</a>, <a href="#cargo_build_script-crate_features">crate_features</a>, <a href="#cargo_build_script-version">version</a>, <a href="#cargo_build_script-deps">deps</a>, <a href="#cargo_build_script-build_script_env">build_script_env</a>, <a href="#cargo_build_script-data">data</a>, <a href="#cargo_build_script-kwargs">kwargs</a>)
</pre>

Compile and execute a rust build script to generate build attributes
Expand Down Expand Up @@ -1005,10 +1005,16 @@ load("@io_bazel_rules_rust//cargo:cargo_build_script.bzl", "cargo_build_script")
cargo_build_script(
name = "build_script",
srcs = ["build.rs"],
# Data are shipped during execution.
data = ["src/lib.rs"],
# Environment variables passed during build.rs execution
build_script_env = {"CARGO_PKG_VERSION": "0.1.2"},
# Optional environment variables passed during build.rs compilation
rustc_env = {
"CARGO_PKG_VERSION": "0.1.2",
},
# Optional environment variables passed during build.rs execution
build_script_env = {
"SOME_TOOL_OR_FILE": "$(execroot @tool//:binary)"
}
# Optional data/tool dependencies
data = ["@tool//:binary"],
)

rust_library(
Expand All @@ -1034,6 +1040,7 @@ The `hello_lib` target will be build with the flags and the environment variable
| <a id="cargo_build_script-version"></a>version | The semantic version (semver) of the crate. | <code>None</code> |
| <a id="cargo_build_script-deps"></a>deps | The dependencies of the crate defined by <code>crate_name</code>. | <code>[]</code> |
| <a id="cargo_build_script-build_script_env"></a>build_script_env | Environment variables for build scripts. | <code>{}</code> |
| <a id="cargo_build_script-data"></a>data | Files or tools needed by the build script. | <code>[]</code> |
| <a id="cargo_build_script-kwargs"></a>kwargs | Forwards to the underlying <code>rust_binary</code> rule. | none |


Expand Down
Loading