Skip to content
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

Removed GOPATH check #1026

Merged
merged 2 commits into from
Oct 25, 2017
Merged

Removed GOPATH check #1026

merged 2 commits into from
Oct 25, 2017

Conversation

vhosakot
Copy link
Member

Not all Makefile targets need GOPATH to be set. Makefile targets like demo and demo-v2plugin do not need GOPATH to be set in order to sync the directory src/github.com/contiv/netplugin on the laptop with /opt/gopath inside the vagrant VM (https://github.com/contiv/netplugin/blob/master/Vagrantfile#L370). Targets like tar and compile-with-docker also do not need GOPATH.

Hence, this PR removes the check for GOPATH in the Makefile and adds documentation in README.md for the user to set GOPATH when using Makefile targets like k8s-test, system-test, clean and compile that do need GOPATH to be set (https://github.com/contiv/netplugin/blob/master/Makefile#L99).

Tested on Mac laptop.

Signed-off-by: Vikram Hosakote vhosakot@cisco.com

Signed-off-by: Vikram Hosakote <vhosakot@cisco.com>
@vhosakot
Copy link
Member Author

build PR

@delapsley
Copy link

delapsley commented Oct 20, 2017

Thanks for doing this @vhosakot. I did things slightly differently and it seemed to work:

  • cd ~/repos
  • git clone git@github.com:contiv/netgplugin
  • cd netplugin
  • export GOPATH=pwd
  • make demo

That seemed to work.

Actually, this does not work. Disregard :)

README.md Outdated
@@ -30,7 +30,9 @@ that lives behind an OVS bridge and has its own unique interfaces.
#### Step 1: Clone the project and bringup the VMs

Note: if you have $GOPATH set, then please ensure either you unset GOPATH,
or clone the tree in `$GOPATH/src/github.com/contiv/` location
or clone the tree in `$GOPATH/src/github.com/contiv/` location. If you are
using a Makefile target that needs `GOPATH`, please set it by doing
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need this change, it is covered by the first line.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rchirakk The first line is not clear IMO. It does not clearly say "you MUST set GOPATH for certain targets". Hence, @delapsley and I both saw issues without GOPATH as there is no check for empty GOPATH in Makefile. Since there is no check for empty GOPATH, I added this clear note for the user.

Signed-off-by: Vikram Hosakote <vhosakot@cisco.com>
@vhosakot
Copy link
Member Author

vhosakot commented Oct 25, 2017

@rchirakk I removed the note in the documentation. FYI, when GOPATH is not set, there is no check in the code now and no docs too about it and the build does not fail fast. I still think the documentation is unclear 😄 because of which @delapsley and I both saw the same issue when GOPATH is not set.

@vhosakot
Copy link
Member Author

build PR

Copy link
Contributor

@rchirakk rchirakk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dseevr dseevr merged commit 83e1bda into contiv:master Oct 25, 2017
@vhosakot vhosakot deleted the remove_GOPATH_check branch October 25, 2017 18:31
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.

None yet

4 participants