scijava / pom-scijava

Friendly base POM for all SciJava-based software
https://imagej.net/BOM
The Unlicense
24 stars 33 forks source link

bump n5 artifact and plugin versions #259

Closed bogovicj closed 6 months ago

bogovicj commented 6 months ago

bigwarp failed for a silly reason...on it.

not sure about SNT though.

bogovicj commented 6 months ago

Updates:

@cmhulbert has tracked down the issue with n5-imglib2 and is dealing with that right now.

I believe I fixed bigwarp, and think the remaining issues there stem from n5-imglib2.

SNT is different. I did some investigating, and don't believe the errors relate to the n5 version changes here. This might be interesting to @tferr

Conclusion - I'll wait on Caleb to deal with n5-imglib2. I expect that will be the last issue related to this PR, since from what I can tell, other failures are unrelated. But we will see and revisit when Caleb's done.

SNT

what I added to the SNT pom ``` 3.2.0 2.2.0 4.1.1 1.4.1 1.0.2 1.3.1 10.4.13 1.0.0-beta-34 ```
build failures with new java versions ``` [ERROR] Failed to execute goal org.apache.maven.plugins:maven-jar-plugin:3.2.0:jar (default-jar) on project SNT: Error assembling JAR: Invalid automatic module name: 'org.morphonets._' -> [Help 1] ```
ctrueden commented 6 months ago

I suggest to ignore the SNT failure. I may disable it in the mega-melt again, to work around this issue. It was only recently re-enabled, because I thought it would/could work, but clearly not.

cmhulbert commented 6 months ago

So the n5-imglib2 issue is unrelated from this PR. It seems it has been present ever since this commit: Update component versions.

I am looking into it, but I think for this PR, It's probably worth reverting this version bump: imglib2-label-multisets: 0.11.5 -> 0.12.0 which is what causes the test failure in n5-imglib2.

In the meantime, I'll work on a bug fix for imglib2-label-multisets and can have a separate PR to bump that to the fixed version

ctrueden commented 6 months ago

@cmhulbert Thanks for investigating. Downgrading imglib2-label-multisets to 0.11.5 sounds good to me.

bogovicj commented 6 months ago

I'll downgrade it in this PR and we can see what happens to the build.

ctrueden commented 6 months ago

It looks like everything passed except SNT and bigwarp. I have a commit locally to disable SNT again for now (it fails due to the fact that scijava-plugins-io-table is now obsolete and is no longer managed in pom-scijava). As for bigwarp, I'm not sure, but I'm going to merge this now and then run a melt locally to double check it.

bogovicj commented 6 months ago

@ctrueden , if bigwarp is still causing an issue (I think it might be?) This might address the problem: https://github.com/saalfeldlab/bigwarp/commit/3723ac77339302f57fb0a21f5066789e0d97dab0

tferr commented 6 months ago

SNT fails due to the fact that scijava-plugins-io-table is now obsolete and is no longer managed in pom-scijava

@ctrueden , I was not aware of it. Is https://github.com/scijava/scijava-table the new alternative? If so, I should be able to do the required changes.

On a related note: Since we depend on sciview, I assume SNT needs to be compiled also with Java21? Is that OK, or should we try to depend only on sciview at runtime?

ctrueden commented 6 months ago

@tferr The DefaultTableIOPlugin from scijava-plugins-io-table moved into scijava-table directly, yeah. So you shouldn't need to depend on anything new—just remove the scijava-plugins-io-table dependency.

On a related note: Since we depend on sciview, I assume SNT needs to be compiled also with Java21? Is that OK, or should we try to depend only on sciview at runtime?

I think it'll be simplest to just bump your scijava.jvm.version to 21.

Unfortunately, scenery's move to Java 21 leaves us in kind of a bind with respect to SNT. The pom-scijava "mega-melt" integration test right now is completely focused on Java 8. It is on my radar to make the mega-melt sensitive to the minimum Java version needed for each of the components it tests, but it does not do that yet. So for the moment, it cannot test that components requiring Java >8 still work when built and run with pom-scijava's preferred versions of all managed dependencies.

