Remove subprocess-environment allow list.

[jsirois]

Now [subprocess-environment] env_vars can be used to selectively
allow arbitrary environment variables to be propagated to subprocesses.

[ci skip-rust]
[ci skip-build-wheels]

[jsirois]

This was motivated by a need to propagate ARCHFLAGS to work around giampaolo/psutil#1832 in #11733.

[Eric-Arellano]

Hm, I think this should probably have a blocklist, particularly with PATH in it? Otherwise users can break things. Ditto on PYTHONPATH probably

[jsirois]

@Eric-Arellano I don't follow. We already need PATH to even support binary discovery at all. PYTHONPATH is sealed off by Pex unless you configure it in with Pex knobs - some (all?) exposed by Pants. You or Stu should take this up if I'm missing something because I really haven't followed the handwringing around env vars well.

[stuhood]

I missed the discussion when the "allowlist" was initially added... but given that there is no way to wildcard env vars from the environment (and probably never will be...?), anything in this list has been explicitly added by a user, and I don't think it makes sense to limit things that have been explicitly added.

Aside:

pants/src/python/pants/engine/environment.py

Lines 41 to 42 in 7a6a8df

	If `allowed` is specified, the requested variable names must be in that list, or an error
	will be raised.

is probably dead code now.

[Eric-Arellano]

We already need PATH to even support binary discovery at all.

Yes, but Pants sets PATH and we do not want to allow the user to configure that one. The behavior would depend on our merge strategy: if the user overrides Pants, then things will almost certainly break. If Pants overrides the user, the user setting will do nothing and people will be confused. An eager blocklist and good error message is better UX. Does that make sense? If so, I can put up a patch.

You're right on PYTHONPATH - I think PATH is the only one we need to block.

[stuhood]

Yes, but Pants sets PATH and we do not want to allow the user to configure that one.

Mm. Yea, that probably makes sense... although dogfooding remote caching in the next few days will likely further shape this.

If all we're trying to do is prevent someone from adding it to the list, I think that

pants/src/python/pants/engine/environment.py

Lines 41 to 42 in 7a6a8df

	If `allowed` is specified, the requested variable names must be in that list, or an error
	will be raised.

can be made much simpler... no need to wait to see if it is set to fail.

[jsirois]

Where does Pants set PATH? But otherwise agreed and there is actually no question. The general principle being you only need to block configuration of things you yourself set to avoid trampling - this has nothing really to do with env cars in particular.

[Eric-Arellano]

Where does Pants set PATH?

FindBinary, for example:

pants/src/python/pants/engine/process.py

Lines 551 to 558 in 43007bd

	Process(
	description=f"Searching for `{request.binary_name}` on PATH={search_path}",
	level=LogLevel.DEBUG,
	input_digest=script_digest,
	argv=[script_path, request.binary_name],
	env={"PATH": search_path},
	cache_scope=ProcessCacheScope.PER_RESTART,
	),

[jsirois]

Aha. If that's the only example, then it's not a great one. We can be doing that (bootstrapping finding bash) differently.

[jsirois]

In fact, if that's the only example, it's a bad one. We should use a user specified PATH if present to find bash.

[stuhood]

In fact, if that's the only example, it's a bad one. We should use a user specified PATH if present to find bash.

Probably. But this gets to the fact that "subprocess environment PATH" is probably too coarse a setting. I'm skeptical that someone will want to set the same PATH for all tools, although they might want to set it for FindBinary. FindBinary scans a PATH for tools so that we don't need the entire PATH set later, in general.

Allowing a different PATH to be set for FindBinary might be a usecase for #7735, but it might also be the case that FindBinary needs to become more of a native operation, a la #10769 and #10526. We'll be dogfooding remote caching soon (cc @tdyas), but getting back to dogfooding remote execution is what will really shake these issues out.