Re: cdi-api leaking?

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: cdi-api leaking?

rfscholte
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  
<[hidden email]> wrote:

> Up?
>
> Le 19 janv. 2018 13:18, "Romain Manni-Bucau" <[hidden email]> 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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: cdi-api leaking?

Tibor Digana
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?

On Tue, Feb 6, 2018 at 8:57 AM, Romain Manni-Bucau <[hidden email]>
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 <[hidden email]>:
>
>> 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 <[hidden email]>
>> 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 <[hidden email]>
>>> 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
>>>> >
>>>> > <[hidden email]> wrote:
>>>> > > Up?
>>>> > >
>>>> > > Le 19 janv. 2018 13:18, "Romain Manni-Bucau" <[hidden email]>
>>>> 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: [hidden email]
>>>> > For additional commands, e-mail: [hidden email]
>>>>
>>>>
>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: [hidden email]
>>>> For additional commands, e-mail: [hidden email]
>>>>
>>>>
>>>
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: cdi-api leaking?

Stuart McCulloch
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.

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.

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.”

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

--  
Cheers, Stuart


On Tuesday, 6 February 2018 at 09:09, Romain Manni-Bucau wrote:

> 2018-02-06 9:41 GMT+01:00 Tibor Digana <[hidden email] (mailto:[hidden email])>:
>  
> > 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 <[hidden email] (mailto:[hidden email])>
> > 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 <[hidden email] (mailto:[hidden email])>:
> > >  
> > > > 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 <[hidden email] (mailto:[hidden email])>
> > > > 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 <[hidden email] (mailto:[hidden email])>
> > > > > 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
> > > > > > >  
> > > > > > > <[hidden email] (mailto:[hidden email])> wrote:
> > > > > > > > Up?
> > > > > > > >  
> > > > > > > > Le 19 janv. 2018 13:18, "Romain Manni-Bucau" <
> > [hidden email] (mailto:[hidden email])>
> > > > > > 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: [hidden email] (mailto:[hidden email])
> > > > > > > For additional commands, e-mail: [hidden email] (mailto:[hidden email])
> > > > > > >  
> > > > > >  
> > > > > >  
> > > > > >  
> > > > > >  
> > > > > > ---------------------------------------------------------------------
> > > > > > To unsubscribe, e-mail: [hidden email] (mailto:[hidden email])
> > > > > > For additional commands, e-mail: [hidden email] (mailto:[hidden email])
> > > > > >  
> > > > >  
> > > >  
> > >  
> >  
> >  
>  
>  
>