Fix App Manager failure to load bundles when running in Development mode

This error appears on startup and on each time that I refresh an app repository:

Exception in thread "JavaFX Application Thread" java.lang.NullPointerException
at org.lorainelab.igb.plugin.manager.BundleInfoManager.lambda$isVersionOfBundleInstalled$48(BundleInfoManager.java:149)
at java.util.stream.MatchOps$1MatchSink.accept(MatchOps.java:90)
at java.util.Spliterators$ArraySpliterator.tryAdvance(Spliterators.java:958)
at java.util.stream.ReferencePipeline.forEachWithCancel(ReferencePipeline.java:126)
at java.util.stream.AbstractPipeline.copyIntoWithCancel(AbstractPipeline.java:498)
at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:485)
at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:471)
at java.util.stream.MatchOps$MatchOp.evaluateSequential(MatchOps.java:230)
at java.util.stream.MatchOps$MatchOp.evaluateSequential(MatchOps.java:196)
at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
at java.util.stream.ReferencePipeline.anyMatch(ReferencePipeline.java:449)
at org.lorainelab.igb.plugin.manager.BundleInfoManager.isVersionOfBundleInstalled(BundleInfoManager.java:149)
at org.lorainelab.igb.plugin.manager.AppManagerFxPanel.lambda$null$5(AppManagerFxPanel.java:150)
at java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1374)
at java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:580)
at org.lorainelab.igb.plugin.manager.AppManagerFxPanel.lambda$udpateDataEventNotification$6(AppManagerFxPanel.java:149)
at com.sun.javafx.application.PlatformImpl.lambda$null$173(PlatformImpl.java:295)
at java.security.AccessController.doPrivileged(Native Method)
at com.sun.javafx.application.PlatformImpl.lambda$runLater$174(PlatformImpl.java:294)
at com.sun.glass.ui.InvokeLaterDispatcher$Future.run(InvokeLaterDispatcher.java:95)

When I checkout older commits and build everything is fine.
But on the latest commit to master the app manager does not display any apps.

The latest commit was to pull in the branch for IGBF-1098. I downloaded the installer for that branch, and IGB from that. In that, the app manager is fine. I can only assume there is some flaw in my process here, something that didn't get fully reset.

When I used the installer from master, things are fine.

Can anyone else reproduce this?
Is this an issue with my machine? (mac)
Could this be an issue with local builds or that doesn't affect the installer ?

I checked out master and tried to reproduce the issue in my system.
But I was not able to do so.

After testing on multiple machines we learned this much more:

Installers are good. Every time we run IGB using an installer, there is no error, all is well.
The problem is introduced after running IGB through Netbeans.
The problem is not limited to running IGB in Netbeans; after running IGB through Netbeans, running IGB through the command line generates the error.

To demonstrate this...
$ mvn clean install -DskipTests=true
$ java -jar igb_exe.jar
--> all is well
(though some of the time the default repository is disabled, not sure why, but the apps appear once the repo is enabled)

$ java -jar igb_exe.jar
--> all is well
Running it a second time demonstrates that this command can go through the entire program life-cycle without introducing the error.

Now go to Netbeans. Run main.
--> Error, no apps. (even with repos enabled)

$ java -jar igb_exe.jar
--> Error, no apps. (even with repos enabled)

$ mvn clean install -DskipTests=true
$ java -jar igb_exe.jar
--> all is well

Netbeans running IGB introduces the problem.
The problem affects IGB even when run outside of Netbeans.
Maven clean removes the problem.

Netbeans doesn't do magic. When it runs IGB its just using a system command just like we are when we run in the terminal.
It just has a lot of details in its system command that we don't have.

Here is the command Netbeans uses when you run main:
(note, I have added newlines and spacing to make it easier to read)

cd /Users/ivory/Documents/Repos/integrated-genome-browser/main;

JAVA_HOME=/Library/Java/JavaVirtualMachines/jdk1.8.0_131.jdk/Contents/Home \
"/Applications/NetBeans/NetBeans 8.2.app/Contents/Resources/NetBeans/java/maven/bin/mvn" \
-Dexec.executable=/Library/Java/JavaVirtualMachines/jdk1.8.0_131.jdk/Contents/Home/bin/java \
-Dexec.mainClass=com.affymetrix.main.Main \
"-Dexec.args=-Xmx1g
-DdevelopmentMode=true
-Dapple.laf.useScreenMenuBar=true
-Dsun.java2d.opengl=true
-classpath %classpath com.affymetrix.main.Main" \
org.codehaus.mojo:exec-maven-plugin:1.2.1:exec

