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

Investigate and fix kogito-maven-plugin usage inside kogito-runtimes project itself #1460

Open
gitgabrio opened this issue Sep 2, 2024 · 9 comments · May be fixed by apache/incubator-kie-kogito-apps#2135 or apache/incubator-kie-kogito-runtimes#3757
Assignees

Comments

@gitgabrio
Copy link

gitgabrio commented Sep 2, 2024

kogito-maven-plugin is used to build springboot-based kogito projects.
It contains three mojos and a couple of configurations to drive maven life-cycle.
With that in place, its usage requires the
<extensions>true</extensions> flag (see documentation)
The problem with this approach is that if another module, inside kogito-runtimes, need it, the mvn reactor tries to resolve it during initial pom parsing and, since the plugin has not been built, yet, the initial pom parsing fails.
This became evident after refactoring of springboot integration tests, that relies on it.
The issue is also hidden in our CI for some unclear reason, when using the usual "999-SNAPSHOT" version, but it become evident when the version is changed.

The most straightful solution would be to rewrite/modify the plugin to not depend on its customized lifecycle, so that it can be used without the <extensions>true</extensions> flag, but there could be other problems, so this need investigation.

As further indication, here's the execution order with <extensions>true</extensions> flag:

--------------------------------------------------------------------------------------------------------------
PLUGIN                        | PHASE                  | ID                            | GOAL                 
--------------------------------------------------------------------------------------------------------------
maven-enforcer-plugin         | validate               | enforce-versions              | enforce              
maven-enforcer-plugin         | validate               | enforce-maven-version         | enforce              
maven-enforcer-plugin         | validate               | enforce-java-version          | enforce              
maven-checkstyle-plugin       | validate               | default                       | check                
formatter-maven-plugin        | process-sources        | default                       | format               
impsort-maven-plugin          | process-sources        | default                       | sort                 
maven-remote-resources-plugin | generate-resources     | process-resource-bundles      | process              
maven-resources-plugin        | process-resources      | default-resources             | resources            
maven-compiler-plugin         | compile                | default-compile               | compile              
kogito-maven-plugin           | compile                | default-generateModel         | generateModel        
maven-compiler-plugin         | compile                | default-compile-1             | compile              
kogito-maven-plugin           | process-classes        | default-process-model-classes | process-model-classes
maven-resources-plugin        | process-test-resources | default-testResources         | testResources
...        

and this is with <extensions>false</extensions> flag:

----------------------------------------------------------------------------------------------------
PLUGIN                        | PHASE                  | ID                       | GOAL            
----------------------------------------------------------------------------------------------------
maven-enforcer-plugin         | validate               | enforce-versions         | enforce         
maven-enforcer-plugin         | validate               | enforce-maven-version    | enforce         
maven-enforcer-plugin         | validate               | enforce-java-version     | enforce         
maven-checkstyle-plugin       | validate               | default                  | check           
formatter-maven-plugin        | process-sources        | default                  | format          
impsort-maven-plugin          | process-sources        | default                  | sort            
maven-remote-resources-plugin | generate-resources     | process-resource-bundles | process         
maven-resources-plugin        | process-resources      | default-resources        | resources       
maven-compiler-plugin         | compile                | default-compile          | compile         
maven-resources-plugin        | process-test-resources | default-testResources    | testResources
...    

@baldimir @elguardian @fjtirado @porcelli

@gitgabrio
Copy link
Author

gitgabrio commented Nov 5, 2024

After first iteration, it turns out that the root cause of that "workaround" is the usage of reflection on generated classes to code-gen other classes (see here)
So the proper fix is to completely remove allow both the usage of reflections or direct source scan, depending on the context, thus, removing the need of double compilation (and of that workaround as whole).
Double check if also drools/rules is affected by that.

@elguardian

@fjtirado
Copy link

fjtirado commented Nov 5, 2024

After first iteration, it turns out that the root cause of that "workaround" is the usage of reflection on generated classes to code-gen other classes (see here) So the proper fix is to completely remove the usage of reflections and, then the need of double compilation (and of that workaround as whole). Double check if also drools/rules is affected by that.

@elguardian

If I recall correctly, this reflection is not performed over generated classes, but performed on user provided classes to generate the bridge to call them.

@elguardian
Copy link

@fjtirado the problem lies when you have a service task (pojo) defined in the src where the plugin is being executed. In those cases the codegen cannot find it. (as generate-sources) it is executed before the compilation hence the problem with the lifecycle we had. You have an example of this in here:
https://github.com/apache/incubator-kie-kogito-runtimes/blob/main/jbpm/jbpm-flow-builder/src/main/java/org/jbpm/compiler/canonical/descriptors/ServiceTaskDescriptor.java#L143-L159

@fjtirado
Copy link

fjtirado commented Nov 5, 2024

@elguardian yes, Im skeptical about the possibility of removing such reflection. We need to analyze the classes on src to be able to generate proper code to call them

@elguardian
Copy link

elguardian commented Nov 5, 2024

@fjtirado Yeah.. it is not possible to remove the reflections. So what I pointed out is to fix the use case mentioned above.
For that class for instance we would required to have an instrospection base class to get information about a JavaClass or Source code. One will get the information from the class path, and the other one will get the information from source code.
So you are right being skeptical. We are not removing, we are adding.

@fjtirado
Copy link

fjtirado commented Nov 5, 2024

@elguardian Yes, I misunderstood @gitgabrio I read "over" generated classes, when he wrote "on", probably it should have been "in" (communication in non native language is a ...)
I think we are all aligned, Im going to scan the code for more possible usages of reflection to generate classes during "build" maven phase.

@fjtirado
Copy link

fjtirado commented Nov 5, 2024

Other cases we have are just for quarkus (OpenApi for sonataflow) and we are using Jandex to introspect the user class.

@gitgabrio
Copy link
Author

@elguardian @fjtirado I updated my comment.
Please let me know if you find other usages. My current approach is to debug the kogito-maven-plugin to see when/how it invokes reflection during codegen.

@gitgabrio
Copy link
Author

@elguardian @fjtirado
I've investigated a bit more deeper.

  1. rules requires pre-compiled classes: if a drl file refers to some java bean, this has to be already compiled to be found, otherwise drl compilation fails
  2. while it could be "simpler" to remove pre-compile need for process, this seems extremely hard for rules; and since rules and processes could (and often does) live in the same project, point 1. prevent removal of pre-compile phase
  3. what could be improved, is that instead of invoking maven compiler to re-compile "blindly" everything, we could manually compile and dump only the newly generated classes
  4. the other thing that could be refactored is the ProcessClassesMojo that, IIUC, uses Reflection (and precompiled classes) to code-generate persistence-related classes, that later on are manually compiled and dumped.

To recap, I think we can't escape from "pre-compile" (as happen in the kie-maven-plugin) but we can provide that code-generation happen only once soon after, and that later only newly code-generated sources will be compiled/dumped.

I add @mariofusco to also have his opinion on that, especially about the rule part

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 📋 Backlog
3 participants