Second MNG-6765 ([Regression] tycho pom-less builds fails with 3.6.2)

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Second MNG-6765 ([Regression] tycho pom-less builds fails with 3.6.2)

rfscholte
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-6765
[2]  
https://github.com/apache/maven/commit/24e6c0ec0a87b6682513287a23c36db6996b874c
[3]  
https://github.com/apache/maven/commit/53a70bc8543124569ee787725b2004bc92a681b6

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Second MNG-6765 ([Regression] tycho pom-less builds fails with 3.6.2)

Jason van Zyl-7
Inline:

 On Oct 18, 2019, at 5:15 PM, Stuart McCulloch <[hidden email]> 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 :-)

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.

> 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 <[hidden email]> 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 <[hidden email]>
>> 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 <[hidden email]>
>>>> 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: [hidden email]
>>>> For additional commands, e-mail: [hidden email]
>>>>
>>>
>>>
>>> ---------------------------------------------------------------------
>>> 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]
>>
>>



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Second MNG-6765 ([Regression] tycho pom-less builds fails with 3.6.2)

rfscholte
On Sat, 19 Oct 2019 02:25:47 +0200, Jason van Zyl <[hidden email]> wrote:

> Inline:
>
>  On Oct 18, 2019, at 5:15 PM, Stuart McCulloch <[hidden email]> 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 <[hidden email]>  
>> 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 <[hidden email]>
>>> 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 <[hidden email]>
>>>>> 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: [hidden email]
>>>>> For additional commands, e-mail: [hidden email]
>>>>>
>>>>
>>>>
>>>> ---------------------------------------------------------------------
>>>> 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]
>>>
>>>
>
>
>
> ---------------------------------------------------------------------
> 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: Second MNG-6765 ([Regression] tycho pom-less builds fails with 3.6.2)

Hervé BOUTEMY
In reply to this post by rfscholte
+1
just added a comment on a typo

for the name of the component, perhaps "core-default", but I won't complain
about any choice: the javadoc is what was really needed

notice: perhaps we have other component that should have the same improvement
to permit overriding in the future

Regards,

Hervé

Le vendredi 18 octobre 2019, 20:04:53 CEST Robert Scholte a écrit :

> 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-6765
> [2]
> https://github.com/apache/maven/commit/24e6c0ec0a87b6682513287a23c36db6996b8
> 74c [3]
> https://github.com/apache/maven/commit/53a70bc8543124569ee787725b2004bc92a68
> 1b6
>
> ---------------------------------------------------------------------
> 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]