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-8254] Fix registration of DI-powered beans #1715

Closed
wants to merge 2 commits into from

Conversation

jonasrutishauser
Copy link
Contributor

@jonasrutishauser jonasrutishauser commented Sep 10, 2024

JIRA issue: https://issues.apache.org/jira/browse/MNG-8254
IT PR: apache/maven-integration-testing#358

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

@gnodet
Copy link
Contributor

gnodet commented Sep 11, 2024

@jonasrutishauser the changes cause stack overflow errors:

Caused by: java.lang.StackOverflowError
	at java.base/java.lang.ReflectiveOperationException.<init>(ReflectiveOperationException.java:90)
	at java.base/java.lang.reflect.InvocationTargetException.<init>(InvocationTargetException.java:67)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at com.google.inject.internal.DelegatingInvocationHandler.invoke(DelegatingInvocationHandler.java:50)
	at jdk.proxy2/jdk.proxy2.$Proxy104.provides(Unknown Source)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at com.google.inject.internal.DelegatingInvocationHandler.invoke(DelegatingInvocationHandler.java:50)
	at jdk.proxy2/jdk.proxy2.$Proxy104.provides(Unknown Source)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at com.google.inject.internal.DelegatingInvocationHandler.invoke(DelegatingInvocationHandler.java:50)

@jonasrutishauser
Copy link
Contributor Author

There are a lot of changes regarding DI now on the master branch which cause this stack overflow.
I have to check again and see what is the current state.

@gnodet
Copy link
Contributor

gnodet commented Sep 13, 2024

There are a lot of changes regarding DI now on the master branch which cause this stack overflow. I have to check again and see what is the current state.

I'm actually looking into the SisuBridge, so give me some the day to look at it...

@gnodet
Copy link
Contributor

gnodet commented Sep 13, 2024

There are a lot of changes regarding DI now on the master branch which cause this stack overflow. I have to check again and see what is the current state.

I'm actually looking into the SisuBridge, so give me some the day to look at it...

This may be fixed with #1722

@jonasrutishauser
Copy link
Contributor Author

I found the problem.

I'm not sure why the interfaces are registered in org.apache.maven.di.impl.InjectorImpl.bindImplicit() but I filter them out in the Sisu registration.

@gnodet
Copy link
Contributor

gnodet commented Sep 20, 2024

@jonasrutishauser could have another look at the master branch, now that #1722 has been merged ?

Also, what's your use case for providing an alternative ModelBuilder ? I do intend to push more into the builder (and less in the project builder, see #1700), so it may become a bit complex, but maybe we can offer more SPI here ?

@jonasrutishauser
Copy link
Contributor Author

@jonasrutishauser could have another look at the master branch, now that #1722 has been merged ?

I think my problem was resolved when I tested after your merge (with the current master branch my test will no longer work as there are to many changes in project building).

Also, what's your use case for providing an alternative ModelBuilder ? I do intend to push more into the builder (and less in the project builder, see #1700), so it may become a bit complex, but maybe we can offer more SPI here ?

Our use case is that we want to add additional subprojects using an extension maven plugin.
We have a code generate which generates several maven modules from a source file in a company specific dsl.

This was possible with maven 3 as most objects where mutable and by implementing AbstractMavenLifecycleParticipant.afterProjectsRead() which updates the project list in the maven session.

If I look at the current master branch there is a comment which would cover our use case:

// subprojects discovery
if (getSubprojects(model).isEmpty()
// only discover subprojects if POM > 4.0.0
&& !MODEL_VERSION_4_0_0.equals(model.getModelVersion())
// and if packaging is POM (we check type, but the session is not yet available,
// we would require the project realm if we want to support extensions
&& Type.POM.equals(model.getPackaging())) {

@gnodet
Copy link
Contributor

gnodet commented Oct 1, 2024

@jonasrutishauser could have another look at the master branch, now that #1722 has been merged ?

I think my problem was resolved when I tested after your merge (with the current master branch my test will no longer work as there are to many changes in project building).

Also, what's your use case for providing an alternative ModelBuilder ? I do intend to push more into the builder (and less in the project builder, see #1700), so it may become a bit complex, but maybe we can offer more SPI here ?

Our use case is that we want to add additional subprojects using an extension maven plugin.

We have a code generate which generates several maven modules from a source file in a company specific dsl.

This was possible with maven 3 as most objects where mutable and by implementing AbstractMavenLifecycleParticipant.afterProjectsRead() which updates the project list in the maven session.

If I look at the current master branch there is a comment which would cover our use case:

// subprojects discovery
if (getSubprojects(model).isEmpty()
// only discover subprojects if POM > 4.0.0
&& !MODEL_VERSION_4_0_0.equals(model.getModelVersion())
// and if packaging is POM (we check type, but the session is not yet available,
// we would require the project realm if we want to support extensions
&& Type.POM.equals(model.getPackaging())) {

Right, if you generate files on the fly, that will work. But you still need a way to call your code, so maybe a better alternative would be to register a ModelTransformer, as those are called before validating the file, raw and effective models, and allow changing the models at those points.

@gnodet
Copy link
Contributor

gnodet commented Oct 1, 2024

Duplicates MNG-8220 and #1683

@gnodet gnodet closed this Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants