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