Skip to content

Conversation

rkpagadala
Copy link
Contributor

check for dep in GOPATH

@istio-merge-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
We suggest the following additional approver: sebastienvas

Assign the PR to them by writing /assign @sebastienvas in a comment when ready.

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 /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@rkpagadala rkpagadala requested a review from mattdelco January 5, 2018 22:54
@@ -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
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

@rkpagadala rkpagadala Jan 6, 2018

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)

@rkpagadala rkpagadala closed this Jan 8, 2018
@rkpagadala rkpagadala deleted the rkpagadala-patch-1 branch January 8, 2018 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants