Support RUNFILES_MANIFEST_FILE Set to an Empty Value

[levymp]

Currently, when using the runfiles library in an environment where RUNFILES_MANIFEST_FILE is provided, but set to an empty value results in an error. The rules_python runfiles library supports the environment variable being set to an empty value.

The rules_rust runfiles library should also support an empty RUNFILES_MANIFEST_FILE value and fallback on a directory based approach.

[UebelAndre]

@fmeum do you know if this is the correct behavior for runfiles?

[fmeum]

@fmeum do you know if this is the correct behavior for runfiles?

I would say it doesn't really matter. Bazel itself never sets the variables to an empty value. Treating empty variables as equivalent to them not being set is pretty common, I guess.

[UebelAndre]

@fmeum I created https://github.com/UebelAndre/runfiles_api to see what differences exist among runfiles implementations and it seems of only cc and rust (due to this change) agree on how to parse runfiles. I'm wondering if you have access to any documentation on the runfiles API that I can add to this (and then maybe this is something that can live in a more official home).

[arrdem]

Thanks for patching this, the runfiles.sh implementation in tools_sh actually can create this scenario and I've been carrying a patch for this in aspect_rules_py.

[fmeum]

@UebelAndre I took a look at the tests now:

1. test_runfiles_dir_with_manifest sets up a situation in which only the so-called "output manifest" (foo.runfiles/MANIFEST) is available. Bazel never produces this setup and rules_go intentionally doesn't support it. This may also apply for the other runfiles libraries.
2. test_runfiles_dir_empty_manifest_env is the situation in 1. plus the original motivation for this issue - I would be wary of writing this into any standard, but if a library wants to support this case, doing so is likely harmless.
3. Generally speaking I'm not sure whether the test setup works for languages in which the program under test itself needs runfiles. I think that you would have to copy all runfiles into the runfiles structure under test.

[UebelAndre]

@fmeum

@UebelAndre I took a look at the tests now:

1. `test_runfiles_dir_with_manifest` sets up a situation in which only the so-called "output manifest" (foo.runfiles/MANIFEST) is available. Bazel never produces this setup and rules_go intentionally doesn't support it. This may also apply for the other runfiles libraries.

I'm pretty sure I ran into this which lead me to implementing #3399. Though I don't fully remember the context. Maybe it was for test? Should the behavior be deleted in that caes?

2. test_runfiles_dir_empty_manifest_env is the situation in 1. plus the original motivation for this issue - I would be wary of writing this into any standard, but if a library wants to support this case, doing so is likely harmless.

Yeah, this seems more like it'd a bug in Bazel then anything else so I think it's fine to ignore.

3. Generally speaking I'm not sure whether the test setup works for languages in which the program under test itself needs runfiles. I think that you would have to copy all runfiles into the runfiles structure under test.

I would agree in some contexts the use of a manifest file doesn't really make sense but I know on windows it's unfortunately something that is quite common. In rules I've written for tools that need a runfiles tree, I've had to build this from the manifest file in process wrappers wherever there isn't one and in this case I care immensely that the runfiles API available works as expected and not just assuming they're in one form or another. You'll notice for python and shell I've done some inlining to be able to test the implementation outside of the environment constructed by the rules. I think this is important because there may be other variations of each set of rules (rules_python, rules_py, rules_venv, etc) that all use the same runfiles library (kinda as a requirement from the language) so the implementation should follow whatever the specification is.