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-519] Bump Eclipse JGit dependency to latest version #535

Closed
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@ocafebabe

ocafebabe commented May 6, 2018

No description provided.

@matthiasblaesing

The unittests of libs.git show compile errors and even after fixing them, they all fail with class loading problems. These need to be fixed.

These are the stats before the change: 331 passed, 1 failed, 14 errored
After the change: no test passed.

revWalk.release();
walk.release();
revWalk.close();
walk.close();

This comment has been minimized.

@matthiasblaesing

matthiasblaesing May 6, 2018

Contributor

I suggest to move this to try-with-resource. The method renaming looks as if this is the intended reason.

There are other places, that can be modified accordingly.

This comment has been minimized.

@ocafebabe

ocafebabe May 7, 2018

Weird because it compiles just fine on my side! What ant target are you using to run the tests?

<jar destfile="external/org.eclipse.jgit-4.11.0.201803080745-r-nbm.jar" manifest="manifest.mf">
<fileset dir="external/tmp" />
</jar>
</target>

This comment has been minimized.

@matthiasblaesing

matthiasblaesing May 6, 2018

Contributor

Why are you repackaging the jar there? I don't think this is necessary.

This comment has been minimized.

@ocafebabe

ocafebabe May 7, 2018

Well, this is based on a recommendation from a previous reviewer (@emilianbold). JGit is an OSGi bundle and it depends on some OSGi dependencies that aren't available as OSGi bundles in NetBeans (like JSCH for instance). With this change I'm repackaging the library as a regular NetBeans module!

See this post for more details: https://lists.apache.org/thread.html/ac7fc5d9e2ddaa8ccd7a43dde4bc7fa80a254a11a8a4dc376811cebb@%3Cdev.netbeans.apache.org%3E

@@ -1 +1,4 @@
OpenIDE-Module: org.eclipse.jgit
OpenIDE-Module-Specification-Version: 4.11.0
OpenIDE-Module-Implementation-Version: 4.11.0.201803080745-r
OpenIDE-Module-Public-Packages: org.eclipse.jgit.**

This comment has been minimized.

@matthiasblaesing

matthiasblaesing May 6, 2018

Contributor

Are these changes necessary?

This comment has been minimized.

@ocafebabe

ocafebabe May 7, 2018

Yes, based on my previous comment...

@matthiasblaesing

This comment has been minimized.

Contributor

matthiasblaesing commented May 7, 2018

Sorry - this still breaks the unittests. I switched to your branch and build from source (run ant from the base directory of the repository) and the I changed into the libs.git directory and ran ant test. All tests fail:

   [junit] Null Test: 	Caused an ERROR
    [junit] Invalid signature file digest for Manifest main attributes
    [junit] java.lang.SecurityException: Invalid signature file digest for Manifest main attributes
    [junit] 	at sun.security.util.SignatureFileVerifier.processImpl(SignatureFileVerifier.java:330)
    [junit] 	at sun.security.util.SignatureFileVerifier.process(SignatureFileVerifier.java:263)
    [junit] 	at java.util.jar.JarVerifier.processEntry(JarVerifier.java:275)
    [junit] 	at java.util.jar.JarVerifier.update(JarVerifier.java:230)
    [junit] 	at java.util.jar.JarFile.initializeVerifier(JarFile.java:383)
    [junit] 	at java.util.jar.JarFile.getInputStream(JarFile.java:450)
    [junit] 	at sun.misc.URLClassPath$JarLoader$2.getInputStream(URLClassPath.java:977)
    [junit] 	at sun.misc.Resource.cachedInputStream(Resource.java:77)
    [junit] 	at sun.misc.Resource.getByteBuffer(Resource.java:160)
    [junit] 	at java.net.URLClassLoader.defineClass(URLClassLoader.java:454)
    [junit] 	at java.net.URLClassLoader.access$100(URLClassLoader.java:73)
    [junit] 	at java.net.URLClassLoader$1.run(URLClassLoader.java:368)
    [junit] 	at java.net.URLClassLoader$1.run(URLClassLoader.java:362)
    [junit] 	at java.security.AccessController.doPrivileged(Native Method)
    [junit] 	at java.net.URLClassLoader.findClass(URLClassLoader.java:361)
    [junit] 	at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
    [junit] 	at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:338)
    [junit] 	at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
    [junit] 	at java.lang.Class.forName0(Native Method)
    [junit] 	at java.lang.Class.forName(Class.java:264)
    [junit] 
    [junit] 
    [junit] Test org.netbeans.libs.git.jgit.commands.CherryPickTest FAILED
    [junit] Testsuite: org.netbeans.libs.git.jgit.commands.CleanTest
    [junit] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0 sec

