MNG-6799 issue created [1] proposed fix added to the reproducible branch [2] by simply removing the @Named annotation for the deprecated StringSearchModelInterpolator: CI seems ok ok for merge?
Regards, Hervé [1] https://issues.apache.org/jira/browse/MNG-6799 [2] https://github.com/apache/maven/commit/16961b32b7dccbc9447ed4e18b4e0949d5fe3b56 On 2019/11/03 21:27:21, Stuart McCulloch <mccu...@gmail.com> wrote: > I did a quick review of the various components in core > and StringVisitorModelInterpolator / StringSearchModelInterpolator appears > to be the only case where a role (ModelInterpolator) has two non-default > implementations and another component only asks for the first > implementation. There are other roles which have multiple implementations, > but in those cases that's expected because each implementation has a > specific name and handles a specific case (see the various ProfileActivator > implementations which are all then injected into a list in > DefaultProfileSelector.) > > It also appears that StringVisitorModelInterpolator was only added recently > - if it's meant to be a replacement of StringSearchModelInterpolator then > one option would be to simply remove StringSearchModelInterpolator. > Alternatively if StringSearchModelInterpolator is needed to be kept around > for compatibility reasons (such as someone directly extending it outside of > Maven) then the Named annotation could be removed > from StringSearchModelInterpolator so it isn't automatically picked up. > > Wrt. extra checks, checking for multiple implementations in the same > realm/classloader that haven't been given specific names is a possibility - > you can open feature requests at > https://bugs.eclipse.org/bugs/describecomponents.cgi?product=Sisu > > On Sun, 3 Nov 2019 at 19:05, Gabriel Belingueres <belingue...@gmail.com> > wrote: > > > Hi Hervé: > > > > I was thinking in the lines of some checking at the maven core level only, > > enough to detect problems and ensure there are no regressions. > > > > At the plugin level, I guess the assurance that the right component is > > injected can be checked with an integration test. > > > > The way I realized the problem was that I was adding some logging traces > > into the StringVisitorModelInterpolator class but *nothing* showed (even > > the build was successful and the interpolation indeed was happening > > somewhere). Then added the same logging lines to > > StringSearchModelInterpolator, and the logging appeared in the build, which > > confirmed the issue. (I didn't test if there are any other component with > > the same problem.) > > > > El dom., 3 de nov. de 2019 a la(s) 06:27, Hervé BOUTEMY ( > > herve.bout...@free.fr) escribió: > > > > > nice idea, but overriding by adding a new implementation in classpath is > > > the > > > normal overriding way: not sure how "ambiguity checker" could make a > > > difference > > > between such wanted override vs unwanted ambiguity > > > > > > BTW, how did you see that StringVisitorModelInterpolator was not used but > > > StringSearchModelInterpolator instead, please? > > > > > > Regards, > > > > > > Hervé > > > > > > Le dimanche 3 novembre 2019, 05:30:50 CET Gabriel Belingueres a écrit : > > > > Thanks Stuart. > > > > > > > > The reproducibility PR you mention helps in having a deterministic > > > ordering > > > > inside the Named components file to buld exactly the same executable, > > but > > > > it does not mean it is the right priority for component injection. > > > > > > > > Do you know if it is possible to configure sisu to throw an exception > > if > > > > trying to inject an ambiguous component? Otherwise, I guess we must > > > > implement some sort of "ambiguity checker", either as an integration > > test > > > > (don't know if it is possible) or either built-it into maven > > executable. > > > > > > > > > > > > > > > > > > > > El sáb., 2 de nov. de 2019 a la(s) 20:54, Stuart McCulloch ( > > > > > > > > mccu...@gmail.com) escribió: > > > > > Yes, when there are multiple non-default implementations with the > > same > > > > > priority then it will pick the first in the list (where the "list" in > > > > > question is actually a combination of plugins + extensions + core > > > > > bindings for the given type) > > > > > > > > > > If a particular implementation should be treated as the default then > > it > > > > > should either start with "Default" or be annotated with > > > @Named("default") > > > > > - > > > > > the benefit of this approach is that it documents that this is the > > > default > > > > > implementation, to be favoured over the others. Alternatively if you > > > want > > > > > to have an ordering between implementations then you can use > > @Priority > > > to > > > > > assign specific priorities and favour one over the other. > > > > > > > > > > If you don't mark an implementation as default and don't define any > > > > > priorities then the only current guarantees are that implementations > > in > > > > > plugins and extensions will be favoured over implementations in core > > > (to > > > > > allow overriding). But there is an upcoming improvement to sort the > > > list > > > > > inside each module that would make this more deterministic from build > > > to > > > > > build, at least when ranking implementations inside a particular > > > module: > > > > > https://github.com/eclipse/sisu.inject/pull/5 - with that change > > then > > > > > you'll get an extra guarantee that inside a module it will be ordered > > > by > > > > > package+name. > > > > > > > > > > PS. even with the old Plexus runtime annotations you could be at the > > > whim > > > > > of classpath ordering, similarly Plexus XML descriptors are > > influenced > > > by > > > > > classpath resource ordering - so ideally it's better to be explicit > > > about > > > > > ordering > > > > > > > > > > On Sat, 2 Nov 2019 at 23:07, Gabriel Belingueres < > > > belingue...@gmail.com> > > > > > > > > > > wrote: > > > > > > Hi: > > > > > > > > > > > > I just built maven core from source and found that it was using > > > > > > StringSearchModelInterpolator instead of > > > StringVisitorModelInterpolator, > > > > > > > > > > a > > > > > > > > > > > regression from the last 3.6.2 release. > > > > > > > > > > > > I found that in > > > maven-model-builder/META-INF/sisu/javax.injected.Named > > > > > > file, both interpolators are listed but in reverse order (comparing > > > it > > > > > > > > > > with > > > > > > > > > > > the same file in 3.6.2), that is StringSearchModelInterpolator > > appear > > > > > > > > > > first > > > > > > > > > > > in the file and StringVisitorModelInterpolator second. > > > > > > > > > > > > (I believe the dependency injection picks up the first one it finds > > > with > > > > > > the right type.) > > > > > > > > > > > > Deleting the @Named annotation from StringSearchModelInterpolator > > > solved > > > > > > the issue, as this make the entry disappear from the > > > > > > javax.injected.Named > > > > > > file. Then the dependency injection uses the > > > > > > StringVisitorModelInterpolator. > > > > > > > > > > > > Can anyone confirm this issue? > > > > > > And if so, how to best prevent in the future that this type of > > > > > > dependency > > > > > > injection regressions from happening? > > > > > > > > > > > > Kind regards, > > > > > > Gabriel > > > > > > > > > > > > > > > > > > --------------------------------------------------------------------- > > > 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
