-
Notifications
You must be signed in to change notification settings - Fork 261
Add support for cargo:rustc-env
outputs from build scripts
#929
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.
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
base: main
Are you sure you want to change the base?
Conversation
@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. (Because this pull request was imported automatically, there will not be any future comments.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but can you confirm this works for buckifying the mime-guess crate via reindeer? IIRC it did include a path in its generated environment variable and is likely to surface any issues with that.
prelude/rust/build.bzl
Outdated
for flag in ctx.attrs.env_flags: | ||
compile_cmd.add(cmd_args(flag)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for flag in ctx.attrs.env_flags: | |
compile_cmd.add(cmd_args(flag)) | |
compile_cmd.add(ctx.attrs.env_flags) |
Same effect I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I applied it in this commit.
30bef02
to
7cda182
Compare
Sure! I just created a demonstrating repo: https://github.com/yxdai-nju/buck2-buildscript-envflags-demo. |
Doesn't look like it will work in Remote Execution. That's an absolute path (
It might be necessary to special-case any paths that are prefixed with $OUT_DIR. I think it would be sufficient to de-absolute-ify the paths, and transform
which will be guaranteed to exist at the time we execute the compile step, at least as long as OUT_DIR is passed in as environment, which it is, if you use [buildscript.gen_srcs]. The de-absolute path transform should be done immediately as you read the |
Also thanks for making that demo repo. I was going to just take your word for it, but this has been very helpful. |
* Accept passing environment variables through "--env=NAME=VAL" flags * Add two rust internal tools to assist environment variable flags processing Signed-off-by: Yuxuan Dai <[email protected]>
* Collect build script `cargo:rustc-env=NAME=val` output as `--env=NAME=val` flags * Export flags as an Artifact, which can be passed to cargo.rust_library/binary with `env_flags = ["@$(location :my_crate-0.0.0-build-script-run[env_flags])"],` Signed-off-by: Yuxuan Dai <[email protected]>
* Trim prefix of absolute paths, and pass them with "--path-env" instead of with "--env" Signed-off-by: Yuxuan Dai <[email protected]>
7cda182
to
86dc91e
Compare
After you pointed this out, I tried mime_guess with remote execution, and that's exactly what happened 😅
Your suggestion is accurate! I added this transformation to this PR, may you have a look? @cormacrelf It relies on the fact that the build script runner ( I have simultaneously updated the demonstration repo, adding NativeLink remote execution. You can use it directly with VS Code Dev Container. |
Lgtm. This should cover 99% of build scripts using env vars. Someone from meta should have a look. Buck will always execute build actions from the root, containing buck-out. The only thing that can go wrong here is windows paths having more than one valid prefix, and the windows people can deal with that if it ever arises, which may never happen as long as we live. |
Summary
This PR adds support for collecting
cargo:rustc-env
outputs from build scripts.Relates to facebookincubator/reindeer#74.
Changes
cargo:rustc-env
outputs as a sequence of--env
/--path-env
flags[env_flags]
sub-target for retrieving the flags output from build-script-runenv_flags
attribute torust_library
/rust_binary
rules that accepts such flagsExample with
env_flags
attribute and[env_flags]
sub-target:After the PR to Reindeer, the
env_flags = ["@$(... [env_flags])"],
line will be automatically added, whenbuildscript.run = true
.Details
For each
cargo:rustc-env={key}={value}
output from a build script:If
value
is an absolute path to an artifact, e.g.,/nativelink_work_dir/[hash]/work/buck-out/.../outfile
in remote execution,/buck_root_dir/buck-out/.../outfile
in local execution,generate a
--path-env={key}={relative_path}
flag whererelative_path
is the path with "workdir-prefix" trimmed, e.g.:Otherwise,
generate a standard
--env={key}={value}
flag, e.g.:This aligns with the behavior of
process_env
inprelude/rust/build.bzl
when dealing with paths in environment settings.Testing
A demonstration repository verifies this works for
mime_guess
andring
crates, both havecargo:rustc-env
outputs from build scripts.Closes #918.