Support expanding locations in rustc_env and build_script_env #461
Conversation
dae
force-pushed
the
location-expansion
branch
from
05a67f6
to
b1ed4f7
Compare
|
I've pushed a fix for the failing clippy run; if there was a better way to address it please let me know. |
dae
force-pushed
the
location-expansion
branch
from
aad4084
to
c5ee4d2
Compare
dae
force-pushed
the
location-expansion
branch
from
3e021fd
to
b917295
Compare
|
Ok, this seems to be related to external repos. Changing to the examples folder and running 'bazel test env_locations/...' works, but it fails when running 'bazel test @examples//env_locations/...' from the top level folder. The issue seems to be limited to build scripts - it works correctly in both cases for normal rust_binaries. I'm not yet familiar enough with Bazel to know how to solve this properly - any suggestions would be appreciated. |
- rustc_env now expands $(rootpath //package:target), for passing
the location of files or tools 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
Ideally both build and run would work with execpath/location, but
the approach of walking back to the root folder that cargo_build_script()
is using doesn't seem to be possible at compile time, as ${pwd} is being
substituted after the starlark code is executed. So at compile time,
we use the relative rootpath instead.
dae
force-pushed
the
location-expansion
branch
from
b917295
to
f3d1552
Compare
Rust on Windows seems to struggle with relative pathnames in this case
- I suspect due to the fact that symlinks are being used.
fs::read_to_string("../../../path/to/somewhere")
gives an error
as does calling .canonicalize() on the above. Switching to \ as a
path separator does not seem to help.
If the parent directory references are handled separately, eg
Path::new("../../..").canonicalize(), then the path/to/somewhere
can be joined onto the folder and the file can be read. So instead
of automatically adjusting the path, we just return execroot verbatim,
and rely on the build script to do their own path manipulation.
|
Ok, I've got something that is passing CI. Copying in the updated commit notes:
Ideally both build and run would work with execpath/location, but And from the fix: Rust on Windows seems to struggle with relative pathnames in this case
fs::read_to_string("../../../path/to/somewhere") gives an error as does calling .canonicalize() on the above. Switching to \ as a If the parent directory references are handled separately, eg Path::new("../../..").canonicalize(), then the path/to/somewhere |
dae
changed the title
cargo/cargo_build_script.bzl
Outdated
| "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 |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
(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)
There was a problem hiding this comment.
LGTM - I just tested this out and it worked beautifully.
@dfreese @mfarrugi How do y'all feel about implicitly making $(execpath) into an absolute path? On the one hand, it's surprising and un-bazel-y, on the other hand, the fact that we're running actions not in the execpath is also surprising and un-bazel-y, and this feels like a reasonable way to compensate for that weirdness...
I'm not thrilled by it, particularly since I consider prost-build to be a bad actor by requiring an executable path to be compiled in. (I ended up patching the env! macros out of the build script), but I don't know if I see a better way around it. |
As of https://github.com/danburkert/prost/pull/324 it's at least a runtime rather than compile-time env var :) |
|
A tidied up version of this is in #468 - I'll close this one in favour of the new one. |
This makes it possible to pass in the path to generated files and
external tools.
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