fix(test_env): export canonical CARGO_BIN_EXE env variable

[kwonoj]

• closes Enable //test/test_env on Windows CI #613

This PR attempts to support test_env test on Windows platform. Mainly, PR includes changes to export canonical env variable CARGO_BIN_EXE_[basename] without file extension regardless on platforms.

One thing this PR doesn't change is behavior around runfiles. Current test, and path expectation around exported CARGO_BIN_EXE relies on runfiles with symlink which is not enabled by default on Windows (https://bazel.googlesource.com/bazel/+/master/site/docs/windows.md#enable-symlink-support). Instead of fixing those, PR attempts to workaround by enabling symlink via bazelrc, which consumers of this rule also required to enable symlink support. I am not sure if this is expected or not - but probably worth separate discussion.

[kwonoj]

while doc suggests to enable runfiles (build --enable_runfiles), enabling it makes process_wrapper/std_err test fails somehow.

[UebelAndre]

What is the failure you see when enabling this?

[UebelAndre]

I ran some tests and it seems to test no problem for me. Though I used

Suggested change

	# https://bazel.googlesource.com/bazel/+/master/site/docs/windows.md#enable-symlink-support
	# https://bazel.googlesource.com/bazel/+/master/site/docs/windows.md#enable-symlink-support
	startup --windows_enable_symlinks

Per the recommendation from https://docs.bazel.build/versions/main/windows.html

Does that help here?

[kwonoj]

weirdly I tried again and couldn't repro the error. maybe one off from the system I used?

[kwonoj]

I think these test failures are by enabling runfiles, which is outside of test_env/.

(03:37:10) FAIL: //test/process_wrapper:stdout_test (see C:/b/h3ziskxk/execroot/rules_rust/bazel-out/x64_windows-fastbuild/testlogs/test/process_wrapper/stdout_test/test_attempts/attempt_1.log)

[hlopko]

Hmmm, could you try to use https://github.com/bazelbuild/rules_rust/blob/main/tools/runfiles/runfiles.rs#L55 in the test? Using runfiles library is how everything rusty should find runfiles. If that works you don't need symlinks on Windows.

Warning, I've never testing that code on Windows, I was just hoping it would work :)

[kwonoj]

@hlopko Happy to try, but mind elaborate bit as I may misunderstand few things?

It looks like current test want to verify path to binary via env variable (https://github.com/bazelbuild/rules_rust/pull/842/files#diff-1bf83a2d7f83a35ad4c8411142f1bd35af8656c4ac54beb38bd291018e29f9e9R3). This fails on Windows, due to symlink does not created to relative path to runfiles while env variable is set to short_path, which is relative to cwd test is running.

Runfiles::create seems returning path constructed based on manifest and can construct path like https://github.com/bazelbuild/rules_rust/blob/main/tools/runfiles/runfiles.rs#L91 - in this case, what's expected test cases to verify CARGO_BIN_EXE? should test try to lookup absolute path to binary by calculating between exported variable with runfiles paths?

[hlopko]

Oh just ignore me, I misinterpreted the problem and the test. Now I'm taking a deeper look.

1. Are you saying that execpath on Windows doesn't work (meaning it points to a location where on Linux would be a symlink, but on windows there is nothing) unless you enable symlinks? @meteorcloudy is it a known problem? Are we doing something wrong?

2. So I think the intention of the test was to verify that env attribute is make-variable-expanded. rootpath is enough for that. I propose deleting the part of the test that does execpath, especially if 1) turns out to be an existing problem. There are other make-variables that the test doesn't cover, like location, and IMHO it doesn't need to cover. I'm happy with just rootpath there.

CC @UebelAndre

[kwonoj]

1. Not sure if I understood q correctly, but CARGO_BIN_EXE_hello-world supposed to return relative path to hello-world binary from current dir where test runs (which points to runfiles). On non-Windows platform this relative path is available since symlink will points to actual binary location, but on Windows, symlink is not supported out of the box unless it is explicitly enabled. So looking for relative path from current location to spawning process will fail.

2. Am I reading correctly test would be suffecient to verify CARGO_BIN_EXE_hello-world exists? If so, what'd be recommended way for Windows users to actually construct paths to those binary (either absolute, or relative). Maybe I don't have enough knowledge around purpose of those env itself.

[meteorcloudy]