You need to strip the *.SF and *.RSA files from the META-INF directory. Please have a look at the com.jcraft.jsch module. There OSGI meta data is added to the plain jar. Maybe this would be an alternative way.

@ocafebabe

This comment has been minimized.

ocafebabe commented May 7, 2018

But why does the build ran successfully on Travis after my commits yesterday? Aren't the tests executed on this environment?

@ocafebabe

This comment has been minimized.

ocafebabe commented May 7, 2018

Regarding the com.jcraft.jsch module, yes you're right about the OSGi meta data that is added to the original jar, but it doesn't work with JGit!

Look at the exception I got during my initial work on this PR (this was before using JGit as a NetBeans module):

org.osgi.framework.BundleException: The bundle "org.eclipse.jgit_4.11.0.201803080745-r [183]" could not be resolved. Reason: Missing Constraint: Import-Package: com.jcraft.jsch; version="[0.1.37,0.2.0)"
    at org.eclipse.osgi.framework.internal.core.AbstractBundle.getResolverError(AbstractBundle.java:1332)
    at org.eclipse.osgi.framework.internal.core.AbstractBundle.getResolutionFailureException(AbstractBundle.java:1316)
    at org.eclipse.osgi.framework.internal.core.BundleHost.startWorker(BundleHost.java:323)
    at org.eclipse.osgi.framework.internal.core.AbstractBundle.resume(AbstractBundle.java:390)
    at org.eclipse.osgi.framework.internal.core.Framework.resumeBundle(Framework.java:1184)
    at org.eclipse.osgi.framework.internal.core.StartLevelManager.resumeBundles(StartLevelManager.java:559)
    at org.eclipse.osgi.framework.internal.core.StartLevelManager.resumeBundles(StartLevelManager.java:544)
    at org.eclipse.osgi.framework.internal.core.StartLevelManager.incFWSL(StartLevelManager.java:457)
    at org.eclipse.osgi.framework.internal.core.StartLevelManager.doSetStartLevel(StartLevelManager.java:243)
    at org.eclipse.osgi.framework.internal.core.EquinoxLauncher.internalStart(EquinoxLauncher.java:271)
    at org.eclipse.osgi.framework.internal.core.EquinoxLauncher.start(EquinoxLauncher.java:241)
    at org.eclipse.osgi.launch.Equinox.start(Equinox.java:258)
    at org.netbeans.core.netigso.Netigso.start(Netigso.java:183)
    at org.netbeans.NetigsoHandle.startFramework(NetigsoHandle.java:186)
    at org.netbeans.ModuleManager.enable(ModuleManager.java:1344)
    at org.netbeans.ModuleManager.enable(ModuleManager.java:1148)
    at org.netbeans.core.startup.ModuleList.installNew(ModuleList.java:315)
    at org.netbeans.core.startup.ModuleList.trigger(ModuleList.java:251)
    at org.netbeans.core.startup.ModuleSystem.restore(ModuleSystem.java:276)
    at org.netbeans.core.startup.Main.getModuleSystem(Main.java:156)
    at org.netbeans.core.startup.Main.getModuleSystem(Main.java:125)
    at org.netbeans.core.startup.Main.start(Main.java:282)
    at org.netbeans.core.startup.TopThreadGroup.run(TopThreadGroup.java:98)
[catch] at java.lang.Thread.run(Thread.java:748)
@ocafebabe

This comment has been minimized.

ocafebabe commented May 8, 2018

I've found out why the com.jcraft.jsch module isn't working with JGit: it's because the export-package attribute is missing the version value (see http://wiki.netbeans.org/OSGiAndNetBeansMetadataMapping):

<attribute name="Export-Package" value="com.jcraft.jsch;version=0.1.54"/>

Indeed, after adding the version, the module is found by JGit!

