Skip to content
This repository has been archived by the owner on May 23, 2023. It is now read-only.

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

Merged
merged 1 commit into from
Apr 19, 2018

Conversation

sjoerdtalsma
Copy link
Contributor

@sjoerdtalsma sjoerdtalsma commented Feb 7, 2018

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:

  • io.opentracing.api
  • io.opentracing.noop
  • io.opentracing.util
  • io.opentracing.mock
  • io.opentracing.util.tests

In preparation for Java 9 jigsaw modularization.
This effectively only 'claims' the name for the automatic
modules:
- io.opentracing.api
- io.opentracing.noop
- io.opentracing.util
- io.opentracing.mock
- io.opentracing.util.tests
@coveralls
Copy link

Coverage Status

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

@sjoerdtalsma
Copy link
Contributor Author

@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
Copy link
Member

cc @objectiser @jpkrohling

@jpkrohling
Copy link
Contributor

jpkrohling commented Apr 16, 2018

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

@gunnarmorling
Copy link

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
Copy link
Contributor

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
Copy link
Contributor Author

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

@sjoerdtalsma
Copy link
Contributor Author

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

  • Packages in module io.opentracing.api:
    • io.opentracing
    • io.opentracing.log
    • io.opentracing.propagation
    • io.opentracing.tag
  • Packages in module io.opentracing.noop:
    • io.opentracing.noop
  • Packages in module io.opentracing.util:
    • io.opentracing.util
  • Packages in module io.opentracing.mock:
    • io.opentracing.mock

@kevinearls
Copy link

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
Copy link
Contributor

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

@jpkrohling jpkrohling merged commit c187d9d into opentracing:master Apr 19, 2018
@sjoerdtalsma sjoerdtalsma deleted the automatic-module-names branch April 19, 2018 18:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants