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

unix sockets on windows #1637

Merged
merged 4 commits into from
Apr 7, 2024
Merged

Conversation

Catalyn45
Copy link
Contributor

@Catalyn45 Catalyn45 commented Mar 13, 2024

Windows supports unix domain sockets for some years now but they are not used in lf. Changed the default socket to unix on windows to bring the benefits of it on the windows platform also. If the unix sockets are not supported ( very old version of windows ) fallback on tcp localhost sockets.

https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/

@Catalyn45
Copy link
Contributor Author

Catalyn45 commented Mar 14, 2024

I can delete the check for unix sockets support if we don't want to support EOL windows platforms, like windows 7, 8 and a few of the first versions of windows 10.

Copy link
Collaborator

@joelim-work joelim-work left a comment

Choose a reason for hiding this comment

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

Thanks for the patch, it looks fine overall to me.

Currently we have a change freeze since we are about to release a new version r32, see #1427 (comment). So there's a possibility this might not be merged until after that, but I don't mind either way.

os_windows.go Outdated
gDefaultSocketPath = filepath.Join(data, "lf", fmt.Sprintf("lf.%s.sock", gUser.Username))
syscall.Close(socket)
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Slightly pedantic, but I suggest to move this to the bottom of the init function after the path definitions, to maintain the same order as os.go.

Copy link
Collaborator

@joelim-work joelim-work left a comment

Choose a reason for hiding this comment

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

Thanks for the changes once again.

Regarding support for IP sockets, I'm not sure about this since I'm not a Windows user. There's also the question of whether the socket should be created in os.TempDir instead of data, which is more for files representing the state of lf.

I think it might be better if leave this PR open for a bit longer to see if anyone else wants to comment.

@Catalyn45
Copy link
Contributor Author

I'm actively using this change on my Windows machine and I haven't encountered any problems yet. About the creation directory of the socket: yes, maybe TempDir is a better option.

I agree with you, let's wait for other people to see their opinions.

@gokcehan gokcehan merged commit 21d256d into gokcehan:master Apr 7, 2024
4 checks passed
@joelim-work joelim-work added this to the r33 milestone Jun 24, 2024
@joelim-work joelim-work added the new Pull requests that add new behavior label Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new Pull requests that add new behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants