Let Starlark tests inherit env variables #14849

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

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.

fmeum · 2022-02-17T11:25:48Z

@meteorcloudy The tests currently fail due to an existing inconsistency: ActionEnvironment gives precedence to inherited variables, while env_inherit on native rules gives precedence to fixed variables.

I actually think that the behavior of ActionEnvironment is more useful: Rules could still guard against it by filtering the inherited env list, but can use it to provide sensible defaults. What do you think?

meteorcloudy · 2022-02-17T11:31:45Z

@comius might be the right person to answer your question ;)

fmeum · 2022-02-17T11:41:52Z

I pushed a new commit that should make the tests pass. I'm now documenting the exact interaction in Starlark (using the ActionEnvironment behavior). It is still possible to implement the env_inherit behavior on top in case that is desired for starlarkified native rules, so we don't necessarily have to resolve this conflict at this point.

fmeum · 2022-02-17T13:23:42Z

@comius I will push temporary commits on top to debug the failure on Windows, but will not touch the first commit.

fmeum · 2022-02-18T20:19:45Z

The Windows failure has been fixed.

meteorcloudy

Thanks, it generally looks good to me, but @comius should do the final review.

fmeum · 2022-03-15T17:47:52Z

@comius Friendly ping. Can you already say roughly when you would be able to review this?

comius · 2022-03-16T13:52:09Z

Work towards #7364

What else is coming? Having a plan first might help making things more consistent.

src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test/TestEnvironmentInfoApi.java

comius · 2022-03-16T14:28:22Z

src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test/TestingModuleApi.java

+                  "A sequence of string keys that represent environment variables. These variables"
+                      + " will be made available during the test execution with their current value"
+                      + " taken from the environment. If a variable is contained in both"
+                      + " <code>environment</code> and <code>inherited_environment</code>, the"
+                      + " value inherited from the environment will take precedence if set.")


Same here, can this be more precise?
Perhaps:
"A sequence of names of environment variables. These variables are made available during the test execution with their current value taken from the shell environment".

"shell environment" makes it consistent with documentation of "use_default_shell_env" attribute on ctx.actions.run.

If a variable is contained in both environment and inherited_environment, the value inherited from the environment will take precedence if set.

This is the opposite of implementation in ActionEnvironment. I would also think it makes more sense that env takes precedence.

Thanks, I modified the comment as you suggested.

This is the opposite of implementation in ActionEnvironment. I would also think it makes more sense that env takes precedence.

Sorry, I think the missing shell qualifier in front of environment makes this comment confusing. Updated it to say "the value inherited from the shell environment will take precedence if set", which should align with the behavior of ActionEnvironment#resolve.
Is this the behavior you find most reasonable? I do, simply because it allows rules to choose the behavior they want: They can provide an overridable default by specifying a variable name in both environment and inherited_environment, but they can also ensure that the fixed value in environment is always used simply by omitting it from inherited_environment. If environment always were to take precedence, the former behavior wouldn't be possible to achieve.

Note though that the current behavior of env and env_inherit on native rules is in fact different. Here, env always takes precendence. But since the new provider field would still allow to implement the current behavior, that shouldn't cause problems for starlarkification.

I just noticed that ActionEnvironment says "The order in which the environments are combined is undefined, so callers need to take care that the key set of the {@code fixedEnv} map and the set of {@code inheritedEnv} elements are disjoint.". Is there a reason not to specify one or the other behavior?

Is there a reason not to specify one or the other behavior?

I think the following options are valid:

define the behavior

throw an error when sets are not disjoint

Saying it's undefined and asking the user to keep it disjoint but not enforcing it is the worse choice. So feel free to update it.

@comius Friendly ping. If you could clarify your preference regarding the fixedEnv/inheritedEnv precedence, I could finalize the PR ahead of your final review.

