Bazel: Add common env attribute for *_binary and *_test

[Globegitter]

Description of the problem / feature request:

In the same way as we can define in code that bazel passes on cli args on bazel run and bazel test it would be great to be able to define env vars in the same way.

Feature requests: what underlying problem are you trying to solve with this feature?

Some applications/libraries/tests/test frameworks prefer environment variables over cli args and being able to define that on a per target level in code would save the need to use external tools or test_env in a lot of cases. We also have cases where we just need env variables in ci for certain tests (that use docker) and this could also accommodate for that. Also with make var substitution or stamping we could even pass in non static env vars this way.

This could also help solving part of the issue described in #6111

Also I noticed on our team, developers where quite surprised when they saw one can set args in code but not env. So it seems some developers even have some expectations around this behaviour.

One could of course work around this by wrapping the binary into a sh_binary or similar and that is probably a reasonable solution, but the same would be true for args so parity on this would be ideal.

Have you found anything relevant by searching the web?

Not really, maybe #955 is somewhat related.

[Globegitter]

Also one interesting case we ran into where we did not just need a hardcoded env var, but one with an absolut path to the location of some generated files, so now we are wrapping that target and setting $(pwd)/some/path, so one interesting question for such feature would be if the env vars would get quoted, i.e. take the value literal or of this would just pass it through as is and therefore apply standard shell mechanisms and pwd would expand to a path.

But either way this would be a nice addition.

[lberki]

I don't have any fundamental counter-arguments, except that it's Yet Another Feature... I would expect that since you control both the test rule and the code it runs, it's just as easy to just do export ENV=value at the very beginning of our test (with value potentially coming from the command line)

@aehlig , mind giving a second opinion?

[lberki]

(pull request is https://github.com/bazelbuild/bazel/issues/7364)

/cc @lberki

[blico]

A little more context on why this feature would be valuable for us:
There is a common environment variable that a lot of tests and binaries within our repo depend on during execution time. The environment variable's value needs to be a relative path to another directory within the repo. When auto-generating BUILD files we can determine the location of this directory, so if the envs attribute existed we could also set the proper env value in the generated rules. This way developers would never have to worry about adding an export ENV=value into the sources of their tests/binaries.

[lberki]

Erm, sorry about the delay.

If we added this feature, we would have to worry about maintaining it forever... I think if there were more people who wanted it, I would be more amenable to it, but as it is, I don't want to add a single-use feature to Bazel.

I'll take a look at the code review, but I'm not very enthusiastic and would much prefer not to merge it.

[blico]

@lberki is there anything that would make you more enthusiastic about this change?

I originally tried to add the feature into rules_go https://github.com/bazelbuild/rules_go/pull/1973, but it was recommend that the feature belongs here. As my earlier comment explains, the feature would be very valuable for our repo.

[lberki]

More use cases and more people to whom this would be useful.

[wesleyw72]

I have a use case need for this. I have a cc_binary which dynamic links against a shared object Liba.so. I’d like to test runtime polymorphism of two different implementations of the same shared library , so in a test I would like to specify a different Liba.so on the LD_LIBRARY_PATH. Having the env attribute would help a lot.

[wesleyw72]

@lberki is this still something you'd be opposed to merging?

[ulfjack]

I would like to have this feature for Google to remove some internal hacks in test execution, and I have a patch to implement it.

[ulfjack]

https://bazel.googlesource.com/bazel/+/02d28a26c58d96b0f130d9669774f76a83375086

[Globegitter]

@ulfjack I can not find the commit here on github. Is the plan to get this patch merged into open source also?

[ulfjack]

It wasn't merged yet - Google doesn't have a separate code base for Bazel. :-)

There are some open questions that I haven't addressed yet. I was considering changing the name of the attribute to test_env. The other main question was whether to signal inheritance inline like I did by using a special value or to use separate attributes. Technically, it would be sufficient to only support inheriting env variables, but not setting them (if you set an env variable to a fixed value, you can also do that inside the test, rather than outside, e.g., by wrapping it in a shell script).

[blico]

Technically, it would be sufficient to only support inheriting env variables, but not setting them (if you set an env variable to a fixed value, you can also do that inside the test, rather than outside, e.g., by wrapping it in a shell script).

We find value in being able to set env variables via attribute rather than inside tests.

For example, one use case we have is the ability to conditionally set env variables for a subset of our repo. Meaning we have a config_setting defined with narrow visibility and based off of that we use a select statement on the test_env attr for certain test rules.
AFAIK the only other way we could achieve this same functionality would be via a select on the args attr, and then conditionally setting the env variable in code based off of the passed in args. In my opinion, the first approach is much more elegant and straight-forward.

[TheKaur]

@ulfjack Any update on when this will be merged?

[ulfjack]

