Re: Second MNG-6765 ([Regression] tycho pom-less builds fails with 3.6.2)
Inline: On Oct 18, 2019, at 5:15 PM, Stuart McCulloch 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 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 >> 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 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: dev-unsubscr...@maven.apache.org For additional
Re: Second MNG-6765 ([Regression] tycho pom-less builds fails with 3.6.2)
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.) 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. On Fri, 18 Oct 2019 at 20:35, Robert Scholte 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 > 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 > >> 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: dev-unsubscr...@maven.apache.org > >> For additional commands, e-mail: dev-h...@maven.apache.org > >> > > > > > > - > > To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org > > For additional commands, e-mail: dev-h...@maven.apache.org > > - > To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org > For additional commands, e-mail: dev-h...@maven.apache.org > >
Re: Second MNG-6765 ([Regression] tycho pom-less builds fails with 3.6.2)
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 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 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: dev-unsubscr...@maven.apache.org For additional commands, e-mail: dev-h...@maven.apache.org - To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org For additional commands, e-mail: dev-h...@maven.apache.org - To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org For additional commands, e-mail: dev-h...@maven.apache.org
Re: Second MNG-6765 ([Regression] tycho pom-less builds fails with 3.6.2)
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 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: dev-unsubscr...@maven.apache.org > For additional commands, e-mail: dev-h...@maven.apache.org > - To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org For additional commands, e-mail: dev-h...@maven.apache.org
Re: JSR250 in the lib
Petar, can you create a JIRA issue[1] for it plus a PR based on the advice of Stuart. You should be able to verify the change for yourself, if upgrading the dependency works. thanks, Robert [1] https://issues.apache.org/jira/browse/MNG On Mon, 14 Oct 2019 13:42:40 +0200, Olivier Lamy wrote: +1 I definitely agree with Stuart, This is a very simple API with a minimum of interfaces and used a lot. I guess we already did such mistake with Maven2 and Plexus :) So it's only a matter of not replay history. Who really wants to have to maintain another already existing library whereas we already need people for what we have now. On Mon, 14 Oct 2019 at 21:37, Stuart McCulloch wrote: Sorry, but why would you want to write your own version of javax.inject which is a very small static API supported by many injection systems, both EE and non-EE It means you can't inject components written outside of Maven unless you write adapters or manually construct them. It also means that someone who wants to write an injectable component for Maven and re-use it elsewhere (such as with Spring) now needs to write two versions. You'll spend a lot of time and end up with duplicate APIs for doing exactly the same thing, when you could have just bumped the JSR-250 version and moved on. On Mon, 14 Oct 2019 at 12:22, Tibor Digana wrote: > It would not be finally the same/identical list of annotations. > We do not have to copy everything, as for instance we do not have to copy > descriptive methods, names of annotations, packages. > So custom annotations means that: > + we have all responsibility in our hands > + we do not have to rely on dead Java EE annotations > + we can add new annottaions tailored to our Maven domain (not EE domain) > > We can erase some annotations, e.g.: > 1. https://docs.oracle.com/javaee/6/api/javax/inject/Named.html - not sure > if we use expressions with @Named beans > instead maybe we meant something like @javax.annotation.ManagedBean but it > is only my guess because @Named is useless without using expressions in the > code (not in POM). > 2. we can erase > https://docs.oracle.com/javaee/6/api/javax/inject/Scope.html > because we do not have e.g. HTTP Session, Conversation scope and Html > forms, we do not have Request Sope because we have our own lifecycle with > phases. So again not very compatible with Maven domain. Usually we have > singletones and I have never seen scope of liveness of the bean instances > in Maven. Maybe we would need to have a range of phases in the future where > the bean would be valid (from compile to process-classes) even in the > parallel Maven -T 2C, maybe. > > It's good that we have tried to adopt annotations from different domain and > IMHO the MNG-6084 is the experience that we should not repeat and should > not again adopt foreign annotations (as here in non-EE container) from > non-Maven domains. > > > On Mon, Oct 14, 2019 at 12:43 PM Stuart McCulloch > wrote: > > > There are already equivalents, adding yet another "standard" that's > > specific to Maven is just like writing yet another logging API IMHO: > > > >javax.annotation.Priority org.eclipse.sisu.Priority > > > >javax.annotation.PostConstruct > > org.codehaus.plexus.personality.plexus.lifecycle.phase.Initializable > >javax.annotation.PreDestroy > > org.codehaus.plexus.personality.plexus.lifecycle.phase.Disposable > > > >(these last two are not totally equivalent since the Plexus API uses > > interfaces instead of annotations, but they support the same goal) > > > > The container will work with or without JSR-250 on the classpath so this > is > > more about what you want to let plugin component authors to use. If > you're > > happy with them only using Maven specific annotations and having to > > retro-fit or add adapters when they want to use components outside of > Maven > > that use PostConstruct and PreDestroy then just roll back MNG-6084, > making > > sure to add a release note warning any plugin authors depending on this > > feature that they will need to rewrite or adapt their plugins. > > > > Also note that this API is only exposed to plugins, it should _not_ be > > leaking onto the build classpath ... if it is then that's a bug. So this > > situation is specific to when a plugin either actively uses a dependency > > that wants a later version of JSR-250 or wants to use that later version > > itself. > > > > Since the JSR-250 API doesn't change much I still think just bumping the > > version in the distro is the simplest and less disruptive option. > > > > On Mon, 14 Oct 2019 at 11:03, Tibor Digana > wrote: > > > > > All these annotations are for Java EE containers and application > servers. > > > A clear solution in 4.0 or 5.0 would be to develop our own annotations > > for > > > Maven Domain (not EE domain) within or Java package. > > > > > > This way we would reach: > > > + annotations looking similar to EE
Second MNG-6765 ([Regression] tycho pom-less builds fails with 3.6.2)
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: dev-unsubscr...@maven.apache.org For additional commands, e-mail: dev-h...@maven.apache.org
Did you see dependabot?
Hey guys, Did you see dependabot on our repos? Like this automatic PR https://github.com/apache/maven-plugins/pull/147#pullrequestreview-303889692 I feel this is very useful, but... does anyone enabled it? Do we have to set a policy, this suggestions are security related fixes, we could give them some kind of high priority? Enrico
