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

Fixes #367: generate a JPMS module descriptor #370

Merged
merged 1 commit into from Aug 17, 2023

Conversation

ppkarwasz
Copy link
Contributor

@ppkarwasz ppkarwasz commented Nov 15, 2022

Until now jctools-core used the Apache Felix Maven Bundle Plugin to generate its OSGI descriptor.

This PR:

  • moves the internal classes to the org.jctools.util.internal. If needed this package can be exported to other JCTools artifacts using the @ExportTo annotation from BND.
  • exports only explicitly annotated packages (i.e. all except org.jctools.util.internal). The OSGI descriptor becomes:
Export-Package: 
 org.jctools.queues.atomic;version="4.0.2";uses:="org.jctools.queues",
 org.jctools.queues.unpadded;version="4.0.2";uses:="org.jctools.queues",
 org.jctools.queues;version="4.0.2",
 org.jctools.maps;version="4.0.2",
 org.jctools.util;version="4.0.2",
 org.jctools.counters;version="4.0.2"
Import-Package: sun.misc;resolution:=optional
Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=1.8))"
  • automatically generates a module-info.class JPMS descriptor, which is in sync (cf. JPMS libraries) with the OSGI descriptor:
module org.jctools.core@4.0.2.SNAPSHOT {
  requires java.base;
  requires static jdk.unsupported;
  exports org.jctools.queues;
  exports org.jctools.queues.atomic;
  exports org.jctools.queues.unpadded;
  exports org.jctools.maps;
  exports org.jctools.util;
  exports org.jctools.counters;
}

This descriptor can be generated by JDK 8.

@franz1981
Copy link
Collaborator

@nitsanw wdyt?

@jponge this can help for Mutiny integration?

@jponge
Copy link

jponge commented Jul 26, 2023

That'd be good, although I think in JPMS land you should not append a version but just use org.jctools.core

@nitsanw
Copy link
Contributor

nitsanw commented Jul 26, 2023

Can you please remove the spaces/tabs commit?

@nitsanw
Copy link
Contributor

nitsanw commented Jul 26, 2023

I'm confused about the value of moving the internal classes to internal, can you elaborate?

@nitsanw
Copy link
Contributor

nitsanw commented Jul 26, 2023

@ppkarwasz I appreciate your effort here, but please minimize the contents of the PR to the absolute minimum.
Re-formatting of spaces and tabs, re-ordering of imports, changing unrelated pom files, all of these are not part of the value proposition of your PR. Please revise the PR to contain only the essential changes.

Thanks!

@franz1981
Copy link
Collaborator

many thanks @nitsanw for the quick review!!

This commit enables the automatic generation of a JPMS module
descriptor together with a matching OSGI manifest.
@ppkarwasz
Copy link
Contributor Author

ppkarwasz commented Jul 26, 2023

Hi @nitsanw,

I refactored the PR to the bare minimum: the maven-bundle-plugin is reconfigured to export only packages explicitly marked as exported and to generate a module-info.class with a matching OSGI manifest.

I'm confused about the value of moving the internal classes to internal, can you elaborate?

OSGI-wise it doesn't make sense: since the other artifacts in this repo use classes from the org.jctools.core.util package, you need to export the package to everybody.

The advantage appears on the JPMS side: using the exports ... to directive you can prevent third-party modules from using the classes, while at the same time allowing your modules to use them. This is what JUnit 5 did with junit-platform-common (cf. their module-info.java file). Since PaddedAtomicLong must be exported for backward compatibility, it is easier to move the remaining classes.

@ppkarwasz
Copy link
Contributor Author

That'd be good, although I think in JPMS land you should not append a version but just use org.jctools.core

@jponge: I just copied the result of javap module-info.class in the above comment. The compiled module descriptor has a version attribute, but I don't think it can be used for anything practical.

@jponge
Copy link

jponge commented Aug 4, 2023

@franz1981 note that a 4.0.2 with a Automatic-Module-Name manifest entry would work for my case in Mutiny

@jponge
Copy link

jponge commented Aug 4, 2023

I've checked this PR, the generated module-info looks good:

module org.jctools.core {
    requires java.base;
    requires jdk.unsupported;

    exports org.jctools.maps;
    exports org.jctools.util;
    exports org.jctools.queues;
    exports org.jctools.queues.atomic;
    exports org.jctools.queues.unpadded;
    exports org.jctools.counters;
}

@nitsanw nitsanw merged commit 463181b into JCTools:master Aug 17, 2023
1 check passed
@ppkarwasz ppkarwasz deleted the jpms-module branch August 17, 2023 15:50
@franz1981
Copy link
Collaborator

@nitsanw do you think it's ok to roll a new release and get this in?
AFAIK @jponge as others could rely on the presence to dep on JCTools

@nitsanw
Copy link
Contributor

nitsanw commented Oct 5, 2023

@franz1981 it's on my todo list :-) I hope to release in the next couple of weeks

@franz1981
Copy link
Collaborator

@nitsanw If I can help you somehow, just ping me via email (I've sent you an email some time ago, so you'll likely see that one too ;) ) that I will happily help

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

4 participants