I guess I could do the same for SLF4J and then leave JGit intact (no metadata alteration)!

What do you think?

@matthiasblaesing

This comment has been minimized.

Contributor

matthiasblaesing commented May 8, 2018

Christian, thank you for your investigation. I'll answer your first question first:

But why does the build ran successfully on Travis after my commits yesterday? Aren't the tests executed on this environment?

Travis does a full build - including tests. Even before the migration to apache not all unittests of the code base were excercised before a tree merge. Some of the unittests are currently broken and other just plain take much to long to run on each build. It is always a good idea to check the unittests of packages, you know that are affected by your change explicitly.

Analysis Result: The problem lies in the switch from bundle versioning to package versioning, which was not correctly mapped in the jsch case and is missing in slf4.

This sounds like a good route to take. Every dependency we can take without modifications is a good dependency (and before a nitpicker takes this on - every depedency we don't need is even better).

@ocafebabe

This comment has been minimized.

ocafebabe commented May 8, 2018

@matthiasblaesing no worries and thanks a lot for your feedback!

Duly noted regarding the tests and I totally agree with you about the dependency modifications! I'll remove the jar modifications I made to JGit and take care of SLF4J as well!

The only other remaining issue is the missing servlet-api OSGi bundle but this dependency isn't even used by JGit (it was reintroduced by mistake)! I already warned the lead developer and he removed it but he hasn't released a new version yet! So I'll complete the PR as soon as he does it! See this commit for details...

@matthiasblaesing

This comment has been minimized.

Contributor

matthiasblaesing commented May 9, 2018

@ocafebabe agreed - I added the work-in-progress label, so that it is clear, that this PR should not yet be merged. Please remove the label when you want this revisited.

@ocafebabe

This comment has been minimized.

ocafebabe commented Aug 16, 2018

@matthiasblaesing some tests in libs.git are failing because they're trying to connect to an external remote host: ssh://vcs-test.cz.oracle.com/srv/git/repo/ which doesn't exist anymore!

It seems very fragile to me for a unit test to rely on an external service, don't you think?

@matthiasblaesing

This comment has been minimized.

Contributor

matthiasblaesing commented Aug 18, 2018

Thanks you for the update. This PR got a bit stale, so it would be great if you could squash the current commits and rebase the changes onto current master.

Comments right now:

  • for the slf4j library, the name of the license file needs to be aligned with the binary it covers (or you need to place a Files header into the license file. So please update the name to slf4j-api-1.7.2-license.txt
  • the license requires the copyright to be included, so the file needs to be present with the year + names. Add a new license "MIT-SLF4J" or similar to nbbuild/licenses and reference that ffrom the license file in the module (the license text also needs to be included)
  • the unittest checking for Java7 Extension CheckJava7ExtensionTest could be removed (I assume that code was folded into the new version?)
  • unittests fail because spelling of message was changed and different file status are reported

The unittests that rely on a remote service are indeed more integration test. You could change them to depend on a property (jgittesthost or something like that) to make it possible to run them from CLI, but not be run by default.

@ocafebabe

This comment has been minimized.

ocafebabe commented Aug 20, 2018

@matthiasblaesing Thanks for you comments!

What's the ant target to validate licence files? It would be neat if it was documented somewhere...

@matthiasblaesing

This comment has been minimized.

Contributor

matthiasblaesing commented Aug 20, 2018

The validation can be invoked via:

ant -Dcluster.config=full verify-all-libs-and-licenses

this will download all external binaries (verify-all-libs-and-licenses) and validate all licenses (the full cluster should include all modules).

The result is then written to: nbbuild/build/verifylibsandlicenses.xml

@lkishalmi

This comment has been minimized.

Contributor

lkishalmi commented Sep 30, 2018

@ocafebabe Could you rebase this PR on the current master?
Also what is the actual state of resolving this issue?

@ocafebabe

This comment has been minimized.

ocafebabe commented Oct 1, 2018

@ocafebabe Could you rebase this PR on the current master?
Also what is the actual state of resolving this issue?

I tried to rebase but it seems impossible because I get a ton of conflicts! All modules were moved since I started my work on this PR! I'll close this one and start a new fresh one...

Thanks

@ocafebabe ocafebabe closed this Oct 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment