maven-resolver
maven-resolver copied to clipboard
apache
[MRESOLVER-157] Drop ServiceLocator
Initial change: drops ServiceLocator and Service, drops default ctors, makes injected ctors public, and finally demo tests uses resolver as in maven, using SISU.
https://issues.apache.org/jira/browse/MRESOLVER-157
High level changes:
- dropped ServiceLocator and Service ifaces (and their default impl)
- dropped ServiceLocator related tests (those testing ServiceLocator)
- dropped deprecated AetherModule
- components: drop default ctor, make injected ctors public
- change demo to use Resolver using SISU (as in Maven)
Ultimate goal: make components immutable.
@michael-o not yet, will do. But I wanted first to go over resolver by itself... do you happen to know any "dirty" uses of resolver components in Maven maybe?
None with ServiceLocator. furnace-maven-plugin will be broken. but I do t care.
Another strange thing I just now realized: if you look at org.eclipse.aether.impl.guice.AetherModule, bindings are in Singleton scope. But, if you look at corresponding classes, they have @Named annontation only. This means, that SISU will NOT treat them as singletons (but as "prototype")! IMO, this is a mistake, as these components are meant to be singletons...
Another strange thing I just now realized: if you look at
org.eclipse.aether.impl.guice.AetherModule, bindings are inSingletonscope. But, if you look at corresponding classes, they have@Namedannontation only. This means, that SISU will NOT treat them as singletons (but as "prototype")! IMO, this is a mistake, as these components are meant to be singletons...
Indeed! Let's move this to a new PR please. Checked most classes and there is no reason why they should not be reused.
Please note that Maven Core uses this stupid locator too: https://github.com/apache/maven/blob/39641ac803e17360df40288aaeb40ea0c5ccd77d/maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultVersionResolver.java.
@cstamas I have added few more commits here. Please have a look too. Preparing a Maven branch for testing...
LGTM, thanks. As per comment on MRESOLVER-157, I think this should NOT be merged until 1.7.0 is out (as on master.ServiceLocator is already deprecated).
LGTM, thanks. As per comment on MRESOLVER-157, I think this should NOT be merged until 1.7.0 is out (as on master.
ServiceLocatoris already deprecated).
Agreed. This is post 1.7.0. Likely 2.0.0. Remove old cruft.
@cstamas @michael-o I gave a try to this PR and there is one thing which I would still warmly welcome in 2.0. This is separation of sisu and guice code from resolver implementation. In such case resolver classes should not be marked as private package but that's another story.
I believe that below packages are DI implementation specific. I don't think they are really required to get resolver working:
com.google.inject;version="[1.4,2)",
com.google.inject.binder;version="[1.4,2)",
com.google.inject.name;version="[1.4,2)",
org.eclipse.sisu,
These dependencies are coming through AetherModule, which I guess might be used somehow elsewhere. They should be marked as optional (for OSGi users) or AetherModule should be extracted somewhere else. Is it required to stay in impl since there is already component index for sisu (META-INF/sisu)?
@splatch Your proposal is to move the Guice/Sisu binding to a separate module to have more control over for non-DI users?
@splatch Your proposal is to move the Guice/Sisu binding to a separate module to have more control over for non-DI users?
I made one attempt with https://github.com/cstamas/maven-resolver/pull/4 - it makes guice/sisu optional. Sadly it still leaves implementation classes invisible for use under OSGi. Above PR allows all maven-resolver osgi bundles become active, yet it still does not let actual resolver service to be run. I guess we would need to consult this with someone working on m2e to see how they approach this issue (I suppose they are fine as long as sisu stays). Anyhow, I can think of few ways how we could work-around that (sisu index in META-INF consist list of classes available for IoC).
Just to clear up things: after this PR get merged, guice as dependency (for guice users) and sisu as dependency (for sisu users, but it implicitly needs guice as well) is needed, as there is no more "poor mans DI" (service locator) - DI-less mode. Simply put, you need either guice or sisu (hence guice as well) at runtime with resolver. There is STILL possibility to go "third way", and manually wire up things, which I'd not recommend, but in that case you just need to exclude guice and sisu and you are done.
@cstamas I don't mind getting guice/sisu specific implementation classes, however import of these packages should be marked as optional to allow installation of resolver under OSGi without repackaging. Otherwise this bundle will force installation of sisu just to get module resolved.
@michael-o Its been a while since I did but I published my findings in comments since it didn't work. I did test it with vanilla Karaf where guice and sisu are gone.
Sad, we have no OSGI experience to make any reasonable progress. For that reason OSGi support has been dropped in HttpComponents 5 too.
how do you use now maven-resolver when not using sisu/guice just as a simple class instantiation? I bet few projects use it.. Such Jetty as here https://github.com/eclipse/jetty.project/blob/ca8d147ec4de9bc9174594a53f3ff0002c493466/tests/test-distribution/src/main/java/org/eclipse/jetty/tests/distribution/JettyHomeTester.java#L245
@cstamas OSGi comes with a bunch of rules, similar to switching to JPMS if you investigated that bit. On this particular topic, if the packages do not depend directly on sisu / guice, this should be reflected in the generated manifest, but the fact that a DI framework is required at runtime should be reflected somehow too. Also some of the rules are a bit different that the ones followed by maven in general : for example, the usage is to use different packages for the API and for the implementation, so that the API packages can be exported without the implementation being made available. This is of course doable when using non public classes, but this has other drawbacks in OSGi and thus usually not done. I came to like this way of splitting things btw.
