search

spotify / docker-client

INACTIVE: A simple docker client for the JVM
Apache License 2.0
1.43k stars 549 forks source link

Usage of jnr-unixsocket for unix socket support #98

Closed rgrunber closed 9 years ago

rgrunber commented 9 years ago

I would like to propose changing docker-client's unix socket support from using unix-socket-factory (junix-socket) to jnr-unixsocket [1]. The latter is more actively developed and also used in JRuby.

Currently it seems docker-client requires de.gesellix:unix-socket-factory but only uses a single bundled class (org.newsclub.net.unix.AFUNIXSocket) from junixsocket (likely since this provides the classes through a mavenized resource). The version that is bundled is listed as 1.4 [2], but the project's latest release was 1.3 [3] so it's unclear how to reproduce it.

NOTE: There are some additional changes that I would need to propose and push into jnr-unixsocket to support some necessary socket options. Once they're completed, I could create a pull request.

The changes can be found at : https://github.com/rgrunber/docker-client https://github.com/rgrunber/jnr-unixsocket

[1] https://github.com/jnr/jnr-unixsocket [2] https://github.com/gesellix/unix-socket-factory/tree/master/libs [3] https://code.google.com/p/junixsocket/

rohansingh commented 9 years ago

I would like to propose changing docker-client's unix socket support from using unix-socket-factory (junix-socket) to jnr-unixsocket [1]. The latter is more actively developed and also used in JRuby.

Sounds great.

Currently it seems docker-client requires de.gesellix:unix-socket-factory but only uses a single bundled class (org.newsclub.net.unix.AFUNIXSocket) from junixsocket (likely since this provides the classes through a mavenized resource).

That's correct, we're only using it since it's Mavenized. I would like to drop it though since it's not actively maintained.

Once they're completed, I could create a pull request.

I'd be happy to merge that. Skimmed the changes in your branch and they look good at a first glance.

rohansingh commented 9 years ago

@rgrunber I noticed that jnr/jnr-unixsocket has barely been touched since 2012, so it seems unlikely that your PR for it will get merged.

rgrunber commented 9 years ago

It hasn't received much attention in a while, but jnr-posix on the other hand is active, so I figured maybe they only developed it enough to satisfy their JRuby needs. The maintainer I spoke to was open to accepting the contribution though.

The commit referenced has been merged into jnr-unixsocket, and it is included in the newly tagged 0.4 release. It has also been deployed to maven central. I'll create a pull request with the updated version, rebased against the latest master. From there it can be reviewed.

rohansingh commented 9 years ago

Great! I stand corrected :)