Any issue moving forward on https://github.com/apache/maven/pull/408 ?
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> Le jeu. 3 déc. 2020 à 20:08, Romain Manni-Bucau <[email protected]> a écrit : > created https://github.com/apache/maven/pull/408 about it > > 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> > > > Le mar. 1 déc. 2020 à 17:50, Romain Manni-Bucau <[email protected]> a > écrit : > >> Up, >> >> Encountered a few bugs related to this regression, wonder how we want to >> tackle it. >> My 2cts would be to drop cdi-api and replace the single used >> annotation from there by a maven one. >> If we don't want to break plugins (not sure any use that) we can rewrite >> it with asm or equivalent at load time since we own the classloading. >> >> Anyone having an opinion on that? >> >> 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> >> >> >> Le mar. 6 févr. 2018 à 18:02, Romain Manni-Bucau <[email protected]> >> a écrit : >> >>> 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]) >>>> > > > > > > > > >>>> > > > > > > > >>>> > > > > > > >>>> > > > > > >>>> > > > > >>>> > > > >>>> > > >>>> > > >>>> > >>>> > >>>> > >>>> >>>> >>>> >>>
