JPMS compatibility#619
Conversation
skalarproduktraum
added
enhancement
skalarproduktraum
force-pushed
the
enhancement/jpms-compatibility
branch
from
bb76fc3 to
7e301c2
Compare
skalarproduktraum
force-pushed
the
enhancement/jpms-compatibility
branch
from
2fdd947 to
cc84355
Compare
| api("sc.fiji:bigdataviewer-core:10.4.14") | ||
| api("sc.fiji:bigdataviewer-vistools:1.0.0-beta-28") | ||
| api("sc.fiji:bigvolumeviewer:0.3.3") { | ||
| exclude("sc.fiji", "bigdataviewer-core") |
There was a problem hiding this comment.
I would suggest to add a comment explaining what it accomplishes to exclude bigdataviewer-core and bigdataviewer-vistools here, when they are included as direct dependencies immediately above.
| @@ -165,6 +170,10 @@ tasks { | |||
| withType<JavaCompile>().all { | |||
| targetCompatibility = project.properties["jvmTarget"]?.toString() ?: "21" | |||
| sourceCompatibility = project.properties["jvmTarget"]?.toString() ?: "21" | |||
There was a problem hiding this comment.
Not a thing with this PR specifically, but maybe rather than setting -target and source, you want to configure the --release?
options.compilerArgs.add("--release")
options.compilerArgs.add(project.properties["jvmTarget"]?.toString() ?: "21")Why? Because.
| exports graphics.scenery.textures; | ||
| exports graphics.scenery.ui; | ||
| exports graphics.scenery.utils; | ||
| exports graphics.scenery.volumes; |
There was a problem hiding this comment.
If I'm reading it right, it looks like this is exporting all the graphics.scenery.* packages, but none of the graphics.scenery.*.* ones? Specifically: missing are attribute.geometry, attribute.material, attribute.renderable, attribute.spatial, backends.software, backends.vulkan, controls.behaviours, controls.eyetracking, utils.extensions. Intentional, or no?
And are you sure you want to expose so much API surface? I guess since scenery hasn't hit 1.0.0 yet, you can still break things. But this work is an opportunity to consider whether it makes sense to create any internal (non-exported) packages, no?
| //import org.janelia.saalfeldlab.n5.GzipCompression | ||
| //import org.janelia.saalfeldlab.n5.N5FSReader | ||
| //import org.janelia.saalfeldlab.n5.N5FSWriter | ||
| //import org.janelia.saalfeldlab.n5.imglib2.N5Utils |
There was a problem hiding this comment.
So, scenery only uses (used) n5 in tests? Please let me know if you find a good way of dealing with test-scope dependencies with JPMS. We have found it very challenging with SciJava Ops; we were unable to make a separate module-info.java in the src/test subtree work (see scijava/scijava@ae39206 for details).
2 participants
This PR improves compatibility with the JPMS, removing the need for
--add-opensVM flags when running with Java 17+.module-info.javais added, which includes thejdk.unsupportedpackage, enabling access toUnsafe