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

Windows port #138

Closed
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@petrsnm
Copy link
Contributor

petrsnm commented Jul 20, 2017

These changes make it so that kr and krd will build on windows. At the least, these changes provide a good foundation for additional window-specific changes that we may need to make later.

I agree to license all rights to my contributions in each modified file exclusively to KryptCo, Inc.

@kcking
Copy link
Member

kcking left a comment

Thanks for the start of a Windows port! It would be great to minimize the duplicated code in socket*.go files. I think the only OS-specific functions should be KrDir, Listen, and Dial, where Listen and Dial return the same interfaces as net.Listen and net.Dial in https://godoc.org/net


var analytics_user_agent = fmt.Sprintf("Mozilla/5.0 (10.0.15063; Win64) (KHTML, like Gecko) Version/%s kr/%s", CURRENT_VERSION, CURRENT_VERSION)

const analytics_os = "Linux"

This comment has been minimized.

@kcking

kcking Jul 24, 2017

Member

"Windows"

This comment has been minimized.

@petrsnm

petrsnm Jul 25, 2017

Author Contributor

Oops. Yes will fix this.

@@ -0,0 +1,181 @@
package kr

This comment has been minimized.

@kcking

kcking Jul 24, 2017

Member

It looks like most of these functions didn't change from socket.go. Can we split these into
socket.go for common functions
socket_windows.go for windows-specific impl (looks like KrDir() and a way to Listen or Dial should be the only windows specific ones)
socket_unix.go for linux/darwin/freebsd impls of KrDir, Listen, Dial

This comment has been minimized.

@petrsnm

petrsnm Jul 25, 2017

Author Contributor

I wasn't very comfortable with the amount of cut-n-paste either, but I thought it might make you more confident in merging if the pull request didn't change socket.go :-) ... so yes. I'll take a shot at refactoring to make sure everything common is in socket.go

This comment has been minimized.

@petrsnm

petrsnm Jul 25, 2017

Author Contributor

Ok. I had some time tonight to look at this a little closer... KrDir refactors really well into windows/unix variants. However, it's not as simple for Listen and Dial. I think we're going to need windows and unix variants of all 4 of the following:

  • AgentListen
  • DaemonListen
  • HostAuthListen
  • DaemonDial

This seems like a cleaner abstract than needing to reference the <AGENT|DAEMONHOST_AUTH>_SOCKET_NAME constants elsewhere in he code.

I've got the beginnings of a commit for this approach, but before I tidy it up and push it, I figured as what you think. Thought?

This comment has been minimized.

@kcking

kcking Jul 25, 2017

Member

Sure that should work!

This comment has been minimized.

@petrsnm

petrsnm Jul 27, 2017

Author Contributor

I've consolidated common code in socket.c with OS-specific AgentListen, DaemonListen, HostAuthListen, and DaemonDial. As discussed via phone there is still some significant cut-n-paste between socket_linux and socket_darwin. I suggest we merge this pull request then go back and tidy that up.

The majority of commit 4 is actually related to getting all of the unit tests to run on windows. It was notably intrusive to fix pairOver() and unPairOver() because they were coded to accept a unix socket name. The fix required significant refactoring to always use the sockets abstraction.

I've tested on windows and on linux. I haven't tested on darwin (because I don't have a mac).

@kcking kcking closed this Oct 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.