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

[FIXED JENKINS-14351] Drop old jna-posix library and use jnr-posix (2.5.2) instead #770

Closed
wants to merge 2 commits into
base: master
from

Conversation

4 participants
@msrb
Copy link
Member

msrb commented May 3, 2013

JENKINS-14351: jna-posix 1.0.3 is quite old and no longer maintained. jnr-posix is still in active development.

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

cloudbees-pull-request-builder commented May 3, 2013

core » jenkins_main_trunk #857 UNSTABLE
Looks like there's a problem with this pull request

@jglick

This comment has been minimized.

Copy link
Member

jglick commented May 7, 2013

I am a little nervous about swapping the library since plugins might have been using it, but I hope they are using only the Jenkins classes.

To what extent has this been tested other than it compiles?

@msrb

This comment has been minimized.

Copy link
Member Author

msrb commented May 14, 2013

To be completely honest, it has not been tested very much. Basically only I am using it on my private jenkins instance with very few plugins installed.

You might be right that switching to library with slightly incompatible API could cause problems in some plugins. I will try to investigate and test it more and will come back with results.

Anyway, thanks for your time.

@jglick

This comment has been minimized.

Copy link
Member

jglick commented May 21, 2013

I think we want to do this, I am just wondering what sanity checks are needed. I have been encouraging people running Java 6 on AIX or HP/UX (who suffer from the current implementation of symlink handling in Util) to evaluate this PR as it might be very useful on those platforms.

By the way SurefireArchiverUnitTest.testArchiveResults is now skipped on the PR builder so do not worry about that test failure.

@jglick

This comment has been minimized.

Copy link
Member

jglick commented May 30, 2013

Comments in JENKINS-13202 suggest that this change would be helpful to AIX users at least, though none have stepped forward to test.

This pull request is not currently mergeable, by the way; need to git merge master.

@stephenc

This comment has been minimized.

Copy link
Member

stephenc commented May 30, 2013

If you are nervous about breaking anyone relying on the old library being present, why not just leave the jna-posix library on the classpath (i.e. <scope>runtime</scope>) and get the version out with the switch over out. Then after a couple of releases drop the jna-posix altogether

Merge remote-tracking branch 'upstream/master'
Conflicts:
	core/src/main/java/hudson/Util.java
@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

cloudbees-pull-request-builder commented May 31, 2013

core » jenkins_main_trunk #901 SUCCESS
This pull request looks good

@jglick

This comment has been minimized.

Copy link
Member

jglick commented May 31, 2013

Downloaded jenkins.war and build permalinks work on JDK 6 on Ubuntu. Not sure if that means anything—was probably using JNA anyway.

jenkins-all grepping turns up no plugin usages of PosixException, so we can safely ignore that, and in fact delete the getErrorCode method as an implementation detail—we do not want to “leak” this library into the Jenkins API.

For org.jruby.ext.posix there are a couple usages:

maven-repo-cleaner-plugin/src/main/java/org/jenkinsci/plugins/mavenrepocleaner/RepositoryCleaner.java
pam-auth-plugin/src/main/java/hudson/security/PAMSecurityRealm.java

and in fact clicking the Test button for Unix user/group database gives an error:

java.lang.NoSuchMethodError: hudson.os.PosixAPI.get()Lorg/jruby/ext/posix/POSIX;
    at hudson.security.PAMSecurityRealm$DescriptorImpl.doTest(PAMSecurityRealm.java:133)

Seems that these plugins need to explicitly declare their jn*-posix dependency and not just assume core will provide it. Since pam-auth-plugin is bundled this would add to the jenkins.war download size, though at least jna-posix-1.0.3.jar seems pretty small.

There are also usages from tmpcleaner-plugin but it seems it declares its own dependency so that is no problem.

@ghost ghost assigned jglick May 31, 2013

@jglick

This comment has been minimized.

Copy link
Member

jglick commented May 31, 2013

It is worse than I thought. These plugins all use PosixAPI, which is changed incompatibly in the branch:

file-leak-detector-plugin
maven-repo-cleaner-plugin
pam-auth-plugin
sicci-for-xcode-plugin
tmpcleaner-plugin

Looks like this class needs to be @Deprecated but left alone. I will try to rework the branch accordingly.

@jglick jglick closed this May 31, 2013

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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.