Copying that command from Netbeans and running it in the terminal is sufficient to produce the problem.

(here is a copy/paste friendly version. same as above, but not spaces or newlines)
cd /Users/ivory/Documents/Repos/integrated-genome-browser/main;
JAVA_HOME=/Library/Java/JavaVirtualMachines/jdk1.8.0_131.jdk/Contents/Home "/Applications/NetBeans/NetBeans 8.2.app/Contents/Resources/NetBeans/java/maven/bin/mvn" -Dexec.executable=/Library/Java/JavaVirtualMachines/jdk1.8.0_131.jdk/Contents/Home/bin/java -Dexec.mainClass=com.affymetrix.main.Main "-Dexec.args=-Xmx1g -DdevelopmentMode=true -Dapple.laf.useScreenMenuBar=true -Dsun.java2d.opengl=true -classpath %classpath com.affymetrix.main.Main" org.codehaus.mojo:exec-maven-plugin:1.2.1:exec

$ mvn clean install -DskipTests=true
$ java -jar igb_exe.jar
--> all is well

$cd main
$ mvn -Dexec.executable=/Library/Java/JavaVirtualMachines/jdk1.8.0_131.jdk/Contents/Home/bin/java -Dexec.mainClass=com.affymetrix.main.Main "-Dexec.args=-Xmx1g -DdevelopmentMode=true -Dapple.laf.useScreenMenuBar=true -Dsun.java2d.opengl=true -classpath %classpath com.affymetrix.main.Main" org.codehaus.mojo:exec-maven-plugin:1.2.1:exec
--> error, no apps

$ cd ..
$ java -jar igb_exe.jar
--> error, no apps

So calling mvn with those args is sufficient to produce the problem (without Netbeans)
---------------------------------------------

$ mvn clean install -DskipTests=true
$ java -jar igb_exe.jar
--> all is well

$ git checkout 5aaf78603354239371f5080ebdfa5ffbe418932f

$ mvn clean install -DskipTests=true
$ java -jar igb_exe.jar
--> all is well

$ cd main
$ mvn -Dexec.executable=/Library/Java/JavaVirtualMachines/jdk1.8.0_131.jdk/Contents/Home/bin/java -Dexec.mainClass=com.affymetrix.main.Main "-Dexec.args=-Xmx1g -DdevelopmentMode=true -Dapple.laf.useScreenMenuBar=true -Dsun.java2d.opengl=true -classpath %classpath com.affymetrix.main.Main" org.codehaus.mojo:exec-maven-plugin:1.2.1:exec
--> all is well

Therefore, on an older commit, that mvn command is NOT sufficient to produce the problem.

$ mvn clean install -DskipTests=true
$ java -jar igb_exe.jar
--> all is well
$ mvn -Dexec.executable="java" "-Dexec.args=-Xmx1g -Dapple.laf.useScreenMenuBar=true -Dsun.java2d.opengl=true -classpath %classpath com.affymetrix.main.Main" org.codehaus.mojo:exec-maven-plugin:1.2.1:exec
--> all is well (twice)
$ mvn -Dexec.executable="java" "-Dexec.args=-Xmx1g -DdevelopmentMode=true -Dapple.laf.useScreenMenuBar=true -Dsun.java2d.opengl=true -classpath %classpath com.affymetrix.main.Main" org.codehaus.mojo:exec-maven-plugin:1.2.1:exec
--> error, no apps

So it looks like the -DdevelopmentMode=true argument has to be included to produce the error.
Netbeans takes these arguments from nbactions.xml.
That argument was present when the nbactions.xml file was added in 2014.

I've had a hard time time finding anything to explain what these arguments do.
It looks like this sets a system property which can be querried programatically useing System.getProperty("developmentMode")
--assuming it is analogous to the 'CATALINA_OPTS' in this post: https://stackoverflow.com/questions/4907075/cross-server-environment-variable

What we know....

The problem requires the most recent commit, so it must have something to do with the samtools library upgrade.
The problem requires that the code with the upgrade is built.
The problem requires that the program is run with -DdevelopmentMode=true.
Whatever the problem is, it sticks around after the run is done. So subsequent runs are broken in the same way.
The problem is removed by mvn clean install.

