-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Update init.sh #2452
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update init.sh #2452
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Assign the PR to them by writing The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
@@ -21,7 +21,7 @@ if [ ${ROOT} != "${GOPATH-$HOME/go}/src/istio.io/istio" ]; then | |||
exit 1 | |||
fi | |||
|
|||
which dep >/dev/null || go get -u github.com/golang/dep/cmd/dep | |||
which ${GOPATH}/bin/dep >/dev/null || go get -u github.com/golang/dep/cmd/dep |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean $GOROOT? $GOPATH has many paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Kuat, GOPATH/bin/dep is being used everywhere, I guess we should change all of them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a problem with just which dep
and installing it if necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following is simpler and should be sufficient:
[ -f ${GOPATH}/bin/dep ] || go get -u github.com/golang/dep/cmd/dep
GOPATH won' t have many paths given the "export GOPATH=$TOP" that happens a few lines above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoa, why do we override GOPATH? that seems counter to go practice. it's a standard practice to use system vendored libraries to make local changes instead of using dep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The top-level Makefile does it too. It seems like a reasonable thing to do from a perspective of build hermetics and reproducibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can isolate the host OS with a container or a VM and use standard go practices.
Doing peculiar things with go tool seems to fall into the bazel trap of being overly opinionated about the build environments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see GOPATH set in Makefile
# Make sure GOPATH is set based on the executing Makefile and workspace.
# Will override GOPATH from the env.
export GOPATH= $(shell cd ../../..; pwd)
check for dep in GOPATH