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

bump n5 artifact and plugin versions #259

Merged
merged 3 commits into from
Mar 8, 2024
Merged

Conversation

bogovicj
Copy link
Contributor

@bogovicj bogovicj commented Mar 6, 2024

No description provided.

@bogovicj
Copy link
Contributor Author

bogovicj commented Mar 6, 2024

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

not sure about SNT though.

@bogovicj
Copy link
Contributor Author

bogovicj commented Mar 8, 2024

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

  • I checked out SNT-4.2.1
  • I pinned the n5 versions to those in this PR (see below)
  • I built SNT
    • aside I tried with java 19, 17, and 11, but got a module related failure (see below)
what I added to the SNT pom
<n5.version>3.2.0</n5.version>
<n5-hdf5.version>2.2.0</n5-hdf5.version>
<n5-ij.version>4.1.1</n5-ij.version>
<n5-universe.version>1.4.1</n5-universe.version>
<n5-zstandard.version>1.0.2</n5-zstandard.version>
<n5-zarr.version>1.3.1</n5-zarr.version>
<bigdataviewer-core.version>10.4.13</bigdataviewer-core.version>
<bigdataviewer-vistools.version>1.0.0-beta-34</bigdataviewer-vistools.version>
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
Copy link
Member

ctrueden commented Mar 8, 2024

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
Copy link
Contributor

cmhulbert commented Mar 8, 2024

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
Copy link
Member

ctrueden commented Mar 8, 2024

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

@bogovicj
Copy link
Contributor Author

bogovicj commented Mar 8, 2024

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

@ctrueden
Copy link
Member

ctrueden commented Mar 8, 2024

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.

@ctrueden ctrueden merged commit 769b9b9 into scijava:master Mar 8, 2024
1 check failed
@bogovicj
Copy link
Contributor Author

bogovicj commented Mar 8, 2024

@ctrueden , if bigwarp is still causing an issue (I think it might be?)
This might address the problem:
saalfeldlab/bigwarp@3723ac7

@tferr
Copy link
Contributor

tferr commented Mar 8, 2024

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
Copy link
Member

ctrueden commented Mar 8, 2024

@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
Copy link
Member

ctrueden commented Mar 8, 2024

@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.

tferr added a commit to morphonets/SNT that referenced this pull request Mar 9, 2024
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.

4 participants