1. Are you saying that execpath on Windows doesn't work (meaning it points to a location where on Linux would be a symlink, but on windows there is nothing) unless you enable symlinks? @meteorcloudy is it a known problem? Are we doing something wrong?

No, execpath should work on Windows, otherwise the build won't work at all.

Can anyone describe what's the purpose of CARGO_BIN_EXE? Is it an absolute or relative path? If it's the runfile path (a relative path to your runfiles directory), then you'll have to use rlocation to map it to an absolute path on Windows during runtime.

[UebelAndre]

Can anyone describe what's the purpose of CARGO_BIN_EXE? Is it an absolute or relative path? If it's the runfile path (a relative path to your runfiles directory), then you'll have to use rlocation to map it to an absolute path on Windows during runtime.

The docs say:

CARGO_BIN_EXE_<name - The absolute path to a binary target's executable. This is only set when building an integration test or benchmark. This may be used with the env macro to find the executable to run for testing purposes. The <name> is the name of the binary target, exactly as-is. For example, CARGO_BIN_EXE_my-program for a binary named my-program. Binaries are automatically built when the test is built, unless the binary has required features that are not enabled.

So seems it's intended to be an absolute path which I'd expect to be something from execpath. I believe this is how most absolute paths are generated since pwd is assumed to be the execroot of the sandbox, right? From which the execpath of a file would create an absolute path? But as you say, there's rlocation as well?

[meteorcloudy]

Then sounds like you should use output.path instead of output.short_path for CARGO_BIN_EXE_<name>?
https://docs.bazel.build/versions/main/skylark/lib/File.html#short_path
https://docs.bazel.build/versions/main/skylark/lib/File.html#path

Or you can still use short_path (the runfile path), and always do a rlocation during runtime, but that requires whoever consumes the env var to depends on the runfiles library.

[kwonoj]

should use output.path instead of output.short_path

output.path also gives relative path but it's relative to execution dir. since cwd of test runner is runfiles, it still requires to calculate full absolute path.

[UebelAndre]

should use output.path instead of output.short_path

output.path also gives relative path but it's relative to execution dir. since cwd of test runner is runfiles, it still requires to calculate full absolute path.

rules_rust uses a process_wrapper which can replace the use of ${pwd} in arguments and environment variables. If the value is "${{pwd}}/{}".format(output.path) then it should be resolved to an absolute path when the action is running.

[UebelAndre]

Friendly ping on this PR 😄 What's the remaining actions for it?

[kwonoj]

Sorry to get back late.

I think process_wrapper solves partially. For example, this test https://github.com/bazelbuild/rules_rust/pull/842/files#diff-1bf83a2d7f83a35ad4c8411142f1bd35af8656c4ac54beb38bd291018e29f9e9R27 expected $rootpath expanded path should be relative to cwd, which hits same issues as CARGO_BIN_EXE (without help of process_wrapper). This test fails on Windows as relative path is constructed to runfiles points to symlinked binary which doesn't exist.

[UebelAndre]

I ran some tests and it seems to test no problem for me. Though I used

Suggested change

	# https://bazel.googlesource.com/bazel/+/master/site/docs/windows.md#enable-symlink-support
	# https://bazel.googlesource.com/bazel/+/master/site/docs/windows.md#enable-symlink-support
	startup --windows_enable_symlinks

Per the recommendation from https://docs.bazel.build/versions/main/windows.html

Does that help here?

[UebelAndre]

I think this needs to be output.path since ${pwd} points to the exec root

Suggested change

	env["CARGO_BIN_EXE_" + env_basename] = "${{pwd}}/{}".format(dep_crate_info.output.short_path)
	env["CARGO_BIN_EXE_" + env_basename] = "${{pwd}}/{}".format(dep_crate_info.output.path)

[UebelAndre]

Ok, I think I understand what's happening now.

I think this is incorrect

rules_rust/test/test_env/tests/run.rs

Line 3 in c2f5701

let path = env!("CARGO_BIN_EXE_hello-world");

CARGO_BIN_EXE is documented to only be set for integration tests and benchmarks. Cargo will set this value to the absolute path of the binary after it's built which means the file is present in the target directory (eg: /home/user/hello_world/target/debug/hello-world.exe). If we were to use the process wrapper to try and resolve "${pwd}/" + binary_file.path, it would correctly do so but the resulting path would be a path inside a sandbox (/private/var/tmp/_bazel_user/76282c66b0dfe3c5cb9a230bdc913a52/sandbox/darwin-sandbox/13/execroot/rules_rust/bazel-out/darwin-fastbuild/bin/test/test_env/hello-world). I suspect the reason binary_file.short_path was used here was because it was more consistent during runtime. But again, I think this is incorrect and does not do what the variable communicates.

