@Hervé: The patch worked fine. Thanks! @Stuart: In the light that the model interpolator was the only case found, that is, both equally qualifying classes were located inside the same javax.injected.Named file, I wonder if it would be easier to check for ambiguities at generation time, in the sisu-maven-plugin (adding an optional parameter to fail the build if this kind of case is found). WDYT?
Regards, Gabriel El mié., 6 de nov. de 2019 a la(s) 13:39, Herve Boutemy (hbout...@apache.org) escribió: > 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 > >
