osiegmar / FastCSV

CSV library for Java that is fast, RFC-compliant and dependency-free.
https://fastcsv.org/
MIT License
542 stars 93 forks source link

Supplemented JAR manifest with headers required to use library in OSGi environment #89

Closed ideas-into-software closed 10 months ago

ideas-into-software commented 11 months ago

Proposed Changes

Currently released version of FastCSV library cannot be used in OSGi environment due to lack of required manifest headers - i.e. MANIFEST.MF having only the following:

Manifest-Version: 1.0
Automatic-Module-Name: de.siegmar.fastcsv

Therefore, biz.aQute.bnd.builder gradle plugin was introduced into the project with most basic configuration, and resulting JAR now has all required headers - i.e. MANIFEST.MF contains the following:

Manifest-Version: 1.0
Bnd-LastModified: 1696783287923
Bundle-ManifestVersion: 2
Bundle-Name: de.siegmar.fastcsv
Bundle-SymbolicName: de.siegmar.fastcsv
Bundle-Version: 3.0.0.SNAPSHOT
Created-By: 1.8.0_362 (Amazon.com Inc.)
Export-Package: de.siegmar.fastcsv.reader;version="3.0.0",de.siegmar.f
 astcsv.util;version="3.0.0",de.siegmar.fastcsv.writer;version="3.0.0"
Import-Package: de.siegmar.fastcsv.util,java.io,java.lang,java.lang.in
 voke,java.nio,java.nio.channels,java.nio.charset,java.nio.file,java.u
 til,java.util.concurrent.atomic,java.util.function,java.util.stream
Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=11))"
Tool: Bnd-6.4.0.202211291949

Merging this PR will enable us and others who develop OSGi apps to use FastCSV library out-of-the box (i.e. without additional manual steps, so called wrapping https://bnd.bndtools.org/chapters/390-wrapping.html).

Performance

(output of lib/build/results/jmh/results.txt)

Benchmark                     Mode  Cnt        Score        Error  Units
FastCsvReadBenchmark.read    thrpt   10  4080920.655 ± 231511.881  ops/s
FastCsvWriteBenchmark.write  thrpt   10  4540875.096 ±  97908.858  ops/s
osiegmar commented 11 months ago

Thanks for your contribution! I wonder if there's enough interest from the FastCSV user base that justifies the effort of supporting and maintaining this. I can't provide any resources to support OSGI related questions or tasks.

ideas-into-software commented 10 months ago

@osiegmar There is nothing to support beyond what's already done / part of this commit. The biz.aQute.bnd.builder plugin and the minor additional config in jar task (https://github.com/osiegmar/FastCSV/pull/89/commits/651a85550994b18b9b715b35dfd1424b0f76b9e9) take care of all that is needed.

You will not need to support anything / no additional work on your part will be needed - this takes care of itself.

osiegmar commented 10 months ago

Every tiny bit needs care!

Build warnings

The build plugin you added throws this build warning:

The AbstractTask.getConvention() method has been deprecated. This is scheduled to be removed in Gradle 9.0. Consult the upgrading guide for further information: https://docs.gradle.org/8.4/userguide/upgrading_version_8.html#deprecated_access_to_conventions

You need to update the build plugin to version 7.0.0. How to verify that after this plugin update everything still works? There's no automatic test for that.

OSGi manifest

The OSGi manifest also exports the package de.siegmar.fastcsv.util. I doubt that this is correct. This package is not exported as an JPMS module. Again: how to verify that the removal of this package doesn't break anything?

Support

What if someone rises an OSGi related issue/question? Who will take care of that?

ideas-into-software commented 10 months ago
  1. I actually started with version '7.0.0' of biz.aQute.bnd.builder plugin, however, I had to downgrade as it requires much higher Java version that currently used in the project - i.e. Java 17:
FAILURE: Build failed with an exception.

* What went wrong:
java.lang.UnsupportedClassVersionError: aQute/bnd/gradle/BndBuilderPlugin has been compiled by a more recent version of the Java Runtime (class file version 61.0), this version of the Java Runtime only recognizes class file versions up to 57.0
> aQute/bnd/gradle/BndBuilderPlugin has been compiled by a more recent version of the Java Runtime (class file version 61.0), this version of the Java Runtime only recognizes class file versions up to 57.0

Screenshot from 2023-10-23 23-13-52

Would you be OK to live with this warning you mentioned, until you move project to Java 17, or should I go ahead and upgrade your project to Java 17 then? Warning cannot be even seen without all warnings turned on..

Screenshot from 2023-10-24 00-34-05

There is no intermediate version of biz.aQute.bnd.builder plugin, i.e. there's 6.4.0, which allows to use same Java version as currently used in the project, and next available version of this plugin is 7.0.0, which requires Java 17.

Screenshot from 2023-10-23 23-17-20

  1. I disabled de.siegmar.fastcsv.util package from being exported, so now generated JAR contains the following manifest:
Manifest-Version: 1.0
Bnd-LastModified: 1698100386510
Bundle-ManifestVersion: 2
Bundle-Name: de.siegmar.fastcsv
Bundle-SymbolicName: de.siegmar.fastcsv
Bundle-Version: 3.0.0.SNAPSHOT
Created-By: 1.8.0_362 (Amazon.com Inc.)
Export-Package: de.siegmar.fastcsv.reader;version="3.0.0",de.siegmar.f
 astcsv.writer;version="3.0.0"
Import-Package: java.io,java.lang,java.lang.invoke,java.nio,java.nio.c
 hannels,java.nio.charset,java.nio.file,java.util,java.util.concurrent
 .atomic,java.util.function,java.util.stream
Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=11))"
Tool: Bnd-6.4.0.202211291949

