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

Support java module system (jpms) via Multi-Release jar with module-info - [was Minimum Java 11...] #53

Closed
rbygrave opened this issue Oct 26, 2020 · 11 comments
Assignees
Milestone

Comments

@rbygrave
Copy link
Contributor

rbygrave commented Oct 26, 2020

Originally this was:

Minimum Java 11 - make avaje-inject Java 11+ (from Java 8+)

@rbygrave rbygrave modified the milestone: 1.2 Oct 26, 2020
@norrisjeremy
Copy link
Contributor

I'm curious to understand why you're considering making it require a minimum of Java11?

@rbygrave
Copy link
Contributor Author

rbygrave commented Oct 27, 2020

In theory:

  • We can make in jpms aware / add a module-info.java which increases the required discipline on avaje-inject in terms of dependencies and what is public
  • I don't really consider Java 9 or Java 10 to be preferred. I think Java 11 is the next target after 8 being somewhat a LTS version
  • I have a Kubernetes/Docker deployment bias so as I see it Java 11 is an minimum runtime

In reality I'm not sure if this is a great idea yet. I don't know when libraries should bump up from Java 8 but I think we will have to do it at some point so when ... (personally and for the places I work they are all Java 11+).

Hmmm.

@norrisjeremy
Copy link
Contributor

From what is an admittedly selfish desire, I would prefer to see that it remain Java8 compatible for now, since we still have several projects that still need to maintain Java8 compatibility unfortunately.

For JPMS compatibility, I think it may be possible to use the moditect-maven-plugin to generate the module-info.class file without fulling moving everything to Java9+ (although I've not fully investigated this particular Maven plugin to confirm).

It's also not especially difficult to build a multi-release jar in order to add Java9+ features. I've done it for the JSch fork to add support for new crypto algorithms that are available in Java11 & Java15, while keeping the base jar file fully Java8 compatible. It involved adding a few extra execution sequences for maven-compiler-plugin & maven-resources-plugin, and then having the maven-jar-plugin add the multi-release attribute to the manifest.

But I understand if you want to avoid adding these sorts of complexities and decide to move to purely requiring Java11+.

@rbygrave
Copy link
Contributor Author

PR for JPMS compatibility via moditech-maven-plugin ... #59

@rbygrave
Copy link
Contributor Author

rbygrave commented Oct 29, 2020

For myself with JPMS we need to explicitly register the BeanContextFactory (which is kind of nice) with a provides clause like:

module junk {

  requires io.avaje.inject;

  provides io.avaje.inject.core.BeanContextFactory with org.example._di$Factory;
}

... with jpms developers need to know about and find the BeanContextFactory interface. My gut says it would be nicer if BeanContextFactory was in the io.avaje.inject package (and perhaps the generated _di$Factory should instead be called _di$BeanContextFactory. Hmmm.

Edit: Updated the example module code.

@norrisjeremy
Copy link
Contributor

... with jpms developers need to know about and find the BeanContextFactory interface. My gut says it would be nicer if BeanContextFactory was in the io.avaje.inject package (and perhaps the generated _di$Factory should instead be called _di$BeanContextFactory. Hmmm.

I think you may be right that it would be better for BeanContextFactory to be in io.avaje.inject. Then I think you wouldn't need to export the io.avaje.inject.core package.

@rbygrave
Copy link
Contributor Author

wouldn't need to export the io.avaje.inject.core package

There are things in core used by the generated code like BeanLifeCycle, Builder etc. So those need to be in an exported package.

I think the options are something like:

  • move BeanContextFactory to io.avaje.inject (it references Builder interface which is in core but maybe that is ok)
  • rename io.avaje.inject.core to say io.avaje.inject.spi (Is spi a better package name? ... more expected?)
  • move everything in core into io.avaje.inject (I don't like that option, core has code the generated code needs but otherwise is "detail")

Hmmm.

@norrisjeremy
Copy link
Contributor

norrisjeremy commented Oct 29, 2020

I think the options are something like:

  • move BeanContextFactory to io.avaje.inject (it references Builder interface which is in core but maybe that is ok)
  • rename io.avaje.inject.core to say io.avaje.inject.spi (Is spi a better package name? ... more expected?)
  • move everything in core into io.avaje.inject (I don't like that option, core has code the generated code needs but otherwise is "detail")

Hmmm.

Hmm, it may be more hassle than it's worth to move BeanContextFactory now that I look more closely.
It appears that DependencyMeta, BuilderFactory & Builder would also need to be moved, since it looks like they are all used by the generator as well (both directly and in the code it generates in consumers).

@rbygrave
Copy link
Contributor Author

more hassle than it's worth to move BeanContextFactory now that I look more closely

I think the same.

I'm probably leaning toward renaming core package to spi - mostly as a gut feel really with "core" sounding a bit too implementation orientated to me. It hasn't mattered in classpath world but once people are explicitly putting the provides clause in module-info core doesn't look right to me.

@norrisjeremy
Copy link
Contributor

I'm probably leaning toward renaming core package to spi - mostly as a gut feel really with "core" sounding a bit too implementation orientated to me. It hasn't mattered in classpath world but once people are explicitly putting the provides clause in module-info core doesn't look right to me.

I think that sounds good to me.

rbygrave added a commit that referenced this issue Oct 30, 2020
* #51 Add jpms module-info using Multi-Release jar

* #53 - Improve error message for JPMS use and no modules.

* #53 - Make javax.inject and org.slf4j transitive. Fix associated readme example.

* #53 - Add explicit requires javax.inject to avaje-inject-generator

* #53 - Tidy avaje-inject-generator module-info, remove javax.inject as brought in transitively
@rbygrave rbygrave changed the title Minimum Java 11 - make avaje-inject Java 11+ (from Java 8+) Support java module system (jpms) via Multi-Release jar with module-info - [was Minimum Java 11...] Oct 30, 2020
@rbygrave rbygrave added this to the 2.0 milestone Oct 30, 2020
@rbygrave rbygrave self-assigned this Oct 30, 2020
@rbygrave
Copy link
Contributor Author

Ok, lets call this bit done :)

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

No branches or pull requests

2 participants