-
Notifications
You must be signed in to change notification settings - Fork 125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[MRESOLVER-157] Drop ServiceLocator #88
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good do far. Have you tested against Core ITs and Ant Tasks?
@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. |
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/SimpleDigest.java
Outdated
Show resolved
Hide resolved
Another strange thing I just now realized: if you look at |
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/SimpleDigest.java
Outdated
Show resolved
Hide resolved
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. |
Initial change: drops ServiceLocator and Service, drops default ctors, makes injected ctors public, and finally demo tests uses resolver as in maven, using SISU.
9a57469
to
05e3002
Compare
@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. |
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:
These dependencies are coming through |
@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 cstamas#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. |
@mickaelistria, can you help here? |
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. |
@splatch Have you evaluated this PR within OSGi? |
@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. |
@gnodet ping ^ |
how do you use now maven-resolver when not using sisu/guice just as a simple class instantiation? |
@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. |
Superseded by #340 |
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:
Ultimate goal: make components immutable.