Do not use test launcher when not needed

[hlopko]

This backwards compatible change does 2 things:

• it uses testing.TestEnvironment to populate the environment for the rust_test execution. This is a new discovery that we were not aware of, and it mostly removes most common duties from our test launcher.
• stops using test launcher for situations when it is not needed (when there is no {pwd} placeholder expansion needed in environment variable values).

What will likely follow is an incompatible change that removes {pwd} expansion altogether (after I do the reserach to see if it is used :)

[UebelAndre]

Hmmm, on second though, I feel like this is something that should be an incompatibility flag. I don't think it's too difficult to have users join pwd and their value themselves. I'm definitely interested in continuing to use execpath but also want to have debugger support. Maybe rules_rust can provide some utility crate for expanding execpath into absolute paths? Not saying that utility has to be done in this PR just trying to figure out if there's a path to completely abolish the launcher and maintain the ability to use absolute paths.

[hlopko]

I think you're saying that you support my followup PR to remove the launcher alltogether using an incompatible flag, and that this PR is good to go.

Or did you mean that this PR needs an incompatible flag?

[UebelAndre]

I was thinking that there'd be no changes in behavior outside that flag. So unless it's flipped you'd still use the launcher. But perhaps that's more restrictive than necessary. But I wonder if anyone would run into issues in this change or be confused why two seemingly similar tests might behave differently.

[UebelAndre]

Or did you mean that this PR needs an incompatible flag?

This is my question and I think it does but hard to really make up my mind since my attention is currently divided 😅

[hlopko]

My experience is that people encounter issues and confusion when using the launcher. Except really unlikely situations I don't think we would be breaking people by not using the launcher. Of course if we stop producing absolute paths that can really break people and we should guard it with an incompatible change.

[hlopko]

If you think this PR warrants the incompatible flag, I d make it a different one from the one that removes absolute paths, since they both have different fallout impact and migration stories.

[UebelAndre]

I can accept this as a middle ground but hope that we can work quickly together to get a feature flag up. As mentioned in another comment. I think the solution for absolute paths would actually be in Bazel proper and not the rules here.

[UebelAndre]

What will likely follow is an incompatible change that removes {pwd} expansion altogether (after I do the reserach to see if it is used :)

One situation I can think of is tests which look for paths to files using environment variables.

if let Ok(exe) = env::var("MY_BIN") {
    Command::new(exe).spawn()
} else {
    // In non-test/normal runtime, this will rely on PATH
    Command::new("my-bin").spawn()
}

In this case I think there are solutions but it can be less convenient.

[hlopko]

Maybe we can move this discussion to slack, as soon as it recovers from the outage? :)

The use case you show is a reasonable one, but with the change I have in mind there would still be MY_BIN in the env, it's just that the path won't be absolute.

[UebelAndre]

The use case you show is a reasonable one, but with the change I have in mind there would still be MY_BIN in the env, it's just that the path won't be absolute.

Yeah, the issues folks will face is that Command::new("bazel-bin/foo/bar") is not the same as Command::new("/home/user/.cache/hash/bazel-bin/foo/bar"). But I think this is probably something that should be solved as an extension to the facny provider I just learned about 😄

[UebelAndre]

LGTM!