PATH not propagated to many darwin actions

[keith]

This is a wider reaching version of #8425

In the linked issue I did some investigation around default PATH variables for actions that at the time I thought only affected py_* targets but it turns out that many other actions on darwin do not inherit any PATH and therefore fallback to macOS's default shell env of /usr/gnu/bin:/usr/local/bin:/bin:/usr/bin:. which can allow leakage of /usr/local/* binaries (which is specifically an issue because of homebrew).

While the default PATH may be ok for some use cases, if you actually try to restrict it using --action_env=PATH=something or by executing bazel with env -i PATH=something bazel ..., neither of these are applied to these actions.

For example any action that is executed in the context of Xcode is executed with these environment variables:

  exec env - \
    APPLE_SDK_PLATFORM=MacOSX \
    APPLE_SDK_VERSION_OVERRIDE=10.15 \
    XCODE_VERSION_OVERRIDE=11.5.0.11E608c \
  external/local_config_cc/wrapped_clang ...

Some actions that don't require Xcode are executed with no environment variables:

  exec env - \
  bazel-out/host/bin/external/build_bazel_rules_apple/tools/bundletool/bundletool ...

This is not the behavior of some other rules like genrule where if you build this target with --action_env=/usr/bin:/bin it hits the "Valid path":

genrule(
    name = "foo",
    outs = ["foo.txt"],
    cmd = """
if [[ "$$PATH" != "/usr/bin:/bin" ]]; then
  echo "error: path not propagated: $$PATH"
  exit 1
else
  echo "Valid path! $$PATH"
  exit 1
fi
""",
)

