fix(hermeticity): do not leak PATH to actions

[alexeagle]

I have a client who vendored this file into their Bazel workspace, and it caused every new machine to have a non-incremental build with all cache misses, due to the CI system (CircleCI in that case) injecting an unstable value into the PATH (/tmp/circleci-launch-agent150557664/circleci-agent/1.0.193225-b3de599b/linux/amd64)

Commit Message: Above
Additional Description:
Risk Level: Unsure
Testing: Not familiar with this project, so whatever tests you normally run? The client says this change is green for them.
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

[repokitteh-read-only]

Hi @alexeagle, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #28517 was opened by alexeagle.

see: more, trace.

[phlax]

lgtm, thanks @alexeagle

cc @keith

[keith]

pretty sure having this passthrough is required for most folks. maybe CI will show that. #24408 would be the long term solution if so

[phlax]

only windows is complaining in CI

/usr/bin/env: 'python3': No such file or directory

https://dev.azure.com/cncf/envoy/_build/results?buildId=143505&view=logs&j=b840a642-5ff3-5357-2e4b-e06e40b0cffd&t=67965174-5100-5631-9dc0-68b9f0aacb53&l=1396

[keith]

yea i don't think there's a standard path we can rely on today because the instructions have folks install clang somewhere random

[keith]

as a short term workaround your folks could override this value and pass whatever hardcoded path would work for you

[phlax]

can we update the docs?

from a general pov it would be better not to set these as defaults - sort of against how you would expect bazel to work i think

but yep we will probably either need to update docs somewhere or handle support tickets

[phlax]

re win error - i think this PR has helpfully weeded out a non-hermetic build issue with luajit

the fail is in our patch here

envoy/bazel/foreign_cc/luajit.patch

Line 138 in 260fe14

+#!/usr/bin/env python3

which i think should be using a py_binary in some way to use the hermetic python bin and not depend on the system python

not sure why only windows fails, i guess its not failing elsewhere due to non-hermeticity - i think the path must be the same on the linux-side so no caches were blown

given that windows only runs a subset of build/tests we might want to run this without cache to weed out any other issues

[alexeagle]

I've suggested the client just comment out this line in their copy of this file. I'd be okay resolving this with a documentation change instead, but then few people read docs, so I'd guess the problem will continue.

For your CI, of course you could use a platform-specific bazelrc setting so this only happens on Windows - but I think @keith is pointing out that a green CI run here may not be enough?

[phlax]

I'd be okay resolving this with a documentation change instead, but then few people read docs, so I'd guess the problem will continue.

i more meant that if we remove this by default we should add to build docs that for host builds you may need to add this (or i guess improve the build somehow to look in expected places)

mho is that this is a welcome change, but im aware that not all agree on that so would prefer to find a consensus

re readding for windows - i think that is fine as a workaround in this case, if that is the only issue but would prefer medium term to make sure nothing is using non-hermetic system tools unless absolutely necessary

I think @keith is pointing out that a green CI run here may not be enough?

yeah, i came to the same conclusion - i think we can test this with --action_env=SOME=cache-busting-variable to force a non-cached run

[phlax]

i was wondering why this went through CI so quickly

i think it gets added again here:

envoy/bazel/setup_clang.sh

Lines 21 to 28 in 4031ea2

	echo "# Generated file, do not edit. If you want to disable clang, just delete this file.
	build:clang --action_env='PATH=${PATH}' --host_action_env='PATH=${PATH}'
	build:clang --action_env=CC=clang --host_action_env=CC=clang
	build:clang --action_env=CXX=clang++ --host_action_env=CXX=clang++
	build:clang --action_env='LLVM_CONFIG=${LLVM_PREFIX}/bin/llvm-config' --host_action_env='LLVM_CONFIG=${LLVM_PREFIX}/bin/llvm-config'
	build:clang --repo_env='LLVM_CONFIG=${LLVM_PREFIX}/bin/llvm-config'
	build:clang --linkopt='-L$(llvm-config --libdir)'
	build:clang --linkopt='-Wl,-rpath,$(llvm-config --libdir)'

i guess this is not an issue for using bazel directly but for using the envoy code paths i think this would need to change also

[keith]

hrm yea i guess ideally we could wrap this without having to generate config files with these contents at various paths. I still think the path of least resistance would be to make the vendored llvm toolchain work and then allow a --config=hosttools or something for folks who didn't want that

[htuch]

@phlax @keith friendly ping on next steps for this PR.

[phlax]

next steps for this PR.

i think testing properly without PATH in the action_env - ie removing from setup_clang.sh would give us a better idea of what breaks

I still think the path of least resistance would be to make the vendored llvm toolchain work

i would agree, but not sure what we need to make it happen - iirc there was gcc issue/s that needed to be ironed out, not sure what else

in terms of Envoy CI we are using the RBE toolchain/s for ~everything - i have a local container sans llvm etc and it mostly works (when using RBE)

[keith]

i would agree, but not sure what we need to make it happen - iirc there was gcc issue/s that needed to be ironed out, not sure what else

Yea I think we'd need a way to disable that and use the default toolchain, but I think we could do that with a custom config

[KBaichoo]

@phlax @keith friendly ping on this PR.

[keith]

I think we could try to merge this as-is now, where PATH only applies to Windows, but given the instructions we provide for building it's possible this breaks folks who install clang to non-standard directories. But we can try and see how it goes

[keith]

/retest

[phlax]

lgtm, thanks @alexeagle

i will look after at potentially removing path from setup clang also - at least to see what effect it has