Kiran found that apps/bundles are compared to all existing bundles, including all of IGB's internal modules.
It compares their "symbolic name" to see if the bundle is already installed.
One of those modules is returning null. The comparison brakes, with a null pointer exception.
This comparison is done with each time that you add/refresh an app repo.
Since the comparison fails, the whole process of displaying apps fails.

The new code upgrades a library. I think its called htslib.
Kiran found that the module that was returning null was placed between h and i in an alphabetical list.

The process of adding modules may be different in development mode to allow developers to remove and add bundles more quickly.

The new code builds a bundle that returns null for a name.

----------
According to this site, the symbolic name is required. So our assumption that all bundles would have it is not all that bad.
http://spring.io/blog/2008/02/18/creating-osgi-bundles/
But I haven't found anything else to support that, so it may just be a Spring requirement, not an OSGi requirement, and thus not a solid assumption.

I'm not sure what to expect to be different from Gradle bundles (like htslib).

I looked to see what bundles are available in different modes of running IGB. For future reference, I saved a file (DiffLoadedBundles.xls) describing which bundles get listed in different modes.

I added a filter for null values.
I also added a method to print a log statement when there are null values to alert developers. This is a separate method so that it does not have be processed in any of the many loops that are each called many times.

The fix is on this branch.
https://bitbucket.org/IvoryBlak/integrated-genome-browser/branch/IGBF-1366_App_Manager_shows_no_apps

I'm moving to first level review.

My testing verifies that this issue has been resolved. The App Manager now opens and displays all proper plugins. On a Mac OS, I ran IGB using the installer from main master, and the App Manager works properly. I also ran IGB with Netbeans, and the App Manager works properly. No error messages appear in the Netbeans IDE log upon startup, nor upon refreshing an app repository.

I tried to test if local files/repos will load and run in the App Manager, but it did not work. Ivory stated that it may not be testable until this fix for the App Manager is merged in.

I added comments in the code in response to Anns comments through bitbucket.

In reading the description of the test, I worry that the test was not thorough enough to see the problem state and verify the fix. It just sounds like it verifies the fix.
I think this should be reviewed by at least one of the other people who explored the problem in depth.

I am re-assigning to Sneha to do a second test.

An error detected:
After testing the application thrice, seems that uninstalling a plugin is not working when we run the application from netbeans. It goes to an unending loop with button saying "Uninstalling".
I also checked it by running the application through command prompt and the issue did not come up. Installing and uninstalling both worked fine.
However, The error which was appearing before about Null pointer exception when the bundle doesn't have a symbolic name in consoles of both command prompt and netbeans is not there anymore.

Moving back to ToDo column and Reassigning to Ivory Blakley to investigate and look through the code of uninstalling process.

I think the original issue had multiple symptoms. The core issue is that there is a bundle that has symbolic name: null. One symptom is that the app manager was not able to display apps. Another symptom is the inability to uninstall apps, but that symptom is un-detectable when you can't display apps in the first place.

I was not able to fix the core issue.
The core fix would be to add some step that gives a symbolic name to any bundle that didn't have one.
The installed bundled are of class BundleImpl
org/apache/felix/framework/BundleImpl.java
I don't see any way to change the symbolic name attribute.

Another core fix might be to change this bundle, maybe wrap it up as a new bundle that does have symbolic name even in developer mode. But as a rule we want to avoid modifying the bundles; better to use them as they are, so we can easily swap it out for an updated version.

Another core fix might just be for the htsjdk to be updated into a bundle that Does have a symbolic name (we should reach out to the developers to ask about this).

My current fix is not a core fix. It addressed the symptom by side-stepping the null pointer exception. Thus, fixing one symptom reveals another.

Please identify which commit introduces the problem.

The following test confirms that this is another symptom of the original core issue.

1. installer
   I used the installer from my branch (IGB-IGBF-1366_App_Manager_shows_no_apps.dmg).
   -->all is well -->IGB runs without any errors, and it can display, install and uninstall Apps.

$ mvn clean install -DskipTests=true

1. run simplest way
   $ java -jar igb_run.jar
   -->all is well
2. run using mvn NOT in developer mode
   $ mvn -Dexec.executable="java" "-Dexec.args=-Xmx1g -Dapple.laf.useScreenMenuBar=true -Dsun.java2d.opengl=true -classpath %classpath com.affymetrix.main.Main" org.codehaus.mojo:exec-maven-plugin:1.2.1:exec
   -->all is well
