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

[NETBEANS-2509] Bring back docker plugin support for unix sockets #1242

Closed
wants to merge 1 commit into from
Closed

[NETBEANS-2509] Bring back docker plugin support for unix sockets #1242

wants to merge 1 commit into from

Conversation

Ihsahn
Copy link
Contributor

@Ihsahn Ihsahn commented May 9, 2019

Restore ability to connect to docker via unix socket, replaced no-longer existing socket access library with (wrapped) com.kohlschutter.junixsocket (Apache License).

Checked on linux/amd64 - I was able to connect and list images/containers.

ps. according to junixsocket docs this should also work on linux/aarch64 and macos, but since I don't have ability to test on these I'm not enabling them (yet)

@Ihsahn Ihsahn changed the title [NETBEANS-2509' Bring back docker plugin support for unix sockets [NETBEANS-2509] Bring back docker plugin support for unix sockets May 9, 2019
// }
// String arch = System.getProperty("os.arch"); // NOI18N
// return arch != null && (arch.contains("x86") || arch.contains("amd64")); // NOI18N
if (BaseUtilities.getOperatingSystem() != BaseUtilities.OS_LINUX) {
Copy link
Member

Choose a reason for hiding this comment

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

What about MacOS X ? junixsocket's documenation says it supports mac too...

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 stated this at the very top already: I don't have ability to verify this on linux/aarch64 and macos

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to doubt, that this will work on Mac OS X? If not I would enable it there also (I'm assuming, that docker on Mac OS X also uses unix domain sockets).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed (afaik MacOs does allow connection via unix domain sockets)

@matthiasblaesing
Copy link
Contributor

The binaries should not be checked in to git. The netbeans build system has its own code to download binaries at build time. The mechanism was updated to also work with maven central, when netbeans was donated to apache.

For a sample have a look at the jna module: platform/libs.jna. There are two things you'll need:

  • external/binaries-list: This lists the binaries with SHA1 sums
  • external/<binary-name>-<version>-license.txt with the license information about the file

-->
<project name="ide/libs.c.kohlschutter.junixsocket" default="build" basedir=".">
<import file="../../nbbuild/templates/projectized.xml"/>
</project>

Choose a reason for hiding this comment

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

The library seems to be on Maven central. Make sure to create external/binaries-files to download the JARs from Maven central instead of having them in the Git repository.

Copy link
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

Thank you for the update and move to maven central dependencies. I left some comments inline.

Please squash your commits and update the author information. Neither the realname part, nor the email part of the author information look right (https://www.git-tower.com/learn/git/faq/change-author-name-email / https://stackoverflow.com/questions/3042437/how-to-change-the-commit-author-for-one-specific-commit)

@@ -0,0 +1,210 @@
Name: junixsocket
License: Apache-2.0
OSR: 4300
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an artifact from the oracle/sun era. Please remove.

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

OSR: 4300
Description: junixsocket is a Java/JNI library that allows the use of Unix Domain Sockets (AF_UNIX sockets) from Java.
Origin: https://github.com/kohlschutter/junixsocket/
Version: 2.2.0
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need for three copies of the license file. Please remove two copies and rename the remaining copy to "junixsocket-license.txt". Add a new header line that references the three files:

Files: junixsocket-common-2.2.0.jar, junixsocket-core-2.2.0.jar, junixsocket-native-common-2.2.0.jar

What is more, the ALv2 requires to carry the NOTICE if the source package has one. In this case the original author failed to include it in his/her binaries, but we are good citizens of the ecosystem.

Here is the notice file:
https://github.com/kohlschutter/junixsocket/blob/master/NOTICE

Please add a file "junixsocket-notice.txt", with the contents of the above referenced file. The contents of that file will be picked up by the build system and integrated into the final notice file of NetBeans.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

licenses/notice changed/updated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some license files have incorrect headers
ide/libs.c.kohlschutter.junixsocket/external/junixsocket-license.txt does not contain the version 2.2.0 in its name

and if I add version to this, than it states, that there are 3 files and only one license, and so on...

// }
// String arch = System.getProperty("os.arch"); // NOI18N
// return arch != null && (arch.contains("x86") || arch.contains("amd64")); // NOI18N
if (BaseUtilities.getOperatingSystem() != BaseUtilities.OS_LINUX) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to doubt, that this will work on Mac OS X? If not I would enable it there also (I'm assuming, that docker on Mac OS X also uses unix domain sockets).

@matthiasblaesing
Copy link
Contributor

This looks reasonably sane. Your test-failure is explainable, as I gave you the wrong filenames, sorry about that. Please move the license and notice to junixsocket-2.2.0-license.txt and junixsocket-2.2.0-notice.txt. I'd also make the junixsocket library friend-only for now. That way we know which modules to check when we update the library. This can be done in the GUI (Project Properties -> API Versionioning -> Export Packages only to Friends. Or directly in the project.xml:

<!-- ... -->
            <!-- Remove public-packages element -->
            <friend-packages>
                <friend>org.netbeans.modules.docker.api</friend>
                <package>org.newsclub.net.unix</package>
            </friend-packages>
<!-- ... -->

@Ihsahn
Copy link
Contributor Author

Ihsahn commented May 16, 2019

This looks reasonably sane. Your test-failure is explainable, as I gave you the wrong filenames, sorry about that. Please move the license and notice to junixsocket-2.2.0-license.txt and junixsocket-2.2.0-notice.txt. I'd also make the junixsocket library friend-only for now. That way we know which modules to check when we update the library. This can be done in the GUI (Project Properties -> API Versionioning -> Export Packages only to Friends. Or directly in the project.xml:

Done.

@neilcsmith-net
Copy link
Member

I'd also make the junixsocket library friend-only for now. That way we know which modules to check when we update the library.

-0 That doesn't seem the best reason to do that?! I'm not a fan of the friend concept in general, particularly with third-party libraries. Anything adding in native libraries that way can be a real pain for platform / plugin developers.

@matthiasblaesing
Copy link
Contributor

Ok - I take your point Neil. To no play request ping-pong, I rebased the changeset onto master, reopened the library module and merged it to master (0d3a544).

Thank you for your contribution Bartosz.

@Ihsahn Ihsahn deleted the feat/junixsocket branch February 24, 2020 06:15
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

5 participants