Bazel@HEAD lacks env_inherit on cc_test

[fmeum]

Description of the bug:

With the Starlark flip of cc_test in e04735d, the rule no longer provides the env_inherit attribute.

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

See https://pipelines.actions.githubusercontent.com/serviceHosts/e1b19da9-c261-489f-82be-008378a078a0/_apis/pipelines/1/runs/1251/signedlogcontent/14?urlExpires=2022-04-15T08%3A25%3A30.3485429Z&urlSigningMethod=HMACV1&urlSignature=hEga7PwNt42ZQYwL232J1U%2BEmPtUJJu%2B30dfi0c0M6g%3D, but really any use of env_inherit on cc_test will fail at analysis time.

Which operating system are you running Bazel on?

Linux

What is the output of bazel info release?

development version

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

Using Bazelisk to fetch 22c91da.

What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?

22c91dab43558503a1d6759ccfd77587d87b28cd

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

ee9727d can potentially be used to implement env_inherit in Starlark.

[fmeum]

@trybka since you authored e04735d.

[trybka]

The log you provided is expired, FWIW, but you're right--cc_test (and Starlark tests in general) lack env_inherit -- I'm sorry this slipped past me (none of the Bazel or downstream tests caught it!).

Since this is not an easy fix, I'll roll it back for now until we get this straightened out.

[trybka]

Oh, for some reason I missed that the commit you linked is in -- nevertheless, I will rollback while I work on that to minimize the breakage window.

[trybka]

I don't think I can close this bug or anything, but the rollback is submitted:
098e397

[fmeum]

With #15232, the analogue of testing.TestEnvironment for the run environment will become available. This would allow implementing env and env_inherit for both cc_binary and cc_test in the same way. Perhaps it makes sense to wait for it land.

[fmeum]

@trybka FYI: There is a subtle difference in behavior between env_inherit on native rules and the inherited_environment parameter on testing.TestEnvironment, see #14849 (comment). If you want to maintain complete compatibility with native rules, you would have to drop those variables from inherited_environment that are also keys in environment. But apart from this very minor backwards compatibility concern, having inherited variables take precedence over fixed variables would seem more useful going forward (see also #14849 (comment)).

[trybka]

@fmeum concretely, you're saying:

a) for proper native compatibility, we should filter keys in environment out of inherited_environment
b) except that might be genuinely more useful and might be preferred?

I believe I implemented (b) in 4ed40ee, but if there's a concern, I could go add filtering.

Whatever we land on, I should update to add a test case.

[fmeum]

If b) was meant to say something like "except that might be genuinely less useful and the alternative might be preferred", then yes. I prefer the solution you implemented in 4ed40ee and would argue that if no one speaks up about any experienced breakage (unlikely), then this is the better of the two alternatives for the future.

+1 on adding a test case codifying this decision.

[fmeum]

@trybka Looks like this could be closed?

[trybka]

Yes, though I don't appear to have access to. @oquenchil can you take care of it?