I agree current order gives you a bit more choice (i.e. having defaults possibly overridden from env) than the opposite (inherit but always override, where duplication doesn't make sense).

src/main/java/com/google/devtools/build/lib/actions/ActionEnvironment.java

fmeum · 2022-03-16T20:48:14Z

In the emails I received from GitHub, I also saw this comment

This should be ImmutableList (and ImmutableMap above). We don't want data inside a provider to mutate.

I can't find it now, but made the suggested change. Let me know if you didn't want me to do that.

fmeum · 2022-03-29T16:46:13Z

@comius Friendly ping. If you could clarify your preference regarding the fixedEnv/inheritedEnv precedence, I could finalize the PR ahead of your final review.

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 <[email protected]>

Baseline: 8d66a41 Cherry picks: + becd149: Remote: Cache merkle trees + d7628e1: Update DEFAULT_IOS_CPU for M1 arm64 simulator support + 80c56ff: Compile Apple tools as fat binaries if possible + 3c09f34: Add protobuf as a well known module + 3a5b360: Remote: Merge target-level exec_properties with --remote_default_exec_properties + 917e15e: Add -no_uuid for hermetic macOS toolchain setup + f5cf8b0: Remote: Fixes an issue when --experimental_remote_cache_async encounter flaky tests. + 77a002c: Remove DigestUtils.getDigestInExclusiveMode() now that SsdModule has … + 557a7e7: Fixes for the Starlark transition hash computation (#14251) + 34c7146: Do location expansion in copts of objc_library + 50274a9: [5.x] Remote: Add support for compression on gRPC cache (#14277) + 61bf2e5: Automated rollback of commit 34c7146. + 79888fe: Silence a zstd-jni GCC warning. + 063b5c9: Remote: Limit max number of gRPC connections by --remote_max_connections. + fd727ec: Do location expansion in copts of objc_library + 23d0969: Fix _is_shared_library_extension_valid + 5cf1d6e: Remove merging of java_outputs in JavaPluginInfo. + cea5f4f: Cherrypick Bzlmod documentation (#14301) + 227e49e: Format work requests according to ndjson spec + ae0a6c9: Enable user_link_flags_feature for macosx cc_toolchain_config + 8c2c78c: Remote: Use Action's salt field to differentiate cache across workspaces. + f948989: [5.x] Remote: Fix "file not found" error when remote cache is changed from enabled to disabled. (#14321) + 3069ac4: Delete marker file before fetching an external repository + c05c626: Remote: Fix file counting in merkletree.DirectoryTreeBuilder + d84f799: Fix remote spawn tests for remote_merkle_tree_cache=true + 59e16e9: Show skipped tests as a warning + 76b3c24: Build xcode-locator as a universal binary + aa52f2d: Exit collect_coverage.sh early if LCOV_MERGER is not set. + 4256d46: Automated rollback of commit d84f799. + dce2435: [apple] fix issues compiling C in objc_library for watchos/armv7k + bfc2413: 5.x: Remote: Ignore blobs referenced in BEP if the generating action cannot be cached remotely. (#14389) + 5aef53a: Remote: Don't blocking-get when acquiring gRPC connections. (#14420) + 005361c: Disable IncludeValidation for ObjC in bazel + d703b7b: Update java_tools v11.6 + 90965b0: Stop remote blob upload if upload is complete. (#14467) + dc59d9e: [5.x] Make remote BES uploader better (#14472) + 2edab73: Avo

fmeum requested a review from lberki as a code owner February 17, 2022 11:20

fmeum mentioned this pull request Feb 17, 2022

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

Closed

meteorcloudy requested a review from c-mita February 17, 2022 11:22

meisterT requested a review from comius February 17, 2022 11:26

fmeum force-pushed the starlark-env-inherit branch from bb08f5b to 1be38df Compare February 17, 2022 11:40

fmeum force-pushed the starlark-env-inherit branch from 1be38df to 2e197fd Compare February 17, 2022 12:07

comius removed request for c-mita and lberki February 17, 2022 12:27

comius self-assigned this Feb 17, 2022

fmeum force-pushed the starlark-env-inherit branch 4 times, most recently from 66217d4 to a72597a Compare February 17, 2022 16:35

meteorcloudy reviewed Feb 22, 2022

View reviewed changes

gregestren added the team-Build-Language label Mar 7, 2022

comius reviewed Mar 16, 2022

View reviewed changes

src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test/TestEnvironmentInfoApi.java Outdated Show resolved Hide resolved

comius requested changes Mar 16, 2022

View reviewed changes

comius reviewed Mar 16, 2022

View reviewed changes

src/main/java/com/google/devtools/build/lib/actions/ActionEnvironment.java Outdated Show resolved Hide resolved

fmeum force-pushed the starlark-env-inherit branch from a72597a to 3b0d95e Compare March 16, 2022 20:41

fmeum force-pushed the starlark-env-inherit branch 2 times, most recently from dbb08ef to 662eb8e Compare March 16, 2022 22:06

bazel-io mentioned this pull request Apr 13, 2022

[5.2.0] Let Starlark tests inherit env variables #15240

Closed

bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Apr 13, 2022

fmeum mentioned this pull request Apr 19, 2022

Bazel@HEAD lacks env_inherit on cc_test #15268

Closed

Let Starlark tests inherit env variables #14849

Let Starlark tests inherit env variables #14849

Uh oh!

Conversation

fmeum commented Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmeum commented Feb 17, 2022

Uh oh!

meteorcloudy commented Feb 17, 2022

Uh oh!

fmeum commented Feb 17, 2022

Uh oh!

fmeum commented Feb 17, 2022

Uh oh!

fmeum commented Feb 18, 2022

Uh oh!

meteorcloudy left a comment

Choose a reason for hiding this comment

Uh oh!

fmeum commented Mar 15, 2022

Uh oh!

comius commented Mar 16, 2022

Uh oh!

Uh oh!

comius Mar 16, 2022

Choose a reason for hiding this comment

Uh oh!

comius Mar 16, 2022

Choose a reason for hiding this comment

Uh oh!

fmeum Mar 16, 2022

Choose a reason for hiding this comment

Uh oh!

fmeum Mar 16, 2022

Choose a reason for hiding this comment

Uh oh!

comius Apr 1, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fmeum commented Mar 16, 2022

Uh oh!

fmeum commented Mar 29, 2022

Uh oh!

fmeum commented Feb 17, 2022 •

edited

Loading