I left Google at the end of January, and I've been exceptionally busy with my new company (www.engflow.com). Not sure when I will have time to look into this again.

[mcwilson07]

This would be extremely useful for setting the ASAN environment variables when executing tests.

[linzhp]

Running into another case where this is needed. The Go package github.com/mattn/go-oci8 requires LD_LIBRARY_PATH to point to Oracle instant client location at its init function. Setting it in the Go tests or TestMain is too late because init is called and failed before that. We can't export LD_LIBRARY_PATH before running the test because some other tests may depend on other C libraries and need a different LD_LIBRARY_PATH.

@lberki Are the use cases and enthusiasm so far enough? According to @ulfjack, it seems Google would also benefit from this too by reducing some internal hacks.

[flaub]

We would also very much like this feature. Many tools use environment variables for configuration rather than just command line args.

[virusdave]

Adding support for this request. We have several use cases where we want to specify that a particular portion of the execution environment (a set of environment variables, specifically) is depended upon explicitly by a small subset of our build graph, but not the rest. Adding these variables to the build or test environment for the whole graph means caching is busted and targets rebuilt/retested whenever they change, when really it should only be the affected subset.

If you squint at this, it's very analogous to a file dependency. I don't want my entire repo rebuilt or the cache invalidated when a file FOO (depended upon by a single target BAR) changes, i only want BAR rebuilt. The same is true for some environment variables.

For real world examples, i can provide a couple. Most immediate one i ran into is:

Small set of tests which are non-cacheable and have legitimate per-machine dependencies. You don't want all tests to become uncacheable because a small set of them require cache-key-busting transitive dependencies on environment variables, but you _do_ want them executed when their dependencies change.

However, i've run into more cases as well. Part of bazel's power comes from allowing the modeling of build steps as cacheable functions from (environment, sources, tools) => (outputs). It provides great mechanisms for maximizing cache hits by restricting the domain in the case of both sources and tools, but not really for environment, which currently is effectively "the union of the environments required by all tools, toolchains, binaries, and targets", and that's the opposite of what you want for maximizing cache hits. We need to be able to apply a projection of the outer, union environment into minimal smaller environments as needed for individual tools, tests, or runs. Allowing explicit specification of such dependencies is one way to accomplish this projection, assuming it's coordinated with cache key calculation correctly.

[aiuto]

@gregestren Is this really a configurability issue?

[wesleyw72]

It seems there may be community support for implementing and getting the change through review from the discussion on this thread. I think one unknown is still whether Google would accept this patch. Could @lberki or another Google Bazel team provide some guidance on this?

[linzhp]

@ulfjack already have a patch up for review. We just need to raise the awareness of the patch among reviewers.

[aiuto]

cc/ @sdtwigg

[dfreese]

I've run into a similar situation where this would be extremely helpful. When testing allocation failures, asan will complain. This requires setting ASAN_OPTIONS. Being able to set ASAN_OPTIONS="allocator_may_return_null=1" for a specific test, would be extremely helpful.

The alternative, currently, is to tag the test, filter it from our primary asan config, and create a secondary "asan_alloc_null_allow" config.

[dws]

This would make it convenient for our bazel macros to set environment variables such as OMP_NUM_THREADS and MKL_NUM_THREADS on a per-test basis to reflect the per-test value of the cpu:N tag.

[jbms]

I'd like to add my support for this feature as well. I have run into a need for it numerous times. Of course there are workarounds, like modifying the test code to accept the relevant parameter as a command-line argument, or creating a wrapper with genrule, but environment variables are an extremely common interface for specifying program and test behavior. It isn't like environment variable are going to go away, and you could likewise eliminate the need for args by using a genrule wrapper as well. Additionally, environment variables are often a lot more convenient than command-line arguments, because they automatically pass through any number of layers of test framework without interference.

[aiuto]

I would be interested in knowing how far having parameterized tests as per https://github.com/bazelbuild/bazel/issues/11772 would work for people. I very much want to add that feature as part of an effort to improve our integration tests.

[konste]

In many cases devs working on the build are separate from the devs working on the product code and tests and build devs don't have much say in how the tests are organized. Or there are just a ton of legacy tests nobody have time to rewrite. For those reasons it is important for the build system (with the help of build devs) to accommodate for existing test requirements which VERY often include setting the environment variables in a specific way. That's why we need this feature as is and test parameters would not help with it.

[jbms]

I'm not clear on how the exactly the parameterized tests proposed in #11772 would work, but regardless I agree with @konste that such a facility would surely be largely orthogonal to the ability to set environment variables.

[konste]

Woohoo! Thank you!

[dws]

Yay!

[sulume]

I'm having trouble and landed here today, and surprised that it's nicely resolved just a week ago. Thank you!
Is this coming in the next minor release, while 4.0 is happening?