On Sat, 19 Oct 2019 02:25:47 +0200, Jason van Zyl <[email protected]> wrote:
Inline:
On Oct 18, 2019, at 5:15 PM, Stuart McCulloch <[email protected]> wrote:
Any string apart from "default" or the empty string will work here - I
happened to chose "core" because I viewed it as a core implementation.
Also a quick reminder that this is only needed when a component uses
@Typed
and wants to allow overriding, it's not necessary in any other
situation -
so in that sense "allowDefaultOverride" wouldn't be quite accurate. (See
the javadoc in the patch for more detail.)
Stuart,
There is an idiom like this used in the ReactorReader and the
GraphBuilder where there are implementations of them in extensions out
in the wild, and while those are not @Typed they use a @Named(
“not-default” ) key pattern. If these components are intended to allow
for custom implementations then a common way of doing this would be
easier to understand. Something like @Named( “coreAllowingOverride” ) or
@Named( “coreExtensionPoint” ) or whatever most appropriate. Right now
there’s a bit of fiddly magic that works in Plexus with a lookup and
Plexus with its annotations, and slightly different way with Sisu with
@Inject annotations. So, sure, this particular case of requiring @Named(
“core” ) to fix the case where you want to override a component with the
implicit “default” key is required, but maybe something we should avoid
and if it’s meant to be extended just have an explicit common pattern. I
realize with DI you can override anything, but I don’t think being a
little more explicit would hurt. The nuance of how the bindings work
maybe you and Igor know/remember how it works, I had to use a debugger
this afternoon :-)
I guess the best value would have been "default", but that won't work.
Since all components can be overridden, I suggest to change it to simply
"DefaultModelProcessor".
Some people already want to add module descriptors to their
plugins/extensions. In order to support that we already need to
restructure interfaces and make a clear separatation of SPIs and APIs.
That's likely better than trying to do more magic with the @Named
annotation that's already around for quite some time.
Robert,
What’s overridden in polyglot is to be injected into Maven’s core is the
ModelProcessor, not the ModelBuilder:
https://github.com/takari/polyglot-maven/blob/master/polyglot-common/src/main/java/org/sonatype/maven/polyglot/TeslaModelProcessor.java
The ModelReaders are injected into a manager within the
TeslaModelProcessor, sure, but it’s the ModelProcessor being overridden
which makes the magic happen in polyglot. When the ModelProcessor
polyglot requires doesn't get injected it’s trying to use the XML-based
model reader to read Kotlin which doesn’t work out so well. Trying to
build a polyglot project without the extensions loaded in 3.6.1 yields
the same result as trying to use 3.6.2 with a polyglot project: the same
error which is a result of the right ModelProcessor not being found.
You’ll see the the DefaultModelProcessor being used instead of the
TeslaModelProcessor in the stack trace.
Right, so the stacktrace fooled me. ModelProcessor it is.
PS. the reason DefaultModelProcessor needs to use @Typed is because it
has
an "interesting" interface hierarchy where ModelProcessor extends
both ModelLocator and ModelReader, so it can act as both and delegate
accordingly - but then it also asks to have a ModelLocator and
ModelReader
injected, which is where things get messy. If it had a cleaner hierarchy
(ie. it wasn't asking to inject something that it also implements) then
it
wouldn't need @Typed and wouldn't then need the custom name.
Agreed. Looks fixable and there are probably only two consumers in the
world. Polyglot and
https://github.com/qoomon/maven-git-versioning-extension where it was
attempt to fix the problem in 3.6.2.
jvz
On Fri, 18 Oct 2019 at 20:35, Robert Scholte <[email protected]>
wrote:
Hi,
the adjusted @Named annotation is on DefaultModelProcessor, not
DefaultModelBuilder.
They both implement the ModelBuilder interface, but the one that
extensions like to overwrite is the implementation of
DefaultModelBuilder.
So I'd prefer to stick to "core" as proposed my Stuart.
thanks for the confirmation that this works,
Robert
On Fri, 18 Oct 2019 21:10:18 +0200, Jason van Zyl <[email protected]>
wrote:
Hi,
As noted in Slack I think it would be more clear if we used something
like
@Named( “allowDefaultOverride” )
vs
@Named( “core” )
As that expresses the intent and can be used anywhere it's allowed for
a
client to override the default component.
The tests in polyglot all pass with this change, and I’m able to run
polyglot example projects again, so I assume the Tycho POM-less are
happy again as well but hopefully someone can verify.
Jason
On Oct 18, 2019, at 2:04 PM, Robert Scholte <[email protected]>
wrote:
Hi,
with the help from Stuart McCulloch we've been able to provide a
patch
for MNG-6765[1]
Please review and test.
thanks,
Robert
[1] https://issues.apache.org/jira/browse/MNG-67
[2]
https://github.com/apache/maven/commit/24e6c0ec0a87b6682513287a23c36db6996b874c
[3]
https://github.com/apache/maven/commit/53a70bc8543124569ee787725b2004bc92a681b6
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
