Do not use test launcher when not needed (#960)

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 :)

Author: hlopko

docs/defs.md

@@ -550,7 +550,7 @@ Run the test with `bazel build //hello_lib:hello_lib_test`.
 | <a id="rust_test-data"></a>data |  List of files used by this rule at compile time and runtime.<br><br>If including data at compile time with include_str!() and similar, prefer <code>compile_data</code> over <code>data</code>, to prevent the data also being included in the runfiles.   | <a href="https://bazel.build/docs/build-ref.html#labels">List of labels</a> | optional | [] |
 | <a id="rust_test-deps"></a>deps |  List of other libraries to be linked to this library target.<br><br>These can be either other <code>rust_library</code> targets or <code>cc_library</code> targets if linking a native library.   | <a href="https://bazel.build/docs/build-ref.html#labels">List of labels</a> | optional | [] |
 | <a id="rust_test-edition"></a>edition |  The rust edition to use for this crate. Defaults to the edition specified in the rust_toolchain.   | String | optional | "" |
-| <a id="rust_test-env"></a>env |  Specifies additional environment variables to set when the test is executed by bazel test. Values are subject to <code>$(execpath)</code> and ["Make variable"](https://docs.bazel.build/versions/master/be/make-variables.html) substitution.   | <a href="https://bazel.build/docs/skylark/lib/dict.html">Dictionary: String -> String</a> | optional | {} |
+| <a id="rust_test-env"></a>env |  Specifies additional environment variables to set when the test is executed by bazel test. Values are subject to <code>$(rootpath)</code>, <code>$(execpath)</code>, location, and ["Make variable"](https://docs.bazel.build/versions/master/be/make-variables.html) substitution.<br><br>Execpath returns absolute path, and in order to be able to construct the absolute path we need to wrap the test binary in a launcher. Using a launcher comes with complications, such as more complicated debugger attachment.   | <a href="https://bazel.build/docs/skylark/lib/dict.html">Dictionary: String -> String</a> | optional | {} |
 | <a id="rust_test-proc_macro_deps"></a>proc_macro_deps |  List of <code>rust_library</code> targets with kind <code>proc-macro</code> used to help build this library target.   | <a href="https://bazel.build/docs/build-ref.html#labels">List of labels</a> | optional | [] |
 | <a id="rust_test-rustc_env"></a>rustc_env |  Dictionary of additional <code>"key": "value"</code> environment variables to set for rustc.<br><br>rust_test()/rust_binary() rules can use $(rootpath //package:target) to pass in the location of a generated file or external tool. Cargo build scripts that wish to expand locations should use cargo_build_script()'s build_script_env argument instead, as build scripts are run in a different environment - see cargo_build_script()'s documentation for more.   | <a href="https://bazel.build/docs/skylark/lib/dict.html">Dictionary: String -> String</a> | optional | {} |
 | <a id="rust_test-rustc_env_files"></a>rustc_env_files |  Files containing additional environment variables to set for rustc.<br><br>These files should  contain a single variable per line, of format <code>NAME=value</code>, and newlines may be included in a value by ending a line with a trailing back-slash (<code>\</code>).<br><br>The order that these files will be processed is unspecified, so multiple definitions of a particular variable are discouraged.   | <a href="https://bazel.build/docs/build-ref.html#labels">List of labels</a> | optional | [] |

docs/flatten.md

@@ -1094,7 +1094,7 @@ Run the test with `bazel build //hello_lib:hello_lib_test`.
 | <a id="rust_test-data"></a>data |  List of files used by this rule at compile time and runtime.<br><br>If including data at compile time with include_str!() and similar, prefer <code>compile_data</code> over <code>data</code>, to prevent the data also being included in the runfiles.   | <a href="https://bazel.build/docs/build-ref.html#labels">List of labels</a> | optional | [] |
 | <a id="rust_test-deps"></a>deps |  List of other libraries to be linked to this library target.<br><br>These can be either other <code>rust_library</code> targets or <code>cc_library</code> targets if linking a native library.   | <a href="https://bazel.build/docs/build-ref.html#labels">List of labels</a> | optional | [] |
 | <a id="rust_test-edition"></a>edition |  The rust edition to use for this crate. Defaults to the edition specified in the rust_toolchain.   | String | optional | "" |
-| <a id="rust_test-env"></a>env |  Specifies additional environment variables to set when the test is executed by bazel test. Values are subject to <code>$(execpath)</code> and ["Make variable"](https://docs.bazel.build/versions/master/be/make-variables.html) substitution.   | <a href="https://bazel.build/docs/skylark/lib/dict.html">Dictionary: String -> String</a> | optional | {} |
+| <a id="rust_test-env"></a>env |  Specifies additional environment variables to set when the test is executed by bazel test. Values are subject to <code>$(rootpath)</code>, <code>$(execpath)</code>, location, and ["Make variable"](https://docs.bazel.build/versions/master/be/make-variables.html) substitution.<br><br>Execpath returns absolute path, and in order to be able to construct the absolute path we need to wrap the test binary in a launcher. Using a launcher comes with complications, such as more complicated debugger attachment.   | <a href="https://bazel.build/docs/skylark/lib/dict.html">Dictionary: String -> String</a> | optional | {} |
 | <a id="rust_test-proc_macro_deps"></a>proc_macro_deps |  List of <code>rust_library</code> targets with kind <code>proc-macro</code> used to help build this library target.   | <a href="https://bazel.build/docs/build-ref.html#labels">List of labels</a> | optional | [] |
 | <a id="rust_test-rustc_env"></a>rustc_env |  Dictionary of additional <code>"key": "value"</code> environment variables to set for rustc.<br><br>rust_test()/rust_binary() rules can use $(rootpath //package:target) to pass in the location of a generated file or external tool. Cargo build scripts that wish to expand locations should use cargo_build_script()'s build_script_env argument instead, as build scripts are run in a different environment - see cargo_build_script()'s documentation for more.   | <a href="https://bazel.build/docs/skylark/lib/dict.html">Dictionary: String -> String</a> | optional | {} |
 | <a id="rust_test-rustc_env_files"></a>rustc_env_files |  Files containing additional environment variables to set for rustc.<br><br>These files should  contain a single variable per line, of format <code>NAME=value</code>, and newlines may be included in a value by ending a line with a trailing back-slash (<code>\</code>).<br><br>The order that these files will be processed is unspecified, so multiple definitions of a particular variable are discouraged.   | <a href="https://bazel.build/docs/build-ref.html#labels">List of labels</a> | optional | [] |

rust/private/rust.bzl

@@ -306,13 +306,14 @@ def _rust_binary_impl(ctx):
         ),
     )
 
-def _create_test_launcher(ctx, toolchain, output, providers):
+def _create_test_launcher(ctx, toolchain, output, env, providers):
     """Create a process wrapper to ensure runtime environment variables are defined for the test binary
 
     Args:
         ctx (ctx): The rule's context object
         toolchain (rust_toolchain): The current rust toolchain
         output (File): The output File that will be produced, depends on crate type.
+        env (dict): Dict of environment variables
         providers (list): Providers from a rust compile action. See `rustc_compile_action`
 
     Returns:
@@ -337,20 +338,12 @@ def _create_test_launcher(ctx, toolchain, output, providers):
         is_executable = True,
     )
 
-    # Get data attribute
-    data = getattr(ctx.attr, "data", [])
-
     # Expand the environment variables and write them to a file
     environ_file = ctx.actions.declare_file(launcher_filename + ".launchfiles/env")
-    environ = expand_dict_value_locations(
-        ctx,
-        getattr(ctx.attr, "env", {}),
-        data,
-    )
 
     # Convert the environment variables into a list to be written into a file.
     environ_list = []
-    for key, value in sorted(environ.items()):
+    for key, value in sorted(env.items()):
         environ_list.extend([key, value])
 
     ctx.actions.write(
@@ -454,8 +447,21 @@ def _rust_test_common(ctx, toolchain, output):
         crate_info = crate_info,
         rust_flags = ["--test"] if ctx.attr.use_libtest_harness else ["--cfg", "test"],
     )
+    data = getattr(ctx.attr, "data", [])
+
+    env = expand_dict_value_locations(
+        ctx,
+        getattr(ctx.attr, "env", {}),
+        data,
+    )
+    providers.append(testing.TestEnvironment(env))
 
-    return _create_test_launcher(ctx, toolchain, output, providers)
+    if any(["{pwd}" in v for v in env.values()]):
+        # Some of the environment variables require expanding {pwd} placeholder at runtime,
+        # we need a launcher for that.
+        return _create_test_launcher(ctx, toolchain, output, env, providers)
+    else:
+        return providers
 
 def _rust_test_impl(ctx):
     """The implementation of the `rust_test` rule
@@ -630,8 +636,12 @@ _rust_test_attrs = {
         mandatory = False,
         doc = dedent("""\
             Specifies additional environment variables to set when the test is executed by bazel test.
-            Values are subject to `$(execpath)` and
+            Values are subject to `$(rootpath)`, `$(execpath)`, location, and
             ["Make variable"](https://docs.bazel.build/versions/master/be/make-variables.html) substitution.
+
+            Execpath returns absolute path, and in order to be able to construct the absolute path we
+            need to wrap the test binary in a launcher. Using a launcher comes with complications, such as
+            more complicated debugger attachment.
         """),
     ),
     "use_libtest_harness": attr.bool(

test/test_env/BUILD.bazel

@@ -26,6 +26,5 @@ rust_test(
     env = {
         "FERRIS_SAYS": "Hello fellow Rustaceans!",
         "HELLO_WORLD_BIN_ROOTPATH": "$(rootpath :hello-world)",
-        "HELLO_WORLD_SRC_EXECPATH": "$(execpath :hello_world_main)",
     },
 )

test/test_env/tests/run.rs

@@ -26,10 +26,4 @@ fn run() {
     );
     assert!(!hello_world_bin.is_absolute());
     assert!(hello_world_bin.exists());
-
-    // Ensure `execpath` expanded variables map to real files and have absolute paths
-    let hello_world_src =
-        std::path::PathBuf::from(std::env::var("HELLO_WORLD_SRC_EXECPATH").unwrap());
-    assert!(hello_world_src.is_absolute());
-    assert!(hello_world_src.exists());
 }

test/test_env_launcher/BUILD.bazel

@@ -0,0 +1,31 @@
+load(
+    "//rust:rust.bzl",
+    "rust_binary",
+    "rust_test",
+)
+
+rust_binary(
+    name = "hello-world",
+    srcs = ["src/main.rs"],
+    edition = "2018",
+)
+
+filegroup(
+    name = "hello_world_main",
+    srcs = ["src/main.rs"],
+)
+
+rust_test(
+    name = "test",
+    srcs = ["tests/run.rs"],
+    data = [
+        ":hello-world",
+        ":hello_world_main",
+    ],
+    edition = "2018",
+    env = {
+        "FERRIS_SAYS": "Hello fellow Rustaceans!",
+        "HELLO_WORLD_BIN_ROOTPATH": "$(rootpath :hello-world)",
+        "HELLO_WORLD_SRC_EXECPATH": "$(execpath :hello_world_main)",
+    },
+)

test/test_env_launcher/src/main.rs

@@ -0,0 +1,3 @@
+fn main() {
+    println!("Hello world");
+}

test/test_env_launcher/tests/run.rs

@@ -0,0 +1,35 @@
+#[test]
+fn run() {
+    let path = env!("CARGO_BIN_EXE_hello-world");
+    let output = std::process::Command::new(path)
+        .output()
+        .expect("Failed to run process");
+    assert_eq!(&b"Hello world\n"[..], output.stdout.as_slice());
+
+    // Test the `env` attribute of `rust_test` at run time
+    assert_eq!(
+        std::env::var("FERRIS_SAYS").unwrap(),
+        "Hello fellow Rustaceans!"
+    );
+
+    // Test the behavior of `rootpath` and that a binary can be found relative to current_dir
+    let hello_world_bin =
+        std::path::PathBuf::from(std::env::var_os("HELLO_WORLD_BIN_ROOTPATH").unwrap());
+
+    assert_eq!(
+        hello_world_bin.as_path(),
+        std::path::Path::new(if std::env::consts::OS == "windows" {
+            "test/test_env_launcher/hello-world.exe"
+        } else {
+            "test/test_env_launcher/hello-world"
+        })
+    );
+    assert!(!hello_world_bin.is_absolute());
+    assert!(hello_world_bin.exists());
+
+    // Ensure `execpath` expanded variables map to real files and have absolute paths
+    let hello_world_src =
+        std::path::PathBuf::from(std::env::var("HELLO_WORLD_SRC_EXECPATH").unwrap());
+    assert!(hello_world_src.is_absolute());
+    assert!(hello_world_src.exists());
+}