3. run using mvn in developer mode
   $ mvn -Dexec.executable="java" "-Dexec.args=-Xmx1g -DdevelopmentMode=true -Dapple.laf.useScreenMenuBar=true -Dsun.java2d.opengl=true -classpath %classpath com.affymetrix.main.Main" org.codehaus.mojo:exec-maven-plugin:1.2.1:exec
   -->Error: Apps displya, and install, BUT cannot uninstall.
4. run using mvn NOT in developer mode (identical to before, but now the state has changed because we have run it in dev mode)
   $ mvn -Dexec.executable="java" "-Dexec.args=-Xmx1g -Dapple.laf.useScreenMenuBar=true -Dsun.java2d.opengl=true -classpath %classpath com.affymetrix.main.Main" org.codehaus.mojo:exec-maven-plugin:1.2.1:exec
   -->Error: Apps display, and install, BUT cannot uninstall.

So even though this bug can only be seen on my branch, it was not caused by any of my changes. There is no error message so it may take slightly longer to track down the method that is throwing the error now.

Kiran and I reviewed the meta data files for several packages.
We need this file:
com/github/samtools/htsjdk/2.15.1/htsjdk-2.15.1.jar:META-INF/MANIFEST.MF
to have this line:
Bundle-SymbolicName: <symbolic name>
Bundle-Version: <version>

To test this, we cloned the samtools github repository, and modified the jar section in the file build.gradle.
Original:
jar {
manifest

}

With our modification:
jar {
manifest

}

Kiran built a jar, modified the jar name to match the existing jar file, and used that jar file in place of the current htsjdk jar.
IGB main master was able to build and run without errors.

We need to either:

• host this jar file ourselves, and make this same modification every time we update the library.
  OR
• maybe we can submit a pull request to com.github.samtools and they can add these two lines to their file so that version in their repo works in our system.

The version cheat:
In order to test the above, we had to cheat with the version number to make it match our current pom files.
The file has a line that defines the version:

• version = isRelease ? gitVersion : gitVersion + "-SNAPSHOT"

Since we can't run it in release mode (partly because I don't know how and partly because that activates their automated update process), we just hard-coded the version to match our current pom files:

• version = '2.15.1'

A real solution will have to address that a bit more elegantly.
____________________

The easy plan:
Kiran reached out to the samtools developers asking them to include the lines we need in their build. If they can add the lines we need to their build, then we don't have to do any customization on our end.

The DIY plan:
This is what we'll have to do if we don't hear back from the samtools developers, or if they say they don't want to add those lines, or it might take a long time.

We'll host the modified jar file ourselves.
To do this, we need to :

• make a fork of the htsjdk repo
• add a commit that adds the lines we need and adds this line for the version
  version = gitVersion
  'Bundle-SymbolicName': project.getGroup() + '.' + project.getName() + '-igb'
  'Bundle-Version': version
• build the jar file and add that to our repo
• modify the IGB pom files to use that version number

When we want to update the library, we would need to:

• rebase our fork based on the main htsjdk repo, applying our one commit on top.
• build the jar file and put it in our repo
• update the version number in our pom files.

Using +'-igb' ensures that IGB will pull our version of the jar file even if it has access to a repository that has the non-modified htsjdk version.
____________________

Kiran reached out to the samtools developers.
They do pretty frequent releases and they have been responsive before, so I have high hopes for the 'easy plan'.
If we haven't heard back from them by Tuesday (31 July 2018) then we'll move forward with the DIY plan.

In the mean time, we'll use our test jar file or installers to do the testing for issues that are affected by this.

Don't use their snapshot release. Use their most recent stable release in our project.

Kiran forked the htsjdk repo and made a commit with the changes we need and made a pull request.
See https://github.com/samtools/htsjdk/pull/1169
_____________________

I forked the htsjdk repo and made a branch called IGB_compatible.
https://github.com/IvoryC/htsjdk/tree/IGB_compatible
I pulled the commit that Kiran made.
I made a commit to add '-igb' to the project name and set the version to use the format it would use for a release.
I rebased this branch (with its 2 commits) back to the last stable release, which was commit 96ce8fb for version 2.16.0.
To check for new releases to repeat this, see: https://github.com/samtools/htsjdk/releases