I think that either all actions should get a very conservative default PATH like /usr/bin:/bin, or the flags like --action_env should be able to add env vars to these actions (I believe the latter is likely the more flexible path, but I'm not sure about the implementation implications).

For native actions it seems that this can be resolved by adding:

.setInheritedEnvironment(ImmutableSet.of("PATH"));

to this logic

bazel/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcRuleClasses.java

Lines 140 to 144 in 8f67892

	static SpawnAction.Builder spawnAppleEnvActionBuilder(
	XcodeConfigInfo xcodeConfigInfo, ApplePlatform targetPlatform) {
	return spawnOnDarwinActionBuilder(xcodeConfigInfo)
	.setEnvironment(appleToolchainEnvironment(xcodeConfigInfo, targetPlatform));
	}

But for starlark actions they inherit this environment:

bazel/tools/osx/crosstool/cc_toolchain_config.bzl

Lines 4546 to 4557 in 8f67892

	env_entry(
	key = "XCODE_VERSION_OVERRIDE",
	value = "%{xcode_version_override_value}",
	),
	env_entry(
	key = "APPLE_SDK_VERSION_OVERRIDE",
	value = "%{apple_sdk_version_override_value}",
	),
	env_entry(
	key = "APPLE_SDK_PLATFORM",
	value = "%{apple_sdk_platform_value}",
	),

When accessing the environment variables for an action with the cc_common api. There doesn't seem to be a way to craft an env_entry that doesn't impose a value. So I could add /usr/bin:/bin there, but it wouldn't allow overrides via --action_env=PATH=foo

I'm happy to make a change here but I'm hoping for some guidance on the right path!

What operating system are you running Bazel on?

macOS

Bazel version?

8f67892

[susinmotion]

@gregestren is this something you'd be able to advise on?

[susinmotion]

Actually, this may be an Apple Rules issue, not an --action_env issue. @googlewalt , is this in your wheelhouse?

[googlewalt]

I think there are two issues:

1. AFAICT, based on code inspection, action_env does not work on bazel. @keith do you know of instances where it works?
2. bazel does not set a default PATH for actions.

In both cases, the issue seems to be that BazelRulesModule does not have some setup code we have internally, that does both (1) and (2).

[googlewalt]

Sorry. I found the setup code in BazelRulesModule.

[googlewalt]

Have you tried --incompatible_strict_action_env?

[keith]

I have tried that flag, AFAICT that flag is only read here:

bazel/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java

Lines 187 to 201 in 84834db

	if (strictActionEnv) {
	env.put("PATH", pathOrDefault(os, null, shellExecutable));
	} else if (os == OS.WINDOWS) {
	// TODO(ulfjack): We want to add the MSYS root to the PATH, but that prevents us from
	// inheriting PATH from the client environment. For now we use System.getenv even though
	// that is incorrect. We should enable strict_action_env by default and then remove this
	// code, but that change may break Windows users who are relying on the MSYS root being in
	// the PATH.
	env.put("PATH", pathOrDefault(os, System.getenv("PATH"), shellExecutable));
	} else {
	// The previous implementation used System.getenv (which uses the server's environment),
	// and fell back to a hard-coded "/bin:/usr/bin" if PATH was not set.
	env.put("PATH", null);
	}

and IIRC this codepath isn't even hit, otherwise action_env would work as well:

bazel/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java

Lines 202 to 207 in 84834db

	// Shell environment variables specified via options take precedence over the
	// ones inherited from the fragments. In the long run, these fragments will
	// be replaced by appropriate default rc files anyway.
	for (Map.Entry<String, String> entry : options.get(CoreOptions.class).actionEnvironment) {
	env.put(entry.getKey(), entry.getValue());
	}

[googlewalt]

[edit: this is wrong, see below]

I verified that this behavior is not specific to apple rules, and it has been this way since at least bazel 1.0. I would have expected the SHELL_ACTION_ENV to be applied to all the rules that run a shell, but I'm not familiar with exactly how that logic works. The most recent non-trivial refactoring of this code was done in commit 8626623. Perhaps someone more familiar with this code can comment?

[keith]

cc @lberki (author of that commit)

[lberki]

I'm certainly not that "someone familiar with this code" :( The commit 8626623 was meant to be a refactoring without user-visible effects; do you have a reason to believe that this is what caused the behavior change?

Looking at https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java#L205 , it looks like the variables set in --action_env should be applied no matter what, which contrary to the statement in the description of this bug. Are you... sure? Or maybe this is something that's specific to Apple rules (I'm even less familiar with those than I am with the computing of the action environment...)

[googlewalt]

Apologies for the red herring. My statements from previous post was wrong (user error).

--action_env and --incompatible_strict_action_env seem to generally work for bazel. I tested it for cc_library, and it also works for me for Apple wrapped clang invocation for Objc compile and linking:

bazel build --action_env PATH=/my_special_path:$PATH -s examples/macos/HelloWorld:HelloWorld
...
  exec env - \
    APPLE_SDK_PLATFORM=MacOSX \
    APPLE_SDK_VERSION_OVERRIDE=10.15 \
    PATH=/my_special_path//usr/local/git/current/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin:/Library/Apple/usr/bin \
    XCODE_VERSION_OVERRIDE=11.6.0.11E708 \
  external/local_config_cc/wrapped_clang ...

But some other Apple actions don't appear to be have default PATHs, or be hooked up to those flags: StoreboardCompile, AssetCatalogCompile, CompileRootInfoPlist, Processing and signing.

[lberki]

No worries! Unfortunately, I won't be able to help further then :(

[benjaminp]

No actions in rules_apple seems to employ use_default_shell_env, which means --action_env will be ignored for them.

[keith]

Are you suggesting all actions should set that? Is that the right switch for this? It surprises me a bit that that would be the way to make this work. Because that means that all non-native rules would need to set that to True (and when would you want to set that to False given this issue?)

Somewhat related I guess:

https://github.com/bazelbuild/rules_go/blob/b4c392fcfc0a44f2cc5ee3dbb884121dd2ab6957/proto/compiler.bzl#L126-L130

[benjaminp]

Are you suggesting all actions should set that? Is that the right switch for this?

The documentation recommends all actions set it.

It surprises me a bit that that would be the way to make this work. Because that means that all non-native rules would need to set that to True

Native rules must also opt in, but some effort was historically put into fixing them (#3320).

(and when would you want to set that to False given this issue?)

The env parameter to actions.run(_shell) is ignored under use_default_shell_env=True.

[keith]

It looks like this is mutually exclusive with setting env on these, which doesn't work for Apple actions since they use env to propagate Xcode version info. I guess we might not be able to have it both ways?

[keith]

Example of the failures caused by this in the Apple toolchain bazelbuild/rules_swift#497

(18:30:51) ERROR: /Users/buildkite/builds/bk-imacpro-16/bazel/rules-swift-swift/examples/xplatform/c_from_swift/BUILD:25:14: Compiling Swift module Counter failed: Worker process did not return a WorkResponse:
--
  |  
  | ---8<---8<--- Start of log, file at /private/var/tmp/_bazel_buildkite/022c118cf0acc0d6f57f13796cd5f4e4/bazel-workers/worker-3-SwiftCompile.log ---8<---8<---
  | Error: DEVELOPER_DIR not set.

[susinmotion]

Yeah, it sounds like it might make sense to go back to your first suggestion, to set a conservative default PATH for Apple-related actions. We do need the environment that's set for Apple rules, but it's ok to add to it.

[keith]

Using aquery (assuming that's a fair way to determine this) here's a quick list of mnemonics in our iOS build that don't have a PATH set even when I pass --action_env=PATH=something

Action
AssetCatalogCompile
BundleApp
BundleResources
CcStrip
CompileRootInfoPlist
CppCompile
CppLink
CppModuleMap
ExecutableSymlink
ExtractFromProvisioningProfile
FileWrite
GenProtoDescriptorSet
Genrule
ImportedDynamicFrameworkProcessor
Middleman
ObjcCompile
ObjcLink
ProcessAndSign
ProcessEntitlementsFiles
PythonZipper
SolibSymlink
SourceSymlinkManifest
StoryboardCompile
StoryboardLink
SwiftArchive
SwiftStdlibCopy
Symlink
SymlinkTree
TemplateExpand
XibCompile

I'm surprised by some here looking briefly at the code:

bazel/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java

Lines 726 to 740 in ecab72e

	Action[] stripAction =
	new SpawnAction.Builder()
	.addInput(input)
	.addTransitiveInputs(toolchain.getStripFiles())
	.addOutput(output)
	.useDefaultShellEnvironment()
	.setExecutable(
	PathFragment.create(
	featureConfiguration.getToolPathForAction(CppActionNames.STRIP)))
	.setExecutionInfo(executionInfoBuilder.build())
	.setProgressMessage("Stripping %s for %s", output.prettyPrint(), ruleContext.getLabel())
	.setMnemonic("CcStrip")
	.addCommandLine(CustomCommandLine.builder().addAll(commandLine).build())
	.build(ruleContext);
	ruleContext.registerAction(stripAction);

[keith]

So there was some confusion with the list above, the problem is that some actions don't print their env with aquery. I'll try to submit a follow up for some of those. This fixes some of my use case bazelbuild/rules_swift#502

but it's not perfect. Firstly piping ctx.configuration.default_shell_env through everywhere can be cumbersome. But more importantly that doesn't include everything we actually want. This only includes the "fixed" environment variables, like those passed with --action_env=FOO=BAR, not the "variable" environment variables which are attempted to be passed through with --action_env=FOO

bazel/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java

Lines 486 to 500 in 080547f

	/**
	* Return the "fixed" part of the actions' environment variables.
	*
	* <p>An action's full set of environment variables consist of a "fixed" part and of a "variable"
	* part. The "fixed" variables are independent of the Bazel client's own environment, and are
	* returned by this function. The "variable" ones are inherited from the Bazel client's own
	* environment, and are returned by {@link #getVariableShellEnvironment}.
	*
	* <p>Since values of the "fixed" variables are already known at analysis phase, it is returned
	* here as a map.
	*/
	@Override
	public ImmutableMap<String, String> getLocalShellEnvironment() {
	return actionEnv.getFixedEnv().toMap();
	}

There doesn't seem to be a way to get any of the other variables from starlark, but I might be missing something?

[keith]

The CppLinkAction is one of the ones where its environment doesn't show up in aquery because it doesn't implement SpawnAction

[cpsauer]

Ran into a variant of this, switching over to an Apple Silicon machine.

It's worth noting that on Apple Silicon, Homebrew no longer installs into the default PATH but rather to /opt/homebrew/bin.

That said, if we do get into this and change defaults, I think it might well make sense to include Homebrew installs in the strict_action_env default. I get the hermeticity concerns, but it's also probably a good idea to default to the tools the user is more likely to expect, rather than, e.g., use the 2007 era bash, old python, and other os-bundled binaries that I think most users replace with those installed from brew. Always possible to make it more restrictive with --action_env, ofc.

[cpsauer]

@keith, what are your thoughts these days on the desirability of having homebrew tools on the action path?
(Sounded negative above, but also now needed to conveniently get e.g. python3 and a version of bash that's newer than the system's ~2007 vintage.)

[keith]

At least with python3 macOS has been keeping relatively up to date so I'm happy to be using that version since it's the same for everyone (assuming you only maintain 1 Xcode version). For the bash thing we just deal with the old one. I think the downside we saw before we started excluding the brew path was just people's brew states could be all over the place depending on when things were last updated, so it added a lot of variance.

[cpsauer]

Got it. Thanks, @keith.
(We have a brew-update thing+use a bunch of add'l tools in scripts that are more easily installed that way. I think that flips the personal calculus but good to know & I therefore won't try to upstream or anything.)

[timsutton]

I also believe that since brew has thousands of formulae in their core repo alone, and most developers are probably using at least some binaries provided by brew (but can be widely-varying and selected according to personal habits), that including it by default opens the door to a ton of subtle variances. I think a lot of developers who are just "using" Bazel as their build system because it's what their projects use, can then easily run into issues that may be hard to debug.

The GNU bash on macOS is old, yes, but since macOS has also shipped with an immutable zsh since 2015 there's now also opportunity for Darwin-specific wrapper scripts to use /bin/zsh if there's a desire for more modern shell scripting language features.

[pauldraper]

So what's the conclusion here?

Making copious PRs to rule sets to set use_default_shell_env = True? Any reason that isn't a good idea?

[keith]

#18235 would be the best of both worlds I think. In my case right now we can't get it to work 100% because values like --action_env=FOO (where the value should be passed through) isn't something we can fetch in starlark.

[keith]

If possible definitely set use_default_shell_env = True and move on though

[iancha1992]

A fix for this issue has been included in Bazel 6.4.0 RC1. Please test out the release candidate and report any issues as soon as possible. Thanks!

[keith]

for others note that the change is only if you enable --incompatible_merge_fixed_and_default_shell_env and make some patches to rules_apple / rules_swift / apple_support, with bazel 7.x+ this will be the default