Fix multi-module jar skip for spring-boot projects #1810

awisniew90 · 2024-04-11T16:49:40Z

No description provided.

cherylking

Just the one question.

liberty-maven-plugin/src/main/java/io/openliberty/tools/maven/ServerFeatureSupport.java

scottkurz

I think it's worth adding an IT here. We have the whole structure in src/it/dev-it/src/test/java/net/wasdev/wlp/test/dev/it... and the validation can be pretty simple to simply check that it wasn't skipped.

awisniew90 · 2024-04-15T17:39:20Z

I think it's worth adding an IT here. We have the whole structure in src/it/dev-it/src/test/java/net/wasdev/wlp/test/dev/it... and the validation can be pretty simple to simply check that it wasn't skipped.

So basically run "liberty:dev" against a spring boot app. I only see two existing spring boot apps in the test source here, and none in the dev it structure. So we'd need to create a Spring Boot app in dev-it and validate that we are not skipping the execution.

scottkurz · 2024-04-15T18:47:39Z

...y-maven-plugin/src/it/dev-it/src/test/java/net/wasdev/wlp/test/dev/it/SpringBootDevTest.java

+public class SpringBootDevTest extends BaseDevTest {
+
+   @BeforeClass
+   public static void setUpBeforeClass() throws Exception {


You have to do more like mvn package liberty:dev.

That said.. I think it might be a bad idea to include this test.. since someone later might assume it implies that we have support for dev mode in Spring Boot which we don't.

This would fit better as a test of 'run'. You still have to do a package with 'run' (so mvn package liberty:run) but you don't have to use loose apps, and so it makes more sense than dev (since neither loose apps in general nor dev work with SB and LMP currently).

^^ What Scott said :-)

scottkurz · 2024-04-15T18:56:00Z

liberty-maven-plugin/src/main/java/io/openliberty/tools/maven/server/DevMojo.java

@@ -1342,7 +1342,11 @@ private void doDevMode() throws MojoExecutionException {

        	// In a multi-module build, dev mode will only be run on one project (the farthest downstream) and compile will
        	// be run on any relative upstream projects. If this current project in the Maven Reactor is not one of those projects, skip it.  
-        	List<MavenProject> relevantProjects = getRelevantMultiModuleProjects(graph);
+        	boolean skipJars = true;
+        	if("spring-boot-project".equals(getDeployPackages())) {


At this point, is this just an error, independently of whether we think we should skip? Or are we trying to make this correct imagining a future effort to add support for Spring Boot in dev mode?

scottkurz · 2024-04-15T18:57:49Z

liberty-maven-plugin/src/main/java/io/openliberty/tools/maven/server/GenerateFeaturesMojo.java

@@ -119,7 +119,11 @@ private void generateFeatures() throws MojoExecutionException, PluginExecutionEx

        	// In a multi-module build, generate-features will only be run on one project (the farthest downstream). 
        	// If this current project in the Maven Reactor is not that project or any of its upstream projects, skip it.  
-        	List<MavenProject> relevantProjects = getRelevantMultiModuleProjects(graph);
+        	boolean skipJars = true;


Does "generate-features" do something meaningful today at LMP v3.10, in the Spring Boot JAR case? Are we adding any value at all by complicating the path here?

I doubt we have tested this scenario. So what would perform the most like it did before the changes went in to skip jar modules? Do we want to not skip jar modules at all for now for generate-features?

I think that might cause problems since generate-features is called from dev mode... so in the non-springboot case, we could flag a conflict in generate-features (a jar and a war module without any downstream projects), that we didnt flag in the dev mode execution. So the two need to behave the same in that regard.

Since dev mode is not supported in the Springboot case, should we just add this SB check to the RunServerMojo and not for DevMojo and GenerateFeaturesMojo?

scottkurz · 2024-04-16T17:44:11Z

Sorry, not sure which comment to respond to so starting a new one:

think it might be best to leave things alone and not add enforcement. In general it's nice to fail fast but a higher burden of proof that that'd be good now that it's out there.
So while maybe I'd suggest failing fast for Spring Boot JAR in dev mode.. I'm backing off that.
Another related change we could make it to skip what I'm calling the multi module "reducer" for a single-module case?
So if this returns a one-item list (in getRelevantMultiModuleProjects)

List sortedReactorProjects = graph.getSortedProjects();

let's just return the single module and keep going. This would solve the original case, the SB guide..which is single module.
However, I think the fix Adam coded is better. One could have a JAR->JAR multi-module... and the original problem would remain. Might as well fix it, like the PR now does. Once that is fixed... is it worth also making the change here to short-circuit single module execution? I don't know. Could someone have done something with a JAR package that we aren't thinking of? Do we care about saving a few cycles? Eh..not sure it matters but helpful to have discussed here.

If we liked the hierarchy better, we could move getDeployPackages() to the existing common base ServerFeatureSupport instead of moving GenerateFeaturesMojo in the hierarchy. I don't know the hierarchy looks better either way so no strong opinion.

Signed-off-by: Adam Wisniewski <awisniew@Adams-MacBook-Pro.local>

awisniew90 requested review from cherylking and scottkurz April 11, 2024 16:49

cherylking reviewed Apr 11, 2024

View reviewed changes

liberty-maven-plugin/src/main/java/io/openliberty/tools/maven/ServerFeatureSupport.java Outdated Show resolved Hide resolved

scottkurz requested changes Apr 11, 2024

View reviewed changes

awisniew90 force-pushed the fix-sb-multi branch 2 times, most recently from 33453ac to 5c45421 Compare April 15, 2024 17:54

scottkurz reviewed Apr 15, 2024

View reviewed changes

awisniew90 force-pushed the fix-sb-multi branch from 5c45421 to 54269bc Compare April 17, 2024 17:15

Fix multi-module jar skip for spring-boot projects

746110c

Signed-off-by: Adam Wisniewski <awisniew@Adams-MacBook-Pro.local>

awisniew90 force-pushed the fix-sb-multi branch from 54269bc to 746110c Compare April 18, 2024 18:31

cherylking approved these changes Apr 18, 2024

View reviewed changes

scottkurz approved these changes Apr 18, 2024

View reviewed changes

cherylking merged commit 9b96fb6 into OpenLiberty:main Apr 19, 2024
12 checks passed

cherylking mentioned this pull request Apr 22, 2024

LMP 3.10.2 liberty:run/dev goal skipped using spring-boot and JAR packaging type #1809

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix multi-module jar skip for spring-boot projects #1810

Fix multi-module jar skip for spring-boot projects #1810

awisniew90 commented Apr 11, 2024

cherylking left a comment

scottkurz left a comment

awisniew90 commented Apr 15, 2024

scottkurz Apr 15, 2024

cherylking Apr 15, 2024

scottkurz Apr 15, 2024

scottkurz Apr 15, 2024

cherylking Apr 15, 2024

awisniew90 Apr 15, 2024

scottkurz commented Apr 16, 2024 •

edited

Loading

Fix multi-module jar skip for spring-boot projects #1810

Fix multi-module jar skip for spring-boot projects #1810

Conversation

awisniew90 commented Apr 11, 2024

cherylking left a comment

Choose a reason for hiding this comment

scottkurz left a comment

Choose a reason for hiding this comment

awisniew90 commented Apr 15, 2024

scottkurz Apr 15, 2024

Choose a reason for hiding this comment

cherylking Apr 15, 2024

Choose a reason for hiding this comment

scottkurz Apr 15, 2024

Choose a reason for hiding this comment

scottkurz Apr 15, 2024

Choose a reason for hiding this comment

cherylking Apr 15, 2024

Choose a reason for hiding this comment

awisniew90 Apr 15, 2024

Choose a reason for hiding this comment

scottkurz commented Apr 16, 2024 • edited Loading

scottkurz commented Apr 16, 2024 •

edited

Loading