rustc: add arbitrary environment variables

[tommilligan]

Rules that call rustc setup environment variables in line with cargo's behaviour (see the documentation here). However they don't allow passing in additional environment variables that may be required at compile time, such as the PROTOC variable in prost-build.

This PR adds the rustc_env attribute to the common attribute set, allowing it to be set in the following rules:

• rust_benchmark
• rust_binary
• rust_library
• rust_test

Variables are provided in a string dictionary, and will be merged into the generated environment overriding existing values.

[damienmg]

Hi @tommilligan,

Thanks for your contribution!

Code-wise, this PR looks ok. It is missing the corresponding entry in the doc, but I think it needs to be left away if we move forward with the doc (keeping the thing as undocumented for now).

It seems like what you are trying to achieve can be done with --action_env command line flag of Bazel. If you already knew about --action_env, could you explain the rationale for adding it vs using --action_env?

[tommilligan]

Sure - for a concrete example, I've removed the rustc_env setting from my test case:

diff --git a/test/build_env/BUILD b/test/build_env/BUILD
index 55496c9..5d14626 100644
--- a/test/build_env/BUILD
+++ b/test/build_env/BUILD
@@ -14,7 +14,4 @@ rust_test(
 rust_test(
     name = "arbitrary_env_test",
     srcs = ["tests/arbitrary_env.rs"],
-    rustc_env = {
-        "USER_DEFINED_KEY": "USER_DEFINED_VALUE",
-    },
 )

I then ran the same test using --action_env as you suggested. The compiler cannot access the environment variable:

$ bazel test --action_env=USER_DEFINED_KEY=USER_DEFINED_VALUE //test/build_env:arbitrary_env_test
DEBUG: <snip>
INFO: Build option --action_env has changed, discarding analysis cache.
INFO: Analyzed target //test/build_env:arbitrary_env_test (3 packages loaded, 439 targets configured).
INFO: Found 1 test target...
ERROR: /home/tom/Documents/personal/rules_rust/test/build_env/BUILD:14:1: error executing shell command: '/bin/bash -c CARGO_MANIFEST_DIR=$(pwd)/test/build_env external/rust_linux_x86_64/bin/rustc "$@" --remap-path-prefix="$(pwd)"=__bazel_redacted_pwd  test/build_env/tests/arbitrary_env.rs --crate-name...' failed (Exit 1) bash failed: error executing command /bin/bash -c 'CARGO_MANIFEST_DIR=$(pwd)/test/build_env external/rust_linux_x86_64/bin/rustc "$@" --remap-path-prefix="$(pwd)"=__bazel_redacted_pwd' '' test/build_env/tests/arbitrary_env.rs ... (remaining 16 argument(s) skipped)

Use --sandbox_debug to see verbose messages from the sandbox
error: environment variable `USER_DEFINED_KEY` not defined
 --> test/build_env/tests/arbitrary_env.rs:3:18
  |
3 |     let actual = env!("USER_DEFINED_KEY");
  |                  ^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

Target //test/build_env:arbitrary_env_test failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 0.615s, Critical Path: 0.15s
INFO: 0 processes.
FAILED: Build did NOT complete successfully
//test/build_env:arbitrary_env_test                             FAILED TO BUILD

FAILED: Build did NOT complete successfully

This is due to the fact that in rustc.bzl we explicitly set the env attribute. As documented here, env sets the environment to exactly the values specified (it doesn't update the default shell environment).

This has been discussed in a couple of places:

• ctx.action.run_shell: use_default_shell_env=True overwrites env={...}  bazel#5980 (comment)
• Add common env attribute for *_binary and *_test bazel#7364

A different way to workaround this constraint would be to get the default shell environment (not sure if this is possible), update it with the cargo specific environment variables, then set this as the shell environment. This would mimic the behaviour of cargo, which also passes through variables from the outer environment.

If this is possible I'd actually prefer it, but it feels like it could be a breaking change, whereas a rustc_env attribute would be completely opt-in.

[damienmg]

I prefer this over the alternatives. I let some time for Marco or Alex to chime in.

[dfreese]

Taking a page from local_defines in cc_library, perhaps it would be worth indicating if the listed env vars propagate to a dependent rust_library, and if make variable expansion applies or not.

A way to do that could be naming it local_env or similar.

https://docs.bazel.build/versions/master/be/c-cpp.html#cc_library_args

Had been dealing with prost-build myself, so this is cool to see!

[tommilligan]

@dfreese thanks for the suggestion, if others agree local_env is a more appropriate name I'm happy to amend it.

Currently this implementation:

• doesn't propagate variables to dependencies
• doesn't support Make variable expansion

which I'm happy to add to the docstring.

[dfreese]

@tommilligan sounds good. Thank for working on this!

@smklein wanted to point this out since you had been looking at cargo raze's handling of build.rs files.

[kornholi]

I've often wanted this myself but usually find better solutions in the end. With prost-build, we don't actually want it to call protoc, see my comment here: https://github.com/danburkert/prost/pull/309#issuecomment-624743491

I think rustc_env should be added to CrateInfo. It's missing a few other attributes as is (e.g. crate_features), but this leads to issues similar to #266.

[tommilligan]

@kornholi could you elaborate on why this should be added to CrateInfo? I'm reasonably new to the bazel ecosystem in general, but it looks like CrateInfo serves two purposes:

1. to aggregate information about a crate and it's dependencies
2. to hold information that is required after compilation, when a consumer uses the compiled artefact

I don't think 1. is relevant, as the environment is only relevant to a specific crate's compilation, and does not propagate up or down the build tree.

It's possible that 2. would be useful to inspect the environment when a crate was compiled, but I think in this case the whole of the generated env should be stored on CrateInfo, not rustc_env specifically. This feels like a different issue/PR concern to me.

[kornholi]

You have the right idea, but there's already some code that (incorrectly) assumes CrateInfo captures everything, e.g. https://github.com/bazelbuild/rules_rust/blob/master/rust/private/rust.bzl#L176-L188. Since it's been broken before your changes, let's not worry about it for now.

[dfreese]

Looking at it again, the name and behavior of rustc_env is consistent with the rust_flags, so this is good with me.

[damienmg]

I think @kornholi is correct, since we might use the CrateInfo for more than just artifact analysis (e.g. for building compilation database). I would prefer you move it there before we merge or we might forget about it and get bugged by it in the future.

[tommilligan]

@damienmg updated. rustc.bzl now pulls the values for rustc_env from the CrateInfo object instead of ctx.attr.

As with aliases, this field is always set to the empty dictionary in proto.bzl, although there is no reason why it couldn't also be exposed there.

[damienmg]

I think all concerns have been resolved for this PR, so I will go ahead and merge it.

Thanks a lot @tommilligan for this!