Anyway, I don't think any further action on your part is needed for the moment. You already did the hard work of updating SNT to the latest scenery & sciview, which is awesome.

As pom-scijava 38 gets closer to release, and we approach a corresponding new release of Fiji, I'll look more closely at SNT's current dependency situation and try to get it squared with the pom-scijava state. The way I'm currently leaning toward dealing with it is via a so-called "preemptive version bump" of SNT in pom-scijava: i.e. we would release pom-scijava 38.0.0 referencing a not-yet-released 5.0.0 version of SNT, and then you would update SNT's parent pom-scijava version to 38.0.0, and you can remove all the version pins from properties, and make sure everything builds and works well with that latest state, and then actually cut the 5.0.0 release. I often use this technique when releasing sc.fiji:fiji, and it works well—I just need to do the real testing before actually releasing the new pom-scijava. What I do is install a local-only 999 version of pom-scijava to my local repo cache, then test Fiji with that, iron out any issues, fix dependency issues as needed, repeating until all is fixed. Only then do I actually release the new pom-scijava and subsequently the new fiji extending it. We can just include SNT in this process as well this time.

Sorry for the long explanation; I hope it makes sense.

ctrueden commented 6 months ago

@bogovicj About bigwarp... the issue is just that we upgraded EJML and it changed groupIds. But the version of bigwarp being referenced in pom-scijava still depends on a too-old release of jitk-tps which depends on the old EJML. The proper fix would be to update bigwarp to depend on jitk-tps 3.0.4 as well as the 0.41 version of ejml. Here is a rough patch to bigwarp's pom.xml:

diff --git a/pom.xml b/pom.xml
index c256480..96f5236 100644
--- a/pom.xml
+++ b/pom.xml
@@ -123,7 +123,7 @@

        <jcommander.version>1.48</jcommander.version>
        <alphanumeric-comparator.version>1.4.1</alphanumeric-comparator.version>
-       <jitk-tps.version>3.0.3</jitk-tps.version>
+       <jitk-tps.version>3.0.4</jitk-tps.version>

        <!-- NB: Deploy releases to the SciJava Maven repository. -->
        <releaseProfiles>sign,deploy-to-scijava</releaseProfiles>
@@ -138,6 +138,7 @@
        <n5-ij.version>4.1.0</n5-ij.version>
        <n5-viewer_fiji.version>6.1.0</n5-viewer_fiji.version>

+       <ejml-all.version>0.41</ejml-all.version>
    </properties>

    <repositories>
@@ -309,6 +310,11 @@
            <groupId>gov.nist.math</groupId>
            <artifactId>jama</artifactId>
        </dependency>
+       <dependency>
+           <groupId>org.ejml</groupId>
+           <artifactId>ejml-all</artifactId>
+           <version>${ejml-all.version}</version>
+       </dependency>
        <dependency>
            <groupId>org.jdom</groupId>
            <artifactId>jdom2</artifactId>
@@ -343,7 +349,6 @@
            <artifactId>xmlunit</artifactId>
            <version>1.5</version>
        </dependency>
-
    </dependencies>

    <profiles>

(I say "rough" because properly, the dependency should not be on ejml-all, but on the individual artifacts containing the classes used directly in the bigwarp code.)

Unfortunately, bigwarp's code needs updating to accommodate this upgrade:

[ERROR] .../bigwarp/src/main/java/bigwarp/source/JacobianDeterminantRandomAccess.java:[24,21] cannot find symbol
  symbol:   class DenseMatrix64F
  location: package org.ejml.data
[ERROR] .../bigwarp/src/main/java/bigwarp/source/JacobianDeterminantRandomAccess.java:[25,20] cannot find symbol
  symbol:   class CommonOps
  location: package org.ejml.ops

Once the code is adjusted to work with the 0.41 version of EJML, then cut a new version of bigwarp_fiji, and bump the version in pom-scijava here, and the mega-melt should be fixed. Let me know if anything is unclear, or if you need any help with anything, or if you disagree with this as the best path forward.