By default, biz.aQute.bnd.builder plugin, if there are no OSGi annotations (org.osgi.annotation.bundle.Export, org.osgi.annotation.versioning.Version) present in package-info files - and theren't any currently, and I did not want add any additional dependencies - will export everything. It does not look at module-info file. I added this exclusion now (see recent commit), so now public API is only via de.siegmar.fastcsv.reader and de.siegmar.fastcsv.writer packages.

  1. Those who would like to make use of FastCSV in OSGi environment will surely be grateful this can be done out-of-the box, without doing these steps manually prior to being able to use it, having most recent version already available with required OSGi headers.

I cannot imagine why they would bother you about OSGi part of your project once this is done / merged with trunk. However, you can ping me then / ping @juergen-albert (Juergen Albert, Data in Motion), or simply refer them to documentation: https://github.com/bndtools/bnd/blob/master/gradle-plugins/README.md, https://bnd.bndtools.org/.

Does that address your concerns?

osiegmar commented 10 months ago

Thanks for your comments.

juergen-albert commented 10 months ago

@osiegmar What you ask for should be no Problem. I suggest to go a bit further, as it might give you some extra value:

As already mentioned by @ideas-into-software we have a bunch of build time annotations (no extra requirement at runtime) you can use to define what the outside world can see in a modular environement. So, with this, there is no need to modify any build files when you e.g. rename or remove a package as the truth is in the code. We can additionally also instruct the bnd plugin to create a proper module-info for you, as JPMS as an OSGi like requires similar data then OSGi does.

As the Annotations are part of an official Specification you can also assume that this will be supported and not change in the future.

osiegmar commented 10 months ago

Sounds interesting, @juergen-albert. Could you open a separate PR for that in order to see and compare?

ideas-into-software commented 10 months ago

@osiegmar Please clarify your preference, i.e. should I go ahead with both what you (https://github.com/osiegmar/FastCSV/pull/89#issuecomment-1776925442) and @juergen-albert (https://github.com/osiegmar/FastCSV/pull/89#issuecomment-1777180298) mentioned ?

@juergen-albert I presume you're busy and I should take care of both ? If so, is it OK I can supplement this PR @osiegmar, instead of opening new PR ?

osiegmar commented 10 months ago

Right now I cannot express a preference as I haven't seen an annotation based implementation. That's why I asked for a separat PR in order to compare the two solutions.

juergen-albert commented 10 months ago

working on it

ideas-into-software commented 10 months ago

@osiegmar

Most recent commit contains whitelist instead of blacklist for public API packages and biz.aQute.bnd.builder upgraded to 7.0.0.

Thanks for your comments.

  • The Java 17+ dependency is for build-time only, so this is not an issue since the develop branch is already built with Java 20 and will be built with 21 when Gradle 8.5 is ready.
  • I'd prefer rather a package whitelist (de.siegmar.fastcsv.reader + de.siegmar.fastcsv.writer) than a blacklist. Could you please align the configuration accordingly?
osiegmar commented 10 months ago

Thanks! Just merged #92