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

Make Mirror a unix only feature #16

Merged
merged 6 commits into from
Jul 7, 2017

Conversation

jrossi
Copy link
Contributor

@jrossi jrossi commented Jun 23, 2017

This should correct #11 as mirror is not something that is going to work on windows at the least with the method currently used.

This patch is not prefect in anyway due the way that configuration is implement in vflow. I more then happy for work on something a little easier to support, but will start that chat in another issues.

@mehrdadrad
Copy link
Collaborator

@jrossi very cool, I like build tag solution! but let's keep makefile simple without change.
Do we need to add +build windows at ipfix_windows.go? Thanks!

//: See the License for the specific language governing permissions and
//: limitations under the License.
//: ----------------------------------------------------------------------------

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggesting that we should // +build windows in this file. That works, but in the case it's also that way based on the file name as _windows.go does the same things.

Copy link
Collaborator

Choose a reason for hiding this comment

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

nice 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding

@@ -37,4 +37,34 @@ depends:
go get -d ./...

build: depends
cd vflow; go build $(LDFLAGS) -o vflow $(GOFILES)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The request about the make file. Due to pushing all the logic into the make file we lose the build control of go tool thank. When I removed GOFILES from here problems all go away :)

Makefile Outdated
cd vflow; go build $(LDFLAGS)

build-windows: depends
cd vflow; gox $(LDFLAGS)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This something worth having as GOX is one of the many cross compile tools just happens to be the one I use. Should that not be used? Or something else?

Copy link
Collaborator

Choose a reason for hiding this comment

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

GOX is cool but why you think we need to have it at vFlow and add one more 3rd party package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't will be happy to remove it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

Makefile Outdated
vflow/vflow_linux_amd64: depends
cd vflow; gox $(LDFLAGS) -os="linux" -arch="amd64"

cross-compile: vflow/vflow_windows_386.exe vflow/vflow_windows_amd64.exe vflow/vflow_darwin_386 vflow/vflow_darwin_amd64 vflow/vflow_freebsd_386 vflow/vflow_freebsd_amd64 vflow/vflow_linux_386 vflow/vflow_linux_amd64
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All this was me being lazy. Happy to remove it just make something simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@@ -10,16 +10,16 @@ test:
bench:
go test -v ./... -bench=. -timeout 2m

run:
cd vflow; go run $(GOFILES) -sflow-workers 100 -ipfix-workers 100
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once again with the go tool chain in the way things like +build windows does not line up here.

I see a few choices:

  • build then run
  • go run static list of files in makefile
  • go run dynamic list of files in makefile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is somethign that gets weird by having the logic of go within the Makefile, and not amking use of the go build tools:

cd vflow; go run vflow.go ipfix.go ipfix_windows.go ipfix_unix.go sflow.go netflow_v9.go options.go stats.go  -sflow-workers 100 -ipfix-workers 100
# command-line-arguments
./ipfix_unix.go:31: mirrorIPFIXDispatcher redeclared in this block
	previous declaration at ./ipfix_windows.go:25
make: *** [run] Error 2

This happens becuase the build controls don't over write your manually adding the file to the cli. So the only way I can think of making this work is pushing the logic into the Makefile (gross) or using go build and then running with CLI?

Happy to do what ever you think is correct.

Copy link
Collaborator

Choose a reason for hiding this comment

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

removing GOFILES is ok but we need to revert other changes at Makefile.

@mehrdadrad mehrdadrad merged commit 034b5d3 into Edgio:master Jul 7, 2017
@mehrdadrad
Copy link
Collaborator

Thanks Jeremy for changes!

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.

2 participants