The rules do not really have a distinction between unit and integration test targets so I don't know that it's currently possible to match the behavior of CARGO_BIN_EXE. At this point, I think it's probably better to leave it as a variable defined for all test targets and set to short_path. I wonder if it's possible to communicate this to users somehow?

@kwonoj What do you think of that?

Also (slight tangent), I'm not sure how the variable manages to only be set for tests. That block of code is a bit confusing...

rules_rust/rust/private/rustc.bzl

Lines 501 to 506 in c2f5701

	# Make bin crate data deps available to tests.
	for data in getattr(attr, "data", []):
	if rust_common.crate_info in data:
	dep_crate_info = data[rust_common.crate_info]
	if dep_crate_info.type == "bin":
	env["CARGO_BIN_EXE_" + dep_crate_info.output.basename] = dep_crate_info.output.short_path

[kwonoj]

At this point, I think it's probably better to leave it as a variable defined for all test targets and set to short_path. I wonder if it's possible to communicate this to users somehow?

As long as rules_rust documents symlink is prerequite for Windows platform, I think short_path would work. There are other cases already works under assumption of symlink (such as #842 (comment)) as well.

It is more desirable if rule itself works without enabling symlink for the Windows, but current impression is existing rules are heavily relying on it and changing those behaviors might cause big breaking changes unfortunately.

[UebelAndre]

I think #861 is probably the most reasonable state for CARGO_BIN_EXE but that does assume symlinking is enabled and does not use an absolute path. Though, I think that's been fine for most crates? Unless there are existing issues I missed on rules_rust.

My opinion is that windows should be expected to enable symlinking unless there's a sizable surge in windows users who are willing to contribute to the rules and change that. In a perfect world we wouldn't require symlinking but there's currently not enough windows expertise or bandwidth to handle all those cases 😞. The best we can currently offer is to try to make the rules work with windows systems that conform to linux-style things (like symlinks).

Does that help advance the PR and solve for the functionality you're after?

[UebelAndre]

Hey, friendly ping here. Is there anything I can do to help with this PR?

[kwonoj]

Sorry, on a sick leave couldn't have access for a while. I'll try to get back asap.

[kwonoj]

Sorry for taking this long, tied with daily work and had sick leave as well.

I have updated PR for cargo_env variable as suggested. It requires symlink still, backs to relative path calculation instead of pwd based absolute path. Please let me know if there's additional change required.

My opinion is that windows should be expected to enable symlinking unless there's a sizable surge in windows users who are willing to contribute to the rules and change that. In a perfect world we wouldn't require symlinking

I agree with this, and the reason I brought PR with enable symlink for Windows as well. I do agree if this is greenfield implementation, or relatively easy to change without symlink it is much desired behavior. Eventually that'd be ideal goal, but I do expect it'll require non trivial size of effort with potentially noticeable breaking changes to existing platforms. For now, the best path seems to let Windows users aware of the prerequisites to enable symlink.

[UebelAndre]

This looks good to me! Thank you so much!

[UebelAndre]

Sorry for taking this long, tied with daily work and had sick leave as well.

Hey, no need to apologize at all. Health has to come first and then you gotta pay the bills. Hope all is well with you 😄

I have updated PR for cargo_env variable as suggested. It requires symlink still, backs to relative path calculation instead of pwd based absolute path. Please let me know if there's additional change required.

Again, this looks good to me, thanks so much for putting this together!

My opinion is that windows should be expected to enable symlinking unless there's a sizable surge in windows users who are willing to contribute to the rules and change that. In a perfect world we wouldn't require symlinking

I agree with this, and the reason I brought PR with enable symlink for Windows as well. I do agree if this is greenfield implementation, or relatively easy to change without symlink it is much desired behavior. Eventually that'd be ideal goal, but I do expect it'll require non trivial size of effort with potentially noticeable breaking changes to existing platforms. For now, the best path seems to let Windows users aware of the prerequisites to enable symlink.

Yeah, hopefully maintainers and contributors can work on achieving this goal together!