commented inline
Romain Manni-Bucau @rmannibucau <https://twitter.com/rmannibucau> | Blog <https://rmannibucau.metawerx.net/> | Old Blog <http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> | LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book <https://www.packtpub.com/application-development/java-ee-8-high-performance> 2018-02-06 17:51 GMT+01:00 Stuart McCulloch <[email protected]>: > On Tuesday, 6 February 2018 at 13:25, Romain Manni-Bucau wrote: > > 2018-02-06 12:25 GMT+01:00 Stuart McCulloch <[email protected] (mailto: > [email protected])>: > > > > > The "javax.enterprise.inject" CDI package was explicitly exported as > part > > > of the ongoing effort to migrate legacy Plexus components onto more > modern > > > standard annotations. > > > > > > > > > Hmm, this looks wrong from my window cause Maven doesn't support CDI API > - > > guice doesn't. So it is an interpretation of a well defined API which is > by > > defintion a bad public API, no? > > > > > > Raw Guice supports JSR330, but requires programmatic configuration (ie. > bindings defined in modules) > > Sisu builds on Guice to add support for things like annotation scanning > and wiring, injectable collections that dynamically update as plugins come > and go, property placeholders, etc. > > If the CDI annotations are on the classpath then it also honours the > @Typed annotation. This was a feature request to help migrate certain > components from other Plexus-based applications over to JSR330 + @Typed. At > the time there was a consideration that the rest of the CDI annotations > would eventually be supported, as another compatibility layer. > > Sisu also provides a Plexus compatibility layer that supports Plexus > annotations and XML > > Maven 3.x switched to Sisu so old Plexus-based components can still be > used, while modern components can be written with JSR330. At the time of > the switch it was decided to enable support for @Typed in Maven > plugins/extensions (because there was a developer need for this feature, > but that may well have changed and no longer be relevant). > > So Maven currently honours using @Typed on components - if it’s decided > that Maven doesn’t want to support @Typed in plugins then just remove the > export and exclude the cdi-api jar. As mentioned previously support for > @Typed is used by other downstream non-Maven applications so it will always > be something the container supports, but it's totally optional so if you > don’t want it then you don’t need to ship the cdi-api jar. > Yes but having the full API for one class is luxury (see later comment for the detail) > > > > > > > > Specifically, the @javax.enterprise.inject.Typed annotation lets > > > components state they are only visible for injection under a specific > type, > > > rather than any type in their hierarchy. > > > > > > There’s no annotation to control binding visibility in JSR330, because > it > > > deliberately avoids configuration concerns, which is why we went with > the > > > closest standard annotation (@Typed from JSR299 aka CDI). While we > could > > > have decided to use our own annotation - and the container does in fact > > > support using @org.eclipse.sisu.Typed - this is not standardised or > > > portable. Also note the container will continue to support this > (optional) > > > feature for other downstream users, regardless of what’s decided here > - the > > > question is whether Maven still wants to use this feature and whether > it > > > wants to use the standard annotation or not. > > > > > > Another point is that whichever annotation is chosen must be > > > visible/defined from the same classloader to both core and plugins. If > the > > > annotation is not exported then core and each plugin will end up with a > > > different @Typed class, defined by different classloaders. Any use of > > > @Typed in plugins would then effectively be invisible to the container, > > > because the JVM’s AnnotatedElement API (getAnnotation, > isAnnotationPresent, > > > etc.) work off classes and not name equivalence. > > > > > > Similarly shading won’t work because neither the plugin’s components > nor > > > the container would know about the shaded package. > > > > > > > > > Hmm, not sure. I mean it works in most projects and it is easy to expose > > the shaded API so not a big deal *technically*. Agree it would be a bad > > solution to use a misused API publicly. > > > > > > By “not work” I really meant “not practical”. It’s not enough to just > shade the CDI jar, you’d also need to shade the container - being careful > that its reflective calls were properly updated (since it uses reflection > to decide whether to load the feature or not). TBH all that work is > overkill, since the container already supports an alternative annotation: > @org.eclipse.sisu.Typed > works for me > > > > > > > > > > As you can see from the thread in http://maven.40175.n5.nabble. > > > com/Linkage-error-td5784411.html a number of alternative solutions > have > > > been discussed before, including narrowing the export to: > > > > > > "javax.enterprise.inject.Typed" > > > > > > as that’s the only annotation we’re currently interested in. Since > @Typed > > > hasn’t changed between 1.x and 2.x that should be a workable solution, > > > assuming you wanted to keep using the standard annotation. > > > > > > Removing the export (and thereby removing the feature to limit > injection > > > visibility to a specific type) was also discussed, and at the time Igor > > > asked for it to be kept: > > > > > > “Please keep @Typed annotation available outside of core. > > > > > > I use @Typed annotation in one of my (private) core extensions where I > > > need a class to implement an interface but not make that interface > > > visible for injection in other components.” > > > > > > > > > Issue is I can say the opposite "I use this in my plugin cause I use CDI > to > > impl my plugin, please ignore it for all Maven usage". Both are valid and > > therefore the Maven API shouldn't have any overlapping. > > > > > > Whenever you embed a container inside any kind of plugin you’re at the > mercy of what’s exposed to that plugin - whether that plugin is running in > Maven, Jenkins, or an IDE. If you want full control/sanity then use a > custom classloader to isolate the embedded container from the plugin’s > environment, and just let through those packages you expect to be provided. > > For example, say we did fully support CDI 1.x components inside plugins > (as in the entire API was supported). You’d still have an issue embedding a > CDI 2.x container, because of the API clash, unless you used a custom > classloader between the plugin and the embedded container. > Yes but would have complained way earlier ;) > > > > > > > > > > Assuming Igor still needs this feature then the only other option > would be > > > to ask him if he can move to the non-standard @org.eclipse.sisu.Typed. > The > > > existing CDI export could then be replaced by exporting > “org.eclipse.sisu”. > > > Once that was done then the cdi-api dependency could be excluded from > the > > > distribution, as the container will still work without it on the > classpath > > > (it’s only required if you want to use the standard CDI annotation). > > > > > > So to summarise, the options are: > > > > > > a) Continue to support the standard API, but narrow the entry in > > > META-INF/maven/extension.xml to “javax.enterprise.inject.Typed” > > > > > > b) Switch to support @org.eclipse.sisu.Typed > > > > > > c) Remove this feature completely from Maven > > > > From what I'm concerned b and c would solve it but I guess sisu users can > > have the same issue - not sure how likely it is. > > > > > > Sisu users typically have control over the container classpath and can > choose whether to include CDI or not (and at which level) > > There is a d) option: add in @Mojo a list of imported API. ClassRealm can > > support filtering from the parent classloader and therefore I could use: > > > > @Mojo(name ="...", pluginPackages={"javax", ...}) > > > > This would allow to keep current setup and let mojo to override it. > > Compared to a) it is defined in plugin.xml and not extension.xml. > > > > > > At the moment there’s a single Maven API realm, which imports all the > packages listed in the core extension.xml from the core classloader. > Plugins then import that realm wholesale, so they automatically get all > exported packages. However, it should be possible to be more selective, > whether that’s using a whitelisting or blacklisting approach. > > That said, it would be much simpler to either remove the export or switch > to @org.eclipse.inject.Typed (since use of the annotation in Maven is > currently very limited) > A last alternative is to still support @Typed without providing it. Concretely it means maven drops cdi (sadly not inject jar) and use asm to check if @Typed if here. Sounds the less breaking compromise even if not the most sexy in terms of impl. > > > > > > > > -- > > > Cheers, Stuart > > > > > > > > > On Tuesday, 6 February 2018 at 09:09, Romain Manni-Bucau wrote: > > > > > > > 2018-02-06 9:41 GMT+01:00 Tibor Digana <[email protected] > (mailto:[email protected]) (mailto: > > > [email protected] (mailto:[email protected]))>: > > > > > > > > > Personally I would like to see a new Git branch with CDI 2.0 and > the > > > > > integration test results on Jenkins. > > > > > This would give us more confidence. > > > > > Question: Does the CDI 2.0 have any NEW mandatory descriptive > methods > > > > > without default value already introduced in OLD annotations CDI > > > > > > > > > > > > > > > > > > > 1.0/1.1? > > > > > > > > > > > > > > > > > > > > > It is more a change in the hierarchy. It doesn't break the user API > since > > > > cdi is designed to be provided but it is broken if new code uses old > API. > > > > > > > > Side note: if the idea behind this answer is to ensure the default > > > provided > > > > API is the last one then it doesn't work cause an API has a few logic > > > > > > which > > > > can require to be overriden (like the SPI and defaults handling). > > > > Maven uses its own API and exposing CDI is a leaking abuse IMHO. > > > > > > > > Note that this is an old bug which should be fixed now IMO before > maven > > > > considers CDI being exposed as part of the contract. > > > > > > > > For reference, older threads: > > > > > > > > http://maven.40175.n5.nabble.com/libs-in-mavens-lib-folder- > > > td5828015.html > > > > http://maven.40175.n5.nabble.com/Linkage-error-td5784411. > html#a5784470 > > > > > > > > > > > > There is no risk removing it, worse case plugins would add the API as > > > > compile instead of provided which should likely already be the case. > > > > > > > > > > > > > On Tue, Feb 6, 2018 at 8:57 AM, Romain Manni-Bucau < > > > [email protected] (mailto:[email protected])> > > > > > wrote: > > > > > > > > > > > > > > > > > For the reproducer here it is https://github.com/ > > > > > > rmannibucau/test-maven-plugin - pretty trivial you'll see ;). > > > > > > > > > > > > 2018-02-06 8:05 GMT+01:00 Tibor Digana <[email protected] > (mailto:[email protected]) > > > (mailto:[email protected])>: > > > > > > > > > > > > > Changing the package would not be possible in 3.x. > > > > > > > > > > > > Why? In particular since it is an old regression already > reported on > > > the > > > > > > list due to guice introduction it shouldn't be delayed for this > kind > > > > > > > > > > > > > > > of > > > > > > reason IMHO. > > > > > > Was less visible until CDI 2 was released cause the API > difference > > > > > > > > > > > > > > > > > > > > > was > > > > > > > > > > > > > > > > > > > > > not > > > > > > triggered but now there are new entries it breaks immediately. > > > > > > > > > > > > > > > > > > > Guessing the version 4.0.0. > > > > > > > WDYT? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Would stay a blocker until 4 is out which is that soon so not > sure > > > it is > > > > > > an option. > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Feb 6, 2018 at 8:01 AM, Tibor Digana < > > > [email protected] (mailto:[email protected])> > > > > > > > wrote: > > > > > > > > > > > > > > > The question is maybe about what is realistic for Maven devs. > > > > > > > > Shading the CPI package (to something like > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > org.apache.maven.cdi.*) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > would > > > > > > > > be maybe the case instead of removing the original CDI and > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > reinventing > > > > > > > > > > > > > > > > > > > > > > > > > > > > the > > > > > > > > wheel. > > > > > > > > > > > > > > > > On Tue, Feb 6, 2018 at 7:52 AM, Hervé BOUTEMY < > > > [email protected] (mailto:[email protected])> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > and does the MNG issue contain a reproducible test case > for us > > > to > > > > > > > > > investigate > > > > > > > > > more precisely? > > > > > > > > > > > > > > > > > > Regards, > > > > > > > > > > > > > > > > > > Hervé > > > > > > > > > > > > > > > > > > Le lundi 5 février 2018, 22:11:56 CET Robert Scholte a > écrit : > > > > > > > > > > Is there a MNG[1] issue? > > > > > > > > > > > > > > > > > > > > Robert > > > > > > > > > > > > > > > > > > > > [1] https://issues.apache.org/jira/browse/MNG > > > > > > > > > > > > > > > > > > > > On Sat, 03 Feb 2018 16:29:49 +0100, Romain Manni-Bucau > > > > > > > > > > > > > > > > > > > > <[email protected] (mailto:[email protected])> > > > wrote: > > > > > > > > > > > Up? > > > > > > > > > > > > > > > > > > > > > > Le 19 janv. 2018 13:18, "Romain Manni-Bucau" < > > > > > [email protected] (mailto:[email protected])> > > > > > > > > > a > > > > > > > > > > > > > > > > > > > > > > écrit : > > > > > > > > > > > > Hi guys, > > > > > > > > > > > > > > > > > > > > > > > > cdi-api is still in maven lib and breaks any plugin > > > using it > > > > > since > > > > > > > > > it is > > > > > > > > > > > > an old version, can it be dropped or at least > isolated > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > from > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > plugin > > > > > > > > > > > > classloaders? > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > Romain Manni-Bucau > > > > > > > > > > > > @rmannibucau <https://twitter.com/rmannibucau> | > Blog > > > > > > > > > > > > <https://rmannibucau.metawerx.net/> | Old Blog > > > > > > > > > > > > <http://rmannibucau.wordpress.com> | Github > > > > > > > > > > > > <https://github.com/rmannibucau> | LinkedIn > > > > > > > > > > > > <https://www.linkedin.com/in/rmannibucau> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > ------------------------------ > ------------------------------ > > > > > --------- > > > > > > > > > > To unsubscribe, e-mail: [email protected] > (mailto:[email protected]) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > (mailto:[email protected]) > > > > > > > > > > For additional commands, e-mail: > [email protected] (mailto:[email protected]) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > (mailto:[email protected]) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > ------------------------------ > ------------------------------ > > > --------- > > > > > > > > > To unsubscribe, e-mail: [email protected] > (mailto:[email protected]) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > (mailto:[email protected]) > > > > > > > > > For additional commands, e-mail: [email protected] > (mailto:[email protected]) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > (mailto:[email protected]) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
