Rust test targets now create a test launcher allow for setting env vars

[UebelAndre]

I was mistaken when I originally implemented #577 because I didn't realize that std::env loaded environment variables at compile time. The goal of this PR was to define environment variables at runtime. This appeared to be more complicated than I had thought. It seems there needs to be a launcher/runner that is produced by your rule and invokes your test executable to be able to guarantee the environment is configured for the test.

This PR creates a small shell script that wraps rust_test targets to enable the use of the env attribute at runtime.

[illicitonion]

This looks useful, and I'm surprisingly OK with it given it's generating compilable rust code in starlark :D I'm a little worried about not escaping quotes in values, so I'd lean towards using something like:

"r#####\"{}\"#####"

in the templates rather than

"\"{}\""

I'd definitely like others' opinions before merging, though, as this is a significant change, and there are potentially a lot of edge cases :)

[illicitonion]

Works for me, but again, would want to hear from others before merging :)

[hlopko]

I kept thinking and chatting with people about this yesterday. The Bazel team said they would accept a contribution to allow specifying env for test runs the same way they support for native rules. I don't expect that to be trivial, but doable. So that's one avenue.

Second avenue is to not use a rules_rust provided testrunner, but to bake custom one only for the tests that need to have env vars set, only in the project that has the affected tests. It doesn't even have to be a template, the test runner can assume a data file at some well specified location.

Third is the approach of this PR, to have a general Rust test runner that is used for all Rust tests built with rules_rust. As with the second point it doesn't necessarily have to be a template, we can use data files instead. This approach has a potential/handwave-handwave advantage that once we want to produce junit xml test outputs which Bazel can parse and report nicer test results, we will have to have the test runner anyway. The downside is a bit of complexity and my paranoid fear of misusing the test runner for other stuff.

Andre, Daniel, what do you think about this?

[UebelAndre]

I don't think users should have to bake environment variables into their tests and it appears that most user's expect to be able to do something like in this PR (as per a recent slack thread). I think for the first and third options, they're more or less invisible to the user. They'll want to set an env attribute and will be able to with both approaches. I feel this PR is a good enough start and we can refactor it into something that is more in alignment with upstream Bazel changes or replace it for something entirely.

[illicitonion]

The Bazel team said they would accept a contribution to allow specifying env for test runs the same way they support for native rules. I don't expect that to be trivial, but doable. So that's one avenue.

This sounds long-term ideal.

Second avenue is to not use a rules_rust provided testrunner, but to bake custom one only for the tests that need to have env vars set, only in the project that has the affected tests. It doesn't even have to be a template, the test runner can assume a data file at some well specified location.

This sounds not particularly worth the complexity, particularly if in the long term we're either going to delete the test runner, or run everything under it anyway (for e.g. junit XML).

Third is the approach of this PR, to have a general Rust test runner that is used for all Rust tests built with rules_rust. As with the second point it doesn't necessarily have to be a template, we can use data files instead. This approach has a potential/handwave-handwave advantage that once we want to produce junit xml test outputs which Bazel can parse and report nicer test results, we will have to have the test runner anyway. The downside is a bit of complexity and my paranoid fear of misusing the test runner for other stuff.

Seems reasonable in the short term, and long-term we can either delete it, or build it up more.

[hlopko]

After looking into ways how to make debugging work I yield :) Let's do this. I propose a different approach that I think will result in simpler code:

• I'd make the rust test runner a rust_binary, and make it an implicit attribute of rust_test rule
• rust test runner will look for a file with environment variables and for a test binary in its runfiles (well known locations and symlinks work well enough imho)

I think it's worth doing this because:

• this way we don't have to recompile the runner for every test
• we can make the runner a regular rust_binary target, we don't have to duplicate the building logic/provider merging and so on
• if the future will ask for modifications, we can add dependencies or more sources to the test runner cleanly

What do you think?

[illicitonion]

Sounds good!

[UebelAndre]

@hlopko @illicitonion How's something like this look?

[hlopko]

Do we need to do all of this? What I had in mind was to add this into rust_test rule declaration:

"_process_wrapper": attr.label(
        default = Label("//util/test_runner"),
        executable = True,
        allow_single_file = True,
        cfg = "exec",
    ),

And then:
0) add a rust_binary at //util/test_runner:test_runner

1. in rust_test access the runner using ctx.executable._test_runner
2. put that in the DefaultInfo that rust_test provides
3. register action that writes list of env variables into a file
4. put that file into DefaultInfo runfiles
5. register action that symlinks the actual test binary into some path
6. put both the test and the symlink into runfiles
7. modify test_runner to look into runfiles for env-vars file and for the symlink

Does it make sense?

[UebelAndre]

This is different from the process wrapper because the process wrapper is used as an executable for an action. However, the launcher/test_runner needs to be the executable returned by the rule so it gets executed when you bazel test //:my_target. Bazel will otherwise throw an error if you return an executable that is not created by an action of that rule. Is there another way around this? Aside from having to create the binary in an action in the rule, I what you've written is more or less what I'm doing, right?

[hlopko]

I think the rust_binary will deal with the extension automatically, so you won't need the installer

[UebelAndre]

The installer is just a script that helps us create the test runner/launcher in an action in the test rule. I made this target so I can use a batch file on windows and not require the use of bash.

[UebelAndre]

Also, I changed the runner to be called a launcher to be more consistent with upstream bazel: https://github.com/bazelbuild/bazel/tree/master/src/tools/launcher

[illicitonion]

There's a toolchain.os property I think you could use? The toolchain should be in the right configuration?

[UebelAndre]

I wasn't sure if the toolchain would always match the configuration of launcher but I think in most cases it's safe to use the toolchain. Updated