On the IGB_compatible branch, I ran this command to build a jar file:
./gradlew jar
This produced a jar file called htsjdk-igb-2.16.0.jar

To test this, I copied this jar file to my .m2 directory and renamed it to match the version that the IGB pom files are expecting.
I can open the manefest file in netbeans and see that it has a symbolic name attribute.
On the IGB master branch, I ran $ mvn clean install and ran igb with dev mode = true.
-->all is well, Apps display, install and uninstall.
_______________________

Nest steps:

• We need to change the IGB pom files to take this jar file.
• We need to put this jar file in the repository so anyone who builds IGB can access it.

For this to be used as a bundle, it would need to include export statements for publicly exposed packages.
How is it working without these additional meta-data in the manifest file?

Requesting Kiran create Powerpoint case study describing problem and fix

The fix for the issue is in:

https://bitbucket.org/kkorey/kkorey-igb/branch/IGBF-1098-sam-so

It also has the code to IGBF-1371.

Comments on branch https://bitbucket.org/kkorey/kkorey-igb/branch/IGBF-1098-sam-so:

This is one commit that does several very separate things.
Most of the changes are what the commit message says "Removed references to older net.sf.samtools/sam-ext library." (which addresses issue IGBF-1371)
But the changes in some files are not related to that at all, and warrant their own commit with their own message.

Most notably, one change in core/shared-lib-wrapper/pom.xml alters how the htsjdk bundle is handled and addresses the problem discussed in IGBF-1366.
Although that change is very small, I think it warrants its own commit message.

Other potentially un-related changes include:

• In file core/quickload/pom.xml, added the scope of "provided" to the dependency org.lorainelab.igb (why was this change needed?)
• In the file optionalPlugins/geometric-mean/pom.xml, it looks like a line was previously just in the wrong place . Moving that line doesn't look like it has anything to do with removing the net.sf.samtools library. It is conceivable that its not just in the wrong place, it was there for a reason, and tracking down this 'fix' later when we find problems will be easier if the change is explained with its own commit. The commit message could be as simple as just 'formatting correction in pom file', and that is more informative than saying the change was made as part of removing the net.sf.samtools library. If this change has some functional motivation, then please explain that.

I think the easiest way to separate these changes into separate commits will be to make a new branch and re-produce these changes.
I am stopping my review now to allow for these updates.

Ivory Blakley Can you please review the changes in this branch
https://bitbucket.org/kkorey/kkorey-igb/branch/IGBF-1371-library

Much clearer!

It looks like the changes for the 1366 issue were removed, so I assume that will be another branch ...?
__________________

The commit for removing the net.sf.samtools is clearly just that.
And the formatting fixes are very simple and clearly just that.
So the code-review and the git review are good. : )

These changes are minor and only affect the pom file. We have already removed all of the import statements for the samtools library.
To make sure IGB is able to build without the net.sf.sam-exec library, I removed it from my .m2, and I cleared IGB's bundle directory, and I ran $mvn clean install.
IGB was able to build with tests, without errors, and run. The net.sf.sam-exec was not re-downloaded to .m2, so it looks like IGB is completely free of the dependency.
That's the only functional review I can think of for this, and it looks good. : )

__________________

I'm copying these comments to issue IGBF-1371.
Branch IGBF-1371-library has to do with that issue, and now has nothing to do with this issue (IGBF-1366).
I'm not going to make any changes to this issue (except to assign back to Kiran since he's doing the ppt, and making a branch for this issue).

I have merged in the changes for this issue as well in the same branch as 1371 as it was just removing an artifact.
Please do take a look at it once.

Reviewing the added commit:

git review: the message matches the change. The issue reference is for issue IGBF-1371 but IGBF-1366 would be a better explanation of why that change was made.

code review: short and sweet. good.

functional review: IGB builds and runs without error. In the app manager, Apps display, install, and uninstall.

This change is much better than working around the null pointer, and much better than modifying the jar file itself.

I am reassigning to Kiran to modify the commit message, and submit a pull request.
The issue will be moved back to in-progress while Kiran finishes the ppt associated with this issue. But IGBF-1371 uses the same branch and it is is "ready for pull request".

Please rebase on latest master & submit pull request.

This issue does not have any code changes. The code change is done as part of IGBF-1371 (Which is already mearged and moved to needs testing)
This is pending only because of the Case study document.