Skip to content
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

[MNG-7954] New dependency injection mechanism #1393

Merged
merged 3 commits into from Feb 5, 2024

Conversation

gnodet
Copy link
Contributor

@gnodet gnodet commented Jan 25, 2024

JIRA issue: https://issues.apache.org/jira/browse/MNG-7954

This PR provides a minimalistic dependency injection library (the code is initially based on the ActiveJ project).
This provides

This is currently plugged into the plugin creation for plugins using the Maven 4 API.
It's currently not used for extensions or for the core maven components. This would require extra support and a plexus compatibility layer (which is kinda what we want to get rid of).

@gnodet gnodet changed the title New dependency injection mechanism [Provide a cleaner DI api] New dependency injection mechanism Jan 25, 2024
@gnodet gnodet changed the title [Provide a cleaner DI api] New dependency injection mechanism [MNG-7954] New dependency injection mechanism Jan 25, 2024
Copy link
Contributor

@rmannibucau rmannibucau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong issue for me but I'd like we review these points before merging then if it is ok just go:

  • do we want to copy all part of guice, guess in maven we just need a subtype
  • do we need bindinset notion, guess we can just lookup sets from a type now
  • do we need all these flavors of structures (tuple for ex), using a list wouldnt be dramatic for us I think
  • do we want all these notions in modules/binders

overall idea is to drop all we dont need from guice if possible to maintain the minimum we need

hope it makes sense

@gnodet
Copy link
Contributor Author

gnodet commented Jan 25, 2024

No strong issue for me but I'd like we review these points before merging then if it is ok just go:

  • do we want to copy all part of guice, guess in maven we just need a subtype

There's no guice dependency here.

  • do we need bindinset notion, guess we can just lookup sets from a type now
  • do we need all these flavors of structures (tuple for ex), using a list wouldnt be dramatic for us I think
  • do we want all these notions in modules/binders
    overall idea is to drop all we dont need from guice if possible to maintain the minimum we need

Agreed. There are a bunch of things that are unneeded to us. That's because the library has been designed with programmatic definition in mind (using the ModuleBuilder DSL) while we need a declarative definition. So I do plan to trim down to what we really need.
But that's just a first draft and I need to try and plug this DI engine into plugin loading mechanism and see how it goes. If I can port a few core plugins to this DI engine, I'll then start trimming it down.

@rmannibucau
Copy link
Contributor

There's no guice dependency here.

Well some part of the code were highly inspired from Guice, this is what I meant.

So I do plan to trim down to what we really need.

Perfect for me. Thanks a lot!

@cstamas
Copy link
Member

cstamas commented Jan 26, 2024

Am just worried that we end up again as we did with old plexus: was our, but began to rot from one point... I'd be happies if we could simply circumvent this by something like eclipse/sisu.inject#103

@gnodet
Copy link
Contributor Author

gnodet commented Jan 26, 2024

Am just worried that we end up again as we did with old plexus: was our, but began to rot from one point... I'd be happies if we could simply circumvent this by something like eclipse/sisu.inject#103

Feel free to give it a try, but I could not find an easy way without rewriting everything...

@cstamas
Copy link
Member

cstamas commented Jan 26, 2024

Do we want something like @Description is? Or it was added to Sisu only as Plexus XML had "component description"? In fact, am unaware of any use of it, but still...

@rmannibucau
Copy link
Contributor

Think we should start minimalistic, maven IoC need is actually pretty low - mainly bean definition + overriding - so less we keep better it is in terms of maintenance.
Rewriting from scratch gives the opportunity to drop guice and sisu at some point too so can be quite promishing if we manage to make plexus-world optional (realm = TCCL) but this is likely mvn 6 or 7 ;).

@gnodet
Copy link
Contributor Author

gnodet commented Jan 26, 2024

Think we should start minimalistic, maven IoC need is actually pretty low - mainly bean definition + overriding - so less we keep better it is in terms of maintenance. Rewriting from scratch gives the opportunity to drop guice and sisu at some point too so can be quite promishing if we manage to make plexus-world optional (realm = TCCL) but this is likely mvn 6 or 7 ;).

Yeah, I think I'll even drop support for scopes for now, we don't really need it for plugins. Those are only needed when dealing with extensions or maven-core, as plugins are always bound to the mojo execution.

@cstamas
Copy link
Member

cstamas commented Jan 26, 2024

Unless, as you mention, is an extension... and we do have quite some of those.

@rmannibucau
Copy link
Contributor

Yes think extensions are in scope there and don't think scope is the biggest code (maybe guice made it verbose but the CDI way is actually quite light - Context usage in subclass proxies which can be done at build time, even if it is just dumping asm one at build time and not using an annot proc or sthg more build oriented). But if we want to split we can maybe start by param injection only.

@gnodet gnodet marked this pull request as ready for review January 29, 2024 10:26
@gnodet gnodet force-pushed the dependency-injection branch 3 times, most recently from 950921f to 5b10ca5 Compare January 29, 2024 10:31
@gnodet gnodet added this to the 4.0.0-alpha-13 milestone Jan 30, 2024
Copy link
Member

@cstamas cstamas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nothing to add really

@gnodet gnodet merged commit a37cf3d into apache:master Feb 5, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants