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]) >>> > > > > > > > > >>> > > > > > > > >>> > > > > > > >>> > > > > > >>> > > > > >>> > > > >>> > > >>> > > >>> > >>> > >>> > >>> >>> >>> >>
