Propagate rustc_env{,_files} from rust_test.crate

[bsilver8192]

rust_test.crate builds the same file as the rust_library it's pointing
to, which will almost certainly depend on the same environment variables
being set.

[UebelAndre]

I think this seems good. Wondering if @scentini could provide a second option 😄

[scentini]

I think now we only need to look into the rustc_env_files attribute for backwards compatibility reasons - custom rules may create crate_infos that currently do not set crate_info.rustc_env_files.
Could we add a condition to only inspect the attribute when crate_info.rustc_env_files is empty? This way we won't be passing each rustc_env_file twice.

Ideally we would also add a incompatible_get_rustc_env_files_from_crate_info flag that we could flip in the future, but I think it's fine for that to be a separate PR with (potentially) a different author.

[bsilver8192]

Isn't this the only place that reads the rustc_env_files from all the rules? And I think having them both is useful if a rust_test wants to add a file in addition to the ones inherited from its crate.

For backwards compatibility, I updated create_crate_info. Do you think that's sufficient, or should I change this to getattr(crate_info, "rustc_env_files", []) to handle custom rules which don't use create_crate_info?

[scentini]

If I read the code correctly, rust_test would have put its file into its crate_info so we'd get the same file twice, once from the getattr(files, "rustc_env_files") and once from the crate_info. That's why I suggest that we only inspect rustc_env_files when crate_info.rustc_env_files is empty.

Ideally we would remove the getattr here altogether, but that is a backwards incompatible change as a custom non-OSS rule can currently rely on collect_inputs to access the attribute. In the future we can introduce an incompatible flag and remove it completely after we've given the users some time to adapt their code.

For backwards compatibility, I updated create_crate_info. Do you think that's sufficient, or should I change this to getattr(crate_info, "rustc_env_files", []) to handle custom rules which don't use create_crate_info?

It is sufficient. create_crate_info exists for the exact purpose of enabling us to do changes like this one in a backwards compatible way. I consider direct calls to CrateInfo() unsupported. We should also have create_*_info for the rest of our providers, but that's orthogonal to this PR.

[bsilver8192]

If I read the code correctly, rust_test would have put its file into its crate_info so we'd get the same file twice, once from the getattr(files, "rustc_env_files") and once from the crate_info. That's why I suggest that we only inspect rustc_env_files when crate_info.rustc_env_files is empty.

I don't think that's how it works. With this:

rust_library(
    name = "lib",
    rustc_env_files = ["a"],
    ...
)

rust_test(
    name = "lib_test",
    crate = "lib",
    ...
)

Then getattr(files, "rustc_env_files") is [File("a")] for lib and [] for lib_test. There's no duplicate file.

With this:

rust_library(
    name = "lib",
    rustc_env_files = ["a"],
    ...
)

rust_test(
    name = "lib_test",
    crate = "lib",
    rustc_env_files = ["b"],
    ...
)

Then getattr(files, "rustc_env_files") is [File("a")] for lib and [File("b")] for lib_test. I don't see why it makes sense to ignore b in this case.

The only way I see to get duplicates is with rustc_env_files on separate rules that include the same file(s), which seems like a silly thing to do in the first place.

[bsilver8192]

Any more thoughts @scentini ?

[scentini]

Sorry for the delayed response. I think you'll understand the point I'm trying to make the easiest if you print(build_env_files) below this line. Running bazel test test/build_env:arbitrary_env_lib_test prints the following for me: [<generated file test/rustc_env_files/rustc_env_file>, <generated file test/rustc_env_files/rustc_env_file>]

[bsilver8192]

You're right, I was confusing this code with the line in _rust_test_impl that adds them. Done

[scentini]

Thanks! I left a suggestion to still consider the rustc_env_files attribute in the case where crate_info.rustc_env_files is not populated. This is so that we keep backwards compatibility. While you have thoroughly populated crate_info.rustc_env_files in the rules_rust area, there can be Starlark rule writers out there that invoke our rustc_compile_action() and that rely on the rustc_env_files attribute being inspected.

[scentini]

Thanks! I left a suggestion to still consider the rustc_env_files attribute in the case where crate_info.rustc_env_files is not populated. This is so that we keep backwards compatibility. While you have thoroughly populated crate_info.rustc_env_files in the rules_rust area, there can be Starlark rule writers out there that invoke our rustc_compile_action() and that rely on the rustc_env_files attribute being inspected.

[scentini]

Thanks!