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

Modularization #160

Merged
merged 5 commits into from
Jun 19, 2020
Merged

Modularization #160

merged 5 commits into from
Jun 19, 2020

Conversation

wirew0rm
Copy link
Member

@wirew0rm wirew0rm commented Apr 19, 2020

Efforts to make chartfx usable from jigsaw modularized projects as proposed in #156.

Before merging this the following points will have to be addressed:

Jigsaw Compatibility

To make chartfx usable for modularized projects, there are some points which have to be considered according to this blogpost.

Addressing these concerns is a matter of good hygiene. If you encounter one of these issues and you can't address those, don't add the Automatic-Module-Name entry yet. For now, your library is better off on the classpath. It will only raise false expectations if you do add the entry.

So before meging this branch, these issues should probably be adressed.

internal JDK types

Make sure your library doesn't use internal types from the JDK (run jdeps --jdk-internals mylibrary.jar to find offending code). JDeps (as bundled with Java 9 and later) will offer publicly supported alternatives for any use of encapsulated JDK APIs. When your library runs from the classpath on Java 9 and later, you can still get away with this. Not so if your library lives on the module path as automatic module.

jdeps --jdk-internals chartfx-chart/target/chartfx-chart-11.1.0-SNAPSHOT.jar 
chartfx-chart-11.1.0-SNAPSHOT.jar -> JDK removed internal API
   de.gsi.chart.utils.SimplePerformanceMeter          -> com.sun.javafx.perf.PerformanceTracker             JDK internal API (JDK removed internal API)

jdeps --jdk-internals chartfx-dataset/target/chartfx-dataset-11.1.0-SNAPSHOT.jar 
chartfx-dataset-11.1.0-SNAPSHOT.jar -> jdk.unsupported
   de.gsi.dataset.serializer.spi.FastByteBuffer       -> sun.misc.Unsafe                                    JDK internal API (jdk.unsupported)

JDK Internal API                         Suggested Replacement
----------------                         ---------------------
sun.misc.Unsafe                          See http://openjdk.java.net/jeps/260

Unsafe replacement links to Variable Handles.

  • com.sun.javafx.perf.PerformanceTracker (chartfx-chart: SimplePerformanceMeter)
  • sun.misc.Unsafe (chartfx-datasets: FastByteBuffer)

unnamed packages

Your library can't have classes in the default (unnamed) package. This is a bad idea regardless, but when your library is used as automatic module, this rule is enforced by the module system.

  • no problems here

split packages

Your library can't split packages (two or more JARs defining the types in the same package), nor can it redefine JDK packages (javax.annotation is a notorious example, being defined in the JDK's java.xml.ws.annotation module but also in external libraries).

Are our testcases affecting this? They are in the same package as the implementation but they are not shipped, so it might be ok?

META-INF/services

When your library's JAR has a META-INF/services directory to specify service providers, then the specified providers must exist in this JAR (as described in the ModuleFinder JavaDoc)

  • check

@codecov
Copy link

codecov bot commented Apr 19, 2020

Codecov Report

Merging #160 into dev-x.2.0 will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##             dev-x.2.0     #160      +/-   ##
===============================================
+ Coverage        45.48%   45.49%   +0.01%     
- Complexity        6273     6277       +4     
===============================================
  Files              372      372              
  Lines            40140    40137       -3     
  Branches          6415     6415              
