This is not obvious. The environment variable GOBIN is explicitly set in the Dockerfile environment. We do not desire to override it in the Makefile.

Also this needs to apply to all repos

I knew the default value for GOBIN is "/gobin". However, this will cause the following problem:

--- FAIL: TestEnvoy (0.56s)
    xds_test.go:116: Failed to setup test: fork/exec /gobin/envoy: no such file or directory

When using local build chain, you can see we explicitly set the "GOBIN" to "GOPATH/bin"

$(info Building with your local toolchain.)
export GOBIN ?= $(GOPATH)/bin
include Makefile.core.mk

endif

The reason is we don't mount "/gobin" as volume in docker container. We run command "make pilot-test" as a fresh container which has no "/gobin/evnoy" in it. Even if we've just generated it with command "make build".

I passed in the GOBIN as argument for generating Envoy binary within the mounted "/work/src/istio.io/istio" directory. So if we changed the default value of GOBIN to "/go/bin", it's also Okay.

happy to permit go test overrides.

FWIW #172 allows arbitrary env

@howardjohn This is "go test" options, not env. Have you planed to cover it in #172 ? If it is, I'll close this PR.

Its an environment variable that we read and pass to go test, so it should be covered in #172

@howardjohn Thank you. I'll close this PR.

using src/istio.io/istio doesn't feel like the correct solution. The correct solution would take into account the environment in the test cases and expect binaries or test files in the correct location.

Well, it is because some legacy unit tests within the Pilot tried to resolve the source root path by matching "/src/istio.io/istio". If we don't intend to do modification here, we have to change the unit tests within Pilot.

This may be a little hard for we don't have any specified pattern to match. For example, if we rest in "[arbitrary path]/pilot/src/core/[here]", how can we tell the root source path since we have replaced "istio" directory name to "work"? If we choice to math "work", what if someone else mapped the "istio" to "mywork" like "mywork/pilot/src/..."?

my knee-jerk reaction to this line of change as well as the other src/istio.io/istio changes is they are not the correct approach. I do not disagree that current testing is busted. I think the correct solution to solving the testing problems within a build container involve reading the target environment variable (or as it may be transposed in the istio/istio/Makefile.core.mk file... to a different variable) - to determine the correct testing behavior rather than adding hardcodes. I will give it some more thought. I am avialble on slack for further conversation.

I fully understand. But the reality is even you choose to build with your local tool chain, you can't pass unit tests if your source code don't resident in "src/istio.io/istio"(Last time I made a soft link to "src/istio.io/istio" with "istio" and the tests failed). So we all agreed that it is the test code's responsibility to figure out the right path no matter where they have been mapped, but we need time to do refactoring. The problem is, if we don't make changes, we can't event make further testing and deployment.

@seflerZ we are headed towards making BUILD_WITH_CONTAINER=1 the default within istio/istio. Anything that detracts from that objective IMO is not ideal. The fact that the test cases need refactoring don't have much if any bearing - other than the work needs to be done, even if it needs to be forced.

I don't think I like this as it may cause changes in the local bin directory. And which envoy would be copied into bin, the local OS (MacOS in my case) or the linux version since the container is using linux? I believe there is another issue which I put a change in the PR comments #19937 for where the linux envoy is polluting the darwin directory structure when building with the container, and this could pollute higher.

@ericvn in all cases where BUILD_WITH_CONTAINER=1, GOBIN should contain Linux binaries.