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

Update to latest dependency versions #181

Merged
merged 10 commits into from
Jul 25, 2023
Merged

Update to latest dependency versions #181

merged 10 commits into from
Jul 25, 2023

Conversation

ctrueden
Copy link

Hi @tferr 👋

This PR upgrades SNT to work with pom-scijava 36, so that the Neuroanatomy site can become functional again with the new Fiji 2.14.0 that was uploaded on Monday.

The scenery+sciview team (CC @kephale @skalarproduktraum) is working on getting new releases of scenery+sciview completed and available on an update site, but this change paves the way for SNT to work with those forthcoming releases as well.

The main concern I have with this change is the required bump to Java 11 as minimum, since scenery+sciview updated to Java 11. One possible way around that would be to encapsulate all the sciview+scenery stuff to a separate plugin-style component, which SNT could then consume in a way that does not reference any scenery or sciview classes. But of course that would be additional work.

So I'm wondering, what is your preference: A) update SNT to require Java 11 now? or B) keep support for Java 8 for the moment by encapsulating the scenery+sciview dependencies into a separate component?

If (A), you should be able to simply merge this PR.

Or if (B), I would scrutinize the code more closely and make an attempt at the encapsulation.

* Remove no-longer-needed exclusions.
* And update the versions of scenery+sciview.
* And therefore update the build to Java 11+.
Accomplished with help from:
  mvn dependency:analyze
Accomplished via:
  mvn license:update-project-license
And update the versions of scenery + sciview.

And make changes to account for API breakages in ImgLib2 and sciview.
These tests take an incredibly long time (maybe forever?) to run,
possibly due to a bug in ImgLib2's LoopBuilder. I am not sure, but for
now, in order to continue unit testing SNT, we need to switch them off.
@tferr
Copy link
Member

tferr commented Jul 14, 2023

Thanks a lot @ctrueden!
(and @kephale @skalarproduktraum: I saw news of the sciview Hackathon, super cool!!)

Adopting Java 11 would not be a problem as long as it is easy for non-developers to update the JRE. In my experience, most (if not all) SNT users just use the JRE inside Fiji.app.

  • Is there a streamlined way for folks to update it?
  • I noticed that Fiji 2.14.0 is still bundled with JDK 8[1]. Are there any plans to bundle it with java 11?

Encapsulation would be great, but I fear it will hijack time nobody has, We need to move away from Java 8 anyways, so let's get moving! We could do something like this: We release a last-of-the-line Java 8 version for SNT. There is currently a notification popup that is displayed when the user starts up a new version:
image
We could tap into it to display a warning of sorts "Newer versions require Java 11, please follow the instructions at https://imagej.net/java11 to migrate your installation", but it would be important to have such page functional, or better yet Fiji's pre-bundled with java11.

[1] I looks like it is not even the latest 8u372b07 bundle. I'm guessing it is not trivial to update the JRE!?

(NB: I am currently on PTO and may be a bit slower to react to this thread)

pom.xml Outdated
<optional>true</optional>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
<scope>test</scope>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is overriding the runtime scope and causes an error. @ctrueden and I found that removing this dependency got SNT and sciview (main branch) playing nicely together.

@ctrueden
Copy link
Author

@kephale Looking more closely at this, the reason I changed it to test scope in 28e6380 is because indeed SNT only uses the org.slf4j:slf4j-api in test classes, and mvn dependency:analyze recommended to declare it at test scope, but unfortunately with Maven when you do this, it narrows the scope, which was transitively at compile, so that was a bad decision on my part. 😅 I have revised this PR not to declare slf4j-api, in favor of a comment explaining why it is undeclared.

@ctrueden
Copy link
Author

@tferr About updating to Java 11: I think this is finally in the cards very soon, see also scenerygraphics/sciview#520.

  • I'm planning to dead-end the Java-8 version of Fiji in favor of a new Java 11 version.
  • We will start shipping sciview (and BigStitcher and the BIG-EPFL plugins again) with Fiji. If you'd like SNT included in Fiji as well, this would be a great time for us to make that happen. Or keeping it on its own update site would also be fine, if you prefer.
  • For people who really want to upgrade their existing Fiji, we can make it possible by providing some instructions to follow, TBD.
  • I have also made a lot of progress on a Maven-driven db.xml.gz generator, which I am hopeful will replace the need to ever manually upload things via the Updater, and be more robust about supporting all previous versions and even downgrades should people need that.

@tferr
Copy link
Member

tferr commented Jul 24, 2023

@ctrueden, Including SNT in Fiji now makes lots of sense, and the timing is perfect:

Pros Cons
Streamlined usage for both Fiji and pyimagej users Jzy3D dependency is (used to be!?) problematic!?
Streamlined sciview integration Something else I'm missing?
SNT depends on many Fiji components (IJ, Ops, Imglib2, script editor, etc.) at compile time
SNT depends on several Fiji components (LUTs, local thickness, Weka (TWS), etc.) at run time
Minimal breaking changes expected. Current development is focused on maintenance
Project is mature and popular

@carshadi, @kephale: do you agree? The neuroanatomy update site could still be around for serving more experimental stuff like cx3d. If you do agree, then we could release a "clean" end-of-the-line release for java-8 (i think there are some bug fixes/improvements in main that have not been released yet). Then I would go ahead and merge this

tferr added a commit that referenced this pull request Jul 25, 2023
@tferr tferr merged commit 13ae87b into main Jul 25, 2023
@ctrueden ctrueden deleted the psj36 branch July 25, 2023 03:23
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.

3 participants