Add common env attribute for _binary and _test #7364

Globegitter · 2019-02-06T08:18:57Z

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.

The text was updated successfully, but these errors were encountered:

Globegitter · 2019-03-01T15:24:06Z

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 · 2019-03-07T12:38:18Z

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 · 2019-03-07T12:39:02Z

(pull request is #7364)

/cc @lberki

blico · 2019-03-08T19:17:11Z

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 · 2019-03-26T10:13:23Z

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 · 2019-04-08T21:12:49Z

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

I originally tried to add the feature into rules_go bazel-contrib/rules_go#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 · 2019-06-19T08:26:23Z

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

wesleyw72 · 2020-01-18T10:49:20Z

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 · 2020-01-20T10:47:57Z

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

ulfjack · 2020-01-21T12:19:19Z

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 · 2020-01-21T12:45:11Z

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

Globegitter · 2020-02-11T10:52:58Z

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

ulfjack · 2020-02-11T12:34:53Z

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 · 2020-02-14T17:41:55Z

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 · 2020-03-19T01:00:49Z

@ulfjack Any update on when this will be merged?

ulfjack · 2020-03-20T16:45:13Z

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 · 2020-03-31T19:20:15Z

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

linzhp · 2020-04-15T19:14:46Z

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.

fmeum · 2022-02-16T09:23:16Z

@lberki Friendly ping. I am generally willing to work on this feature request, but wouldn't want contributions to get stuck in review due to this being labeled P4. In fact, isn't this essentially a P2 issue already due to it being a blocker for e.g. a starlarkified cc_binary?

linzhp · 2022-02-16T23:52:04Z

@fmeum I think the turnaround time for reviewing pull requests is much better than responding to issues. I had two pull requests getting merged in reasonable time.

meteorcloudy · 2022-02-17T09:12:03Z

@oquenchil As we're making progress on starlarkify cc rules, we probably should bump the priority of this issue?
@fmeum A PR on this would be welcome!

fmeum · 2022-02-17T11:20:10Z

@fmeum I think the turnaround time for reviewing pull requests is much better than responding to issues. I had two pull requests getting merged in reasonable time.

I have fared well with this strategy so far, so why not try it again. This PR adds inherited_environment to testing.TestEnvironment: #14849.

comius · 2022-03-16T13:50:53Z

Is adding inherited_environment to testing.TestEnvironment a sufficient solution or is there something following this?

I don't see a problem, giving rule writers an option to use inherited_environment and this being used on some special mostly testing rules that need it.

The title of the bug says "Add common env attribute for *_binary and *_test". I oppose adding any new attributes like this to standard rules that Bazel implements, because they would hurt hermeticity.

Adding it to TestEnvironment and providing extendable standard rules in Starlark, does allow anybody to create custom rules like *_test with it, even if this is a bad idea.

rickeylev · 2022-03-16T16:35:48Z

A missing part is making run + binary work. TestEnvironment is only respected if the rule is a test. A binary returning TestEnvironment will have the TestEnvironment provider ignored.

I'm running into this as part of Starlarkifying the Python rules, where some targets set env for a binary. Given that env is already public, without this feature, an incompatible change is inevitable. It sounds like this problem isn't limited to just the Python rules, either.

I dug through the Bazel code yesterday trying to figure out if there's any way a Starlark rule, or special builtin Starlark rule with a Java helper, could make this work without any modifications to (core) Bazel. I don't think there is; there just isn't any point where Starlark code could try to inject or set something somewhere to cause the necessary, lower-level, logic to run.

The three basic options appear to be:

Check for some provider binary rules can return (probably in RunCommand.java; it already has special code for test+TestEnvironment) (this is what jbms said a few comments back).
Make env/env_inherit present for all binary rules. Prior comments indicate this is incompatible and breaks the docker rules. So either accept the incompatible change, or rules/targets need some indicator that they're opting in to this behavior.
Let rules define their own env/env_inherit attributes, and have a third indicator somewhere that indicates they are to be specially handled. I guess the "flags" setting of an attribute?

(From what I can tell, (2) and (3) would probably require changes within RuleConfiguredTargetBuilder.java#build and/or RunfilesSupport.java#computeActionEnvironmentModify; those seem to be the common code paths that a Starlark rule will go through where the envvars are computed. The logic to do so is already there, it's just only run for test rules)

IMHO, (1) seems most preferable. A provider allows a rule to control, filter, and transform the env/env_inherits values before passing them along. There may be a bit of extra boilerplate to e.g. expand makevars, but I don't think that's a big deal. It's easy to imagine that a rule might want to reject or warn about certain envvars, or silently ignore them, or silently tweak them. If Bazel team is still hesitant about this, we can restrict creation of it to builtin rules and revisit making it more generally available later.

Always present attributes that are "magical" and implemented directly by Bazel have the advantage of guaranteed consistent behavior. But that's also their major drawback: no control. Having to modify core Bazel (and hence wait for a bazel release) just to pass along some envvar also seems annoying (but to be fair, I wouldn't expect this logic to change much).

fmeum · 2022-03-16T20:17:01Z

Is adding inherited_environment to testing.TestEnvironment a sufficient solution or is there something following this?

What I had in mind aligns with @rickeylev's option (1) and also what @jbms proposed: Introduce the equivalent of testing.TestEnvironment for non-test executable Starlark rules, which would then allow these rules to implement their own env and env_inherit attributes.

Adds an inherited_environment field to testing.TestEnvironment to allow Starlark rules to implement the equivalent of the native rules' `env_inherit` attribute. Work towards bazelbuild#7364. To fully resolve that issue, it remains to handle executable non-test rules. RELNOTES: Starlark test rules can use the new inherited_environment parameter of testing.TestEnvironment to specify environment variables whose values should be inherited from the shell environment.

comius · 2022-03-17T06:51:24Z

Is adding inherited_environment to testing.TestEnvironment a sufficient solution or is there something following this?

What I had in mind aligns with @rickeylev's option (1) and also what @jbms proposed: Introduce the equivalent of testing.TestEnvironment for non-test executable Starlark rules, which would then allow these rules to implement their own env and env_inherit attributes.

This sounds reasonable.

Are there use cases where a rule would return both TestEnvironment and RunEnvironment, or we should just call it Environment (aliasing and deprecating TestEnvironment) thus covering both use cases?

Is there a use case for a library? Should there be an error if this is used in a lib?

fmeum · 2022-03-17T07:01:50Z

Are there use cases where a rule would return both TestEnvironment and RunEnvironment, or we should just call it Environment (aliasing and deprecating TestEnvironment) thus covering both use cases?

I don't see a reason for a rule to return both (in fact, this duplication could become very confusing and error-prone). It would make sense to just make a renamed testing.TestEnvironment the generic solution for both binary and test targets and simply preserve it as a Starlark method for backwards compatibility.

Do you have a preference for the Starlark name of the new provider? Does it require namespacing or could it just be called e.g. EnvironmentInfo?

Also, would you agree with the new provider's key being visible to Starlark? This would allow wrapper rules around binaries and tests to forward the provider (which would be very useful for transition wrappers like the one in #14673).

Is there a use case for a library? Should there be an error if this is used in a lib?

Transitively collecting general-purpose environment variables doesn't seem like a good use case to me and would probably be prone to conflicts, so I would say this should be an error.

Adds an inherited_environment field to testing.TestEnvironment to allow Starlark rules to implement the equivalent of the native rules' `env_inherit` attribute. Work towards bazelbuild#7364. To fully resolve that issue, it remains to handle executable non-test rules. RELNOTES: Starlark test rules can use the new inherited_environment parameter of testing.TestEnvironment to specify environment variables whose values should be inherited from the shell environment.

Adds an inherited_environment field to testing.TestEnvironment to allow Starlark rules to implement the equivalent of the native rules' `env_inherit` attribute. Work towards #7364. To fully resolve that issue, it remains to handle executable non-test rules. RELNOTES: Starlark test rules can use the new inherited_environment parameter of testing.TestEnvironment to specify environment variables whose values should be inherited from the shell environment. Closes #14849. PiperOrigin-RevId: 439277689

Adds an inherited_environment field to testing.TestEnvironment to allow Starlark rules to implement the equivalent of the native rules' `env_inherit` attribute. Work towards bazelbuild#7364. To fully resolve that issue, it remains to handle executable non-test rules. RELNOTES: Starlark test rules can use the new inherited_environment parameter of testing.TestEnvironment to specify environment variables whose values should be inherited from the shell environment. Closes bazelbuild#14849. PiperOrigin-RevId: 439277689

fmeum · 2022-04-13T08:22:49Z

@comius I just tried to refactor TestEnvironmentInfo into a general RunEnvironmentInfo provider that can be used for both executable and test rules, but ran into an issue: The ActionEnvironment for a target run with bazel run is constructed in RunfilesSupport, which only receives a RuleContext but not a RuleConfiguredTarget and thus doesn't seem to have access to the providers returned by the target. Do you see a way to work around this limitation?

RunCommand has access to the ConfiguredTarget, which should be good enough.

Adds an inherited_environment field to testing.TestEnvironment to allow Starlark rules to implement the equivalent of the native rules' `env_inherit` attribute. Work towards bazelbuild#7364. To fully resolve that issue, it remains to handle executable non-test rules. RELNOTES: Starlark test rules can use the new inherited_environment parameter of testing.TestEnvironment to specify environment variables whose values should be inherited from the shell environment. Closes bazelbuild#14849. PiperOrigin-RevId: 439277689

Adds an inherited_environment field to testing.TestEnvironment to allow Starlark rules to implement the equivalent of the native rules' `env_inherit` attribute. Work towards bazelbuild#7364. To fully resolve that issue, it remains to handle executable non-test rules. RELNOTES: Starlark test rules can use the new inherited_environment parameter of testing.TestEnvironment to specify environment variables whose values should be inherited from the shell environment. Closes bazelbuild#14849. PiperOrigin-RevId: 439277689 Cherry-pick contains parts of: Delete non-interning, non-singleton @AutoCodec. PiperOrigin-RevId: 411683398

Adds an inherited_environment field to testing.TestEnvironment to allow Starlark rules to implement the equivalent of the native rules' `env_inherit` attribute. Work towards bazelbuild#7364. To fully resolve that issue, it remains to handle executable non-test rules. RELNOTES: Starlark test rules can use the new inherited_environment parameter of testing.TestEnvironment to specify environment variables whose values should be inherited from the shell environment. Closes bazelbuild#14849. PiperOrigin-RevId: 439277689 Cherry-pick contains parts of: Delete non-interning, non-singleton @AutoCodec. PiperOrigin-RevId: 411683398 Cherry-pick makes the following additional changes: - Replace use of ImmutableMap.buildKeepingLast with .copyOf

Adds an inherited_environment field to testing.TestEnvironment to allow Starlark rules to implement the equivalent of the native rules' `env_inherit` attribute. Work towards #7364. To fully resolve that issue, it remains to handle executable non-test rules. RELNOTES: Starlark test rules can use the new inherited_environment parameter of testing.TestEnvironment to specify environment variables whose values should be inherited from the shell environment. Closes #14849. PiperOrigin-RevId: 439277689 Cherry-pick contains parts of: Delete non-interning, non-singleton @AutoCodec. PiperOrigin-RevId: 411683398 Cherry-pick makes the following additional changes: - Replace use of ImmutableMap.buildKeepingLast with .copyOf Co-authored-by: Chenchu Kolli <ckolli@google.com>

The new RunEnvironmentInfo provider allows any executable or test Starlark rule to specify the environment for when it is executed, either as part of a test action or via the run command. Refactors testing.TestEnvironment to construct a RunEnvironmentInfo and adds a warning (but not an error) if the provider constructed in this way is returned from a non-executable non-test rule. If a RunEnvironmentInfo is constructed directly via the Starlark constructor, this warning becomes an error. Fixes bazelbuild#7364 Fixes bazelbuild#15224 Fixes bazelbuild#15225 Closes bazelbuild#15232. PiperOrigin-RevId: 448185352

* Specify fixedEnv/inheritedEnv interaction in ActionEnvironment Previously, ActionEnvironment did not publicly document how fixed and inherited environment variables interact, but still cautioned users to keep the two sets disjoint without enforcing this. As a result, neither could users rely on the interaction nor could ActionEnvironment benefit from the additional freedom of not specifying the behavior. With this commit, ActionEnvironment explicitly specifies that the values of environment variable inherited from the client environment take precedence over fixed values and codifies this behavior in a test. This has been the effective behavior all along and has the advantage that users can provide overrideable defaults for environment variables. Closes #15170. PiperOrigin-RevId: 439315634 * Intern trivial ActionEnvironment and EnvironmentVariables instances When an ActionEnvironment is constructed out of an existing one, the ActionEnvironment and EnvironmentVariables instances, which are immutable, can be reused if no variables are added. Also renames addVariables and addFixedVariables to better reflect that they are operating on an immutable type. Closes #15171. PiperOrigin-RevId: 440312159 * Let Starlark executable rules specify their environment The new RunEnvironmentInfo provider allows any executable or test Starlark rule to specify the environment for when it is executed, either as part of a test action or via the run command. Refactors testing.TestEnvironment to construct a RunEnvironmentInfo and adds a warning (but not an error) if the provider constructed in this way is returned from a non-executable non-test rule. If a RunEnvironmentInfo is constructed directly via the Starlark constructor, this warning becomes an error. Fixes #7364 Fixes #15224 Fixes #15225 Closes #15232. PiperOrigin-RevId: 448185352 * Fix strict deps violation ``` src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java:990: error: [strict] Using type com.google.devtools.build.lib.cmdline.LabelValidator from an indirect dependency (TOOL_INFO: "//src/main/java/com/google/devtools/build/lib/cmdline:LabelValidator"). See command below ** LabelValidator.parseAbsoluteLabel(labelString); ^ ```

Globegitter changed the title ~~Add env attribute for *_binary and *_test~~ Add common env attribute for *_binary and *_test Feb 6, 2019

dslomov added team-Starlark untriaged labels Feb 11, 2019

laurentlb added type: feature request P3 We're not considering working on this, but happy to review a PR. (No assignee) and removed untriaged labels Feb 18, 2019

Globegitter mentioned this issue Mar 1, 2019

Add env attribute to go_test rule bazel-contrib/rules_go#1973

Closed

Globegitter mentioned this issue Mar 3, 2019

feat(@bazel/jasmine): add coverage reporting bazel-contrib/rules_nodejs#553

Merged

5 tasks

blico mentioned this issue Mar 5, 2019

Add common envs attribute for *_binary and *_test rules #7624

Closed

hlopko assigned lberki Mar 7, 2019

This was referenced May 9, 2019

--test_env/--action_env only for specific targets #6011

Open

No way to run action with a modified version of default_shell_env #4912

Open

fmeum mentioned this issue Feb 17, 2022

Let Starlark tests inherit env variables #14849

Closed

This was referenced Apr 12, 2022

Let Starlark tests inherit env variables #15217

Merged

testing.TestEnvironment not equal to itself #15225

Closed

fmeum mentioned this issue Apr 13, 2022

Let Starlark executable rules specify their environment #15232

Closed

bazel-io closed this as completed in 1e40541 May 12, 2022

ckolli5 mentioned this issue Jun 27, 2022

Let Starlark executable rules specify their environment #15750

Closed

fmeum mentioned this issue Jun 29, 2022

Let Starlark executable rules specify their environment #15766

Merged

darkrift mentioned this issue Sep 18, 2024

sh_binary to return RunEnvironmentInfo provider #23658

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add common env attribute for _binary and _test #7364

Add common env attribute for _binary and _test #7364

Globegitter commented Feb 6, 2019

Globegitter commented Mar 1, 2019 •

edited

Loading

lberki commented Mar 7, 2019

lberki commented Mar 7, 2019

blico commented Mar 8, 2019 •

edited

Loading

lberki commented Mar 26, 2019

blico commented Apr 8, 2019 •

edited

Loading

lberki commented Jun 19, 2019

wesleyw72 commented Jan 18, 2020

wesleyw72 commented Jan 20, 2020

ulfjack commented Jan 21, 2020

ulfjack commented Jan 21, 2020

Globegitter commented Feb 11, 2020

ulfjack commented Feb 11, 2020

blico commented Feb 14, 2020

TheKaur commented Mar 19, 2020

ulfjack commented Mar 20, 2020

mcwilson07 commented Mar 31, 2020

linzhp commented Apr 15, 2020

fmeum commented Feb 16, 2022 •

edited

Loading

linzhp commented Feb 16, 2022

meteorcloudy commented Feb 17, 2022

fmeum commented Feb 17, 2022

comius commented Mar 16, 2022

rickeylev commented Mar 16, 2022

fmeum commented Mar 16, 2022

comius commented Mar 17, 2022

fmeum commented Mar 17, 2022 •

edited

Loading

fmeum commented Apr 13, 2022 •

edited

Loading

Add common env attribute for *_binary and *_test #7364

Add common env attribute for *_binary and *_test #7364

Comments

Globegitter commented Feb 6, 2019

Description of the problem / feature request:

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

Have you found anything relevant by searching the web?

Globegitter commented Mar 1, 2019 • edited Loading

lberki commented Mar 7, 2019

lberki commented Mar 7, 2019

blico commented Mar 8, 2019 • edited Loading

lberki commented Mar 26, 2019

blico commented Apr 8, 2019 • edited Loading

lberki commented Jun 19, 2019

wesleyw72 commented Jan 18, 2020

wesleyw72 commented Jan 20, 2020

ulfjack commented Jan 21, 2020

ulfjack commented Jan 21, 2020

Globegitter commented Feb 11, 2020

ulfjack commented Feb 11, 2020

blico commented Feb 14, 2020

TheKaur commented Mar 19, 2020

ulfjack commented Mar 20, 2020

mcwilson07 commented Mar 31, 2020

linzhp commented Apr 15, 2020

fmeum commented Feb 16, 2022 • edited Loading

linzhp commented Feb 16, 2022

meteorcloudy commented Feb 17, 2022

fmeum commented Feb 17, 2022

comius commented Mar 16, 2022

rickeylev commented Mar 16, 2022

fmeum commented Mar 16, 2022

comius commented Mar 17, 2022

fmeum commented Mar 17, 2022 • edited Loading

fmeum commented Apr 13, 2022 • edited Loading

Add common env attribute for _binary and _test #7364

Add common env attribute for _binary and _test #7364

Globegitter commented Mar 1, 2019 •

edited

Loading

blico commented Mar 8, 2019 •

edited

Loading

blico commented Apr 8, 2019 •

edited

Loading

fmeum commented Feb 16, 2022 •

edited

Loading

fmeum commented Mar 17, 2022 •

edited

Loading

fmeum commented Apr 13, 2022 •

edited

Loading