Skip to content

Add support for cargo:rustc-env outputs from build scripts (draft) #919

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

Closed

Conversation

yxdai-nju
Copy link
Contributor

@yxdai-nju yxdai-nju commented Apr 24, 2025

Summary

This PR adds support for collecting cargo:rustc-env outputs from build scripts.

Relates to facebookincubator/reindeer#67.

Changes

  • Modified the build script runner to collect cargo:rustc-env outputs, in the format of "--env=NAME=val" flags
  • Added a new sub-target [env_flags] on buildscript_run, for getting the "--env" flags output
  • Added a new attribute env_flags to rust_library and rust_binary rule, accepting "--env" flags

Example usage:

cargo.rust_library(
    name = "mime_guess-2.0.5",
    env_flags = ["@$(location :mime_guess-2.0.5-build-script-run[env_flags])"],
    # ...
)

After facebookincubator/reindeer#67, the env_flags = ["@$(location ...)"], setting will be automatically added, when any buildscript fixup of rustc_flags or gen_srcs type exists.

Closes #918.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 24, 2025
@facebook-github-bot
Copy link
Contributor

@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.)

@cormacrelf
Copy link
Contributor

All of this would be dramatically simpler, I'm talking +3 lines of code instead of +300, with the unstable --env-set flag (rust-lang/rust#118372).

Could this PR encode the flags the same way (as --env-set=KEY=VALUE once per line) so that when the flag is stabilised and rustc can do this natively, we simply remove special handling of the environment argfile and pass it directly to rustc?

@cormacrelf
Copy link
Contributor

This is the +3 for posterity

facebookincubator/reindeer#48 (comment)

@yxdai-nju
Copy link
Contributor Author

yxdai-nju commented Apr 25, 2025

Could this PR encode the flags the same way (as --env-set=KEY=VALUE once per line) so that when the flag is stabilised and rustc can do this natively, we simply remove special handling of the environment argfile and pass it directly to rustc?

Thank you for your suggestions, this would be a really helpful feature!

However, things are a bit more complex with remote execution. If an environment variable contains a path, it should get converted to an absolute path before passed to rustc, to ensure correct behavior.

Currently, any environment variable values you set in the env attribute will automatically undergo this conversion.

The workflow of my solution is, for example:

1. Collect build script outputs:

# filtered build script outputs
cargo:rustc-env=NAME=val
cargo:rustc-env=MYFILE=path/to/myfile

--->

# flags for internal tool
--env=NAME=val
--env=MYFILE=path/to/myfile

This produces "--env" flags to be passed to write_env_lines_action internal tool.

2. Determine whether the values contain paths:
A critical step in prelude/rust/build.bzl in the PR is to determine whether each environment variable value is a path:

# Step 3: Utilize 'process_env' to determine whether each environment variable
#         is "plain" or "with path", then produce an *-env-flags.txt file
#         containing environment variable flags suitable to pass to the
#         `_long_command` macro. For instance
#         ```
#         --env=USER=john
#         --path-env=HOME=/home/john
#         ```
plain_env, path_env = process_env(compile_ctx, env_map, exec_is_windows, escape_for_rustc_action)

So the conversion done here is:

# flags for internal tool
--env=NAME=val
--env=MYFILE=path/to/myfile

--->

# flags for `_long_command`
--env=NAME=val
--path-env=MYFILE=path/to/myfile

This produces "--env" and "--path-env" flags to be passed to the _long_command macro, which is responsible for preparing the environment variables for rustc invocations; it converts a path to an absolute path, if an environment variable is passed to it with "--path-env" flag.

@yxdai-nju yxdai-nju force-pushed the buildscript-cargo-env-enhancement branch from 329d071 to 4f96e1a Compare April 25, 2025 05:17
@yxdai-nju yxdai-nju changed the title Add support for cargo:rustc-env and improve Cargo environment variable handling Add support for cargo:rustc-env outputs from build scripts Apr 25, 2025
@yxdai-nju yxdai-nju force-pushed the buildscript-cargo-env-enhancement branch from 39f6a2e to a77a2b6 Compare April 27, 2025 09:21
Signed-off-by: Yuxuan Dai <[email protected]>
* 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=(.+?)=(.*)` output into `--env=$1=$2` flags
* Export flags as an Artifact, which can be passed to cargo.rust_library/binary with
  `env_flags = ["@$(location :<CARGO_PKG_NAME>-<CARGO_PKG_VERSION>-build-script-run[env_flags])"],`

Signed-off-by: Yuxuan Dai <[email protected]>
@yxdai-nju yxdai-nju force-pushed the buildscript-cargo-env-enhancement branch from a77a2b6 to e575b5b Compare April 28, 2025 08:29
@yxdai-nju
Copy link
Contributor Author

I misunderstood the purpose of process_env function and did lots of unnecessary conversions in the code. I'm closing this PR and filing a new one.

the process_env function is for distinguishing normal values and path values which are expanded from macros. For example:

env = {
    "NAME": "john",
    "RESULT_PATH": "$(location :my_crate-1.0.0-build-script-run[out_dir])/result.txt",
}

The mapping goes through the process_env function, and will be turned into:

--env=NAME=john
--path-env=buck-out/v2/gen/.../__my_crate-1.0.0-build-script-run__/OUT_DIR/result.txt

And with --path-env instead of --env, the _long_command macro can turn the buck-out/... path into absolute path before passing it to rustc.

Environment variable values emitted by build scripts don't contain macros and don't need this conversion. Using just --env=NAME=val flags would suffice.

@yxdai-nju yxdai-nju closed this Apr 30, 2025
@yxdai-nju yxdai-nju changed the title Add support for cargo:rustc-env outputs from build scripts Add support for cargo:rustc-env outputs from build scripts (draft) Apr 30, 2025
@yxdai-nju
Copy link
Contributor Author

Superseded by #929.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

buildscript_run does not collect environment variables emitted with "cargo:rustc-env=xxx"
3 participants