search

opentracing / opentracing-java

OpenTracing API for Java. 🛑 This library is DEPRECATED! https://github.com/opentracing/specification/issues/163
http://opentracing.io
Apache License 2.0
1.68k stars 344 forks source link

Add "Automatic-Module-Name" entries to MANIFEST.MF. #254

Closed sjoerdtalsma closed 6 years ago

sjoerdtalsma commented 6 years ago

In preparation for Java 9 jigsaw modularization.

Currently, trying to release a java 9 modularized library that requires the opentracing API will use an "Automatic Module" in Jigsaw. However, depending on this module will generate warnings, as its name is not reserved yet (and based off the jar name e.g. without the io.opentracing prefix).

This blog post describes why it is a good idea to start providing Automatic-Module-Name entries in MANIFEST.MF files for libraries, even if they're not yet fully modularized.

This effectively only 'claims' the name for the automatic modules:

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 81.515% when pulling c04aa4d76ae13f236505fe64ce69f52a8e839c9b on sjoerdtalsma:automatic-module-names into 242ba956368c48b804ae4120053955a49f7f9c63 on opentracing:master.

sjoerdtalsma commented 6 years ago

@tedsuo I cannot seem to request reviewers myself and I see that you have automated this for new PR's. Could you request a review for me on this PR?

yurishkuro commented 6 years ago

cc @objectiser @jpkrohling

jpkrohling commented 6 years ago

I need some time to review this, as I'm really not up to date with the current state of JPMS.

gunnarmorling commented 6 years ago

Hey, the automatic module names look good IMO.

That said, adding the headers kind of suggests to users that using the JARs as auto modules is endorsed by the library authors, so I'd recommend to do at least some basic testing whether the OpenTracing JARs can actually be used as automatic modules. This should make sure you don't full upon the most basic potential problems such as split packages between multiple modules etc.

jpkrohling commented 6 years ago

Thanks a lot, @gunnarmorling! Your expertise is much appreciated!

@sjoerdtalsma, how soon do you need this merged? Depending on your requirements, we could try to get @kevinearls to test this and make sure our basic examples will still work with this PR on Java 9.

sjoerdtalsma commented 6 years ago

@jpkrohling Seeing the PR is already over two months old, my priority would be for correctness over speed.

sjoerdtalsma commented 6 years ago

With regards to package names: currently we seem to be ok:

kevinearls commented 6 years ago

I did the following today:

  1. Built opentracing-api 0.31.0-SNAPSHOT from this PR
  2. Ran a modified version of the tests for https://github.com/jaegertracing/jaeger-openshift using jdk9. This is a set of smoke tests that run first using the Jaeger all-in-one image, then with the production Jaeger images with Cassandra and then with ElasticSearch.
  3. Ran a modified version of the Jaeger performance tests from https://github.com/jaegertracing/jaeger-performance with jdk9.

All tests were successful. Please let me know if there's anything else needed here.

jpkrohling commented 6 years ago

Thanks @kevinearls. I think it's time to merge it and see how it reacts in the real world.