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

Fix build macos #33

Closed
wants to merge 2 commits into from
Closed

Fix build macos #33

wants to merge 2 commits into from

Conversation

erikh
Copy link
Contributor

@erikh erikh commented Mar 23, 2015

This fixes build issues on my OS X workstation. I had to make several hacks to get this to work, but it's working. :) I do not know if this patch works on linux, as I don't have a good testing environment for it yet.

There is also a small patch here that fixes some errors I saw in test output when failures happened.

@erikh
Copy link
Contributor Author

erikh commented Mar 23, 2015

Build will likely fail on linux from jenkins reports I've already recieved, but jenkins seems to be down now. I'm going to get a linux environment bootstrapped probably wednesday at which point I'll revisit this patch.

@jainvipin
Copy link

@erikh - I can take your diffs and try to make it work on linux environment... and send you back the diffs. Please let me know...

@@ -1,9 +1,9 @@
# -*- mode: ruby -*-
# vi: set ft=ruby :

netplugin_synced_gopath="/opt/golang"
host_gobin_path="/opt/go/bin"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not very sure, but I think this might have a side effect in environment where host's go-binary might be in a standard path like /usr/bin.

Since we try to mount the host's environment at this location it might wipe out vagrant-vm's own binary location. So I had conservatively chosen a path unique to the vagrant-vm to avoid landing into such a situation, instead of keeping it same as host's location.

@mapuri
Copy link
Contributor

mapuri commented Mar 23, 2015

Looks good.

At a high level I think this change introduces the following:

  • In non-Linux environment, it uses vagant-vm's Go environment.
  • It allows builds inside the vagrant vm. This gives a way for the user to have a build environment spawned from the workspace. It does needs the workspace to be mounted as read-write inside the vagrant-vm, which concerns me a bit if user were to spawn a multi-node cluster environment and modify the files from the vms. They will need to be slightly careful here, with the possibility of editing files from multiple vms.

Alternatively, I was thinking if user had a linux vm installed on their non-linux host and installed Go there, would they need this still?

@erikh
Copy link
Contributor Author

erikh commented Mar 23, 2015

That sounds great Vipin; just send a pull request to my fork.

I ordered a linux workstation last night so I can hack on this some more.

-Erik

On Mar 23, 2015, at 11:07 AM, Vipin Jain notifications@github.com wrote:

@erikh https://github.com/erikh - I can take your diffs and try to make it work on linux environment... and send you back the diffs. Please let me know...


Reply to this email directly or view it on GitHub #33 (comment).

@erikh
Copy link
Contributor Author

erikh commented Mar 23, 2015

I tried this. vagrant just halted and caught fire. :)

Your summary is correct, though. The RW mount is not ideal but it is necessary for go install, IIRC. I can try playing with it again in a bit.

-Erik

On Mar 23, 2015, at 11:28 AM, Madhav Puri notifications@github.com wrote:

Looks good.

At a high level I think this change introduces the following:

In non-Linux environment, it uses vagant-vm's Go environment.
It allows builds inside the vagrant vm. This gives a way for the user to have a build environment spawned from the workspace. It does needs the workspace to be mounted as read-write inside the vagrant-vm, which concerns me a bit if user were to spawn a multi-node cluster environment and modify the files from the vms. They will need to be slightly careful here, with the possibility of editing files from multiple vms.
Alternatively, I was thinking if user had a linux vm installed on their non-linux host and installed Go there, would they need this still?


Reply to this email directly or view it on GitHub #33 (comment).

@@ -1,8 +1,10 @@
.PHONY: all build clean default system-test unit-test

TO_BUILD := ./ ./netdcli/
TO_BUILD := ./netmaster/ ./netdcli/

Choose a reason for hiding this comment

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

@erikh - tried out your changes;
Comment1:

  • this change is not only not needed, but will not build the netplugin binary appropriately. netplugin binary, which comes from netd.go is in ./ direcotry and not ./netmaster. Unless we move the file locations, let's avoid this change.

@jainvipin
Copy link

@erikh - With the three diffs mentioned above, on top of your changes, I was able to pass sanity on a linux system. The diffs were small enough so I didn't send the PR to your fork,

That sounds great Vipin; just send a pull request to my fork. I ordered a linux workstation last night so I can hack on this some more.

@erikh
Copy link
Contributor Author

erikh commented Mar 26, 2015

NP, I'll fix it up. Not sure how I missed the shell bit.

Thanks Vipin!

-Erik

On Mar 26, 2015, at 1:25 AM, Vipin Jain notifications@github.com wrote:

@erikh https://github.com/erikh - With the three diffs mentioned above, on top of your changes, I was able to pass sanity on a linux system. The diffs were small enough so I didn't send the PR to your fork,

That sounds great Vipin; just send a pull request to my fork. I ordered a linux workstation last night so I can hack on this some more.


Reply to this email directly or view it on GitHub #33 (comment).

@erikh
Copy link
Contributor Author

erikh commented Mar 26, 2015

There seems to be too many problems with this patch, after some thought. I have a linux box now so my investment here is greatly reduced. :)

@erikh erikh closed this Mar 26, 2015
dseevr pushed a commit to dseevr-dev/netplugin that referenced this pull request Nov 28, 2017
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

3 participants