===============================================
+ Hits             18257    18262       +5     
+ Misses           20711    20707       -4     
+ Partials          1172     1168       -4     
Impacted Files Coverage Δ Complexity Δ
...ava/de/gsi/chart/utils/SimplePerformanceMeter.java 89.70% <ø> (-0.23%) 37.00 <0.00> (ø)
.../main/java/de/gsi/chart/axes/spi/AbstractAxis.java 63.76% <0.00%> (+0.13%) 145.00% <0.00%> (+1.00%)
...src/main/java/de/gsi/chart/ui/ProfilerInfoBox.java 99.02% <0.00%> (+1.94%) 24.00% <0.00%> (+1.00%)
...ain/java/de/gsi/dataset/utils/CacheCollection.java 91.30% <0.00%> (+4.34%) 19.00% <0.00%> (+1.00%)
...in/java/de/gsi/dataset/utils/DoubleArrayCache.java 100.00% <0.00%> (+8.00%) 13.00% <0.00%> (+1.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24244b8...a72c406. Read the comment docs.

@ennerf
Copy link
Collaborator

ennerf commented Apr 19, 2020

Regarding the compatibility questions:

Split Packages
Test cases don't matter

Internal JDK Types
sun.misc.unsafe is still available for the time being, so it's probably not necessary to replace all references just to get the automatic module name in.

JEP 193: Variable Handles only replaces some of the on-heap use cases Unsafe. In case you're doing anything off-heap, you may need to take a look at the new JEP 370: Foreign-Memory Access API.

@RalphSteinhagen RalphSteinhagen mentioned this pull request May 11, 2020
14 tasks
RalphSteinhagen added a commit that referenced this pull request May 20, 2020
The ProfilerInfoBox is intended to be placed into the ToolBar and
provides a quick indication of FX pulse and actual scene frame rates,
process and system cpu loads, as well as JDK/JavaFx and (soon) Chart-fx
versioning information.

N.B. updates are in view of modularising Chart-fx (see also PR #160)
Interface to 'com.sun' package interface may become deprecated and/or
may cause inconveniences for users that have not yet fully moved to
Jigsaw-type modularisation. To be revisited for the 'x.2.y' release.
RalphSteinhagen added a commit that referenced this pull request May 20, 2020
The ProfilerInfoBox is intended to be placed into the ToolBar and
provides a quick indication of FX pulse and actual scene frame rates,
process and system cpu loads, as well as JDK/JavaFx and (soon) Chart-fx
versioning information.

N.B. updates are in view of modularising Chart-fx (see also PR #160)
Interface to 'com.sun' package interface may become deprecated and/or
may cause inconveniences for users that have not yet fully moved to
Jigsaw-type modularisation. To be revisited for the 'x.2.y' release.
@wirew0rm wirew0rm added this to the chartfx x.2.0 milestone May 26, 2020
@wirew0rm wirew0rm changed the base branch from master to dev-x.2.0 June 15, 2020 15:26
@wirew0rm wirew0rm changed the title Add auto module name to MANIFEST.MF Modularization Jun 15, 2020
@wirew0rm
Copy link
Member Author

We decided to target this for the next major release, so rebased against dev-x.2.0.

Also tried to go fully modularized. This removes the need for our custom launcher, but we still need some jvm arguments, mostly due to controlsfx using internal javafx api via reflection.

The serialisers also rely on reflection, so there are still some exports missing.
Also correctly structuring the tests is difficult, as they cannot be in a different module, because they are in the same package but we cannot require the test dependencies from the module-info because they are only in the maven testscope. Surefire seems to do some magic here by supplying some additional jvm parameters, but at least in combination with testFX it does not work out of the box.

On the bright side, we can now start some of our samles without any additional configuration or wrappers.

Adds the `Automatic-Module-Name: <module name>` to the
`MANIFEST.MF`which gets packaged into the jar file. The artifactId is
used as `<module-name>`.
This is a first step towards making chartfx usable for projects using
the jigsaw module system as proposed in #156 by @ennerf.
@wirew0rm wirew0rm force-pushed the akrimm-auto-module-name branch 4 times, most recently from 3f6440c to efef8c6 Compare June 17, 2020 16:59
@wirew0rm
Copy link
Member Author

Projects missing auto-moudule-name in manifest:

[WARNING] Can't extract module name from batik-script-1.12.jar: Provider class org.apache.batik.bridge.RhinoInterpreterFactory not in module
[WARNING] Can't extract module name from xalan-2.7.2.jar: Provider class org.apache.bsf.BSFManager not in module
[WARNING] * Required filename-based automodules detected: [JTransforms-3.1.jar, commons-math3-3.6.1.jar, javafxsvg-1.3.0.jar, pngj-2.1.0.jar]. Please don't publish this project to a public artifact repository! *

@wirew0rm wirew0rm marked this pull request as ready for review June 19, 2020 08:55
@RalphSteinhagen RalphSteinhagen self-requested a review June 19, 2020 09:12
Copy link
Member

@RalphSteinhagen RalphSteinhagen left a comment

Choose a reason for hiding this comment

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

OK

@RalphSteinhagen RalphSteinhagen merged commit b6bb5bf into dev-x.2.0 Jun 19, 2020
@RalphSteinhagen RalphSteinhagen deleted the akrimm-auto-module-name branch June 19, 2020 09:15
RalphSteinhagen pushed a commit that referenced this pull request Sep 10, 2020
* Add auto module name to manifest

Adds the `Automatic-Module-Name: <module name>` to the
`MANIFEST.MF`which gets packaged into the jar file. The artifactId is
used as `<module-name>`.
This is a first step towards making chartfx usable for projects using
the jigsaw module system as proposed in #156 by @ennerf.

* Document steps to be taken to support jigsaw

* make module name reverse dns

* Remove PerformanceTracker internal api

* remove jigsaw todo list
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.

None yet

3 participants