Goal 'compile-jsp' uses wrong source/target

bmarwell commented 6 months ago

Hi,

when migrating to 11/17, we caught an error in the CompileJspMojo logic:

https://github.com/OpenLiberty/ci.maven/blob/9b96fb6bd9190eebd4cc2928639ba86f18702135/liberty-maven-plugin/src/main/java/io/openliberty/tools/maven/jsp/CompileJspMojo.java#L94

Potential issues

it tries to read "source" from the maven war plugin. However:

Source Level and Target level are two different things - I do not see a target level specified
maven.compiler.release seems ignored which is the better option, if set.
It completely ignores the server.xml properties which are both valid: <jspEngine javaSourceLevel="21" /> or <jspEngine jdkSourceLevel="18" />

While both work, the current docs only mention "javaSourceLevel" to avoid confusion: jdkSourceLevel uses "18" for "1.8" or simply "8" and does not go beyond that.

Recommended changes

use server.xml (location: configurable) when present. a. prefer javaSourceLevel b. then the next best option is jdkSourceLevel
Otherwise use pom.xml settings a. Prefer "release" paramter of Maven compiler Plugin b. Otherwise use source (as of now)

Other changes

"setSource" does not seem to take modern java versions as a parameter: https://github.com/OpenLiberty/ci.ant/blob/d943de9153088c5c39cdf2ea5b950466c2975a09/src/main/java/io/openliberty/tools/ant/jsp/CompileJSPs.java#L491 This would need to get fixed first.
Documentation update needed: https://github.com/OpenLiberty/ci.maven/blob/main/docs/compile-jsp.md#compile-jsps
What about internal mechanisms? Does the plugin actually update the web.xml file as other plugins do? Do JSP files compiled with compile-jsp work on other application servers or does it compile against the OpenLiberty API?

Breaking changes

Potentially fixes builds :) Previously, you could not really set the target properly.
source should have been target all along, as far as I can tell

Other things to be aware of

I found that jsp-level (2.2/2.3) could have been read from server.xml as well. You might want to open another ticket for this.

cherylking commented 6 months ago

@bmarwell Thank you for bringing this to our attention. We will have to look into this further in order to answer your specific questions.

cherylking commented 6 months ago

It completely ignores the server.xml properties which are both valid: <jspEngine javaSourceLevel="21" /> or

@bmarwell Was there a second part to this comment?

scottkurz commented 6 months ago

It completely ignores the server.xml properties which are both valid: <jspEngine javaSourceLevel="21" /> or

@bmarwell Was there a second part to this comment?

I fixed the formatting to reveal this..

cherylking commented 6 months ago

Related issue we may want to consider when fixing this issue - #1467

bmarwell commented 6 months ago

Thanks for fixing the formatting. Quick question: can I somehow use another plugin to compile JSPs?

cherylking commented 6 months ago

Quick question: can I somehow use another plugin to compile JSPs?

I don't think so. I have a fix up for review and can create a snapshot by EOB today. Will you be able to test it @bmarwell ?

cherylking commented 6 months ago

Does the plugin actually update the web.xml file as other plugins do?

No, it only copies the compiled jsp class files into the application binary.

Do JSP files compiled with compile-jsp work on other application servers or does it compile against the OpenLiberty API?

It compiles with the jsp-2.2 / jsp-2.3 feature from OpenLiberty.

bmarwell commented 6 months ago

Quick question: can I somehow use another plugin to compile JSPs?

I don't think so. I have a fix up for review and can create a snapshot by EOB today. Will you be able to test it @bmarwell ?

Sadly no, I don't have access to the source code of that app. I'll check with my colleagues tomorrow if they can do it.

cherylking commented 6 months ago

The fix is in the 3.10.3-SNAPSHOT. If I don't receive any feedback on the fix, I will go ahead and publish a minor fix release including the fix on Wednesday most likely.

bmarwell commented 6 months ago

The fix is in the 3.10.3-SNAPSHOT. If I don't receive any feedback on the fix, I will go ahead and publish a minor fix release including the fix on Wednesday most likely.

will build and distribute here momentarily

cherylking commented 6 months ago

@bmarwell I saw some feedback in the support case that maven.compiler.source no longer works? I have a test case specific for that and it is passing. Can the developer of the app post a pom.xml snippet of how the maven.compiler.source is getting specified so I can try to recreate?

Couple of points:

We will not be adding logic to get the info from the server.xml.
Only LTS releases are supported by the javaSourceLevel attribute on the jspEngine element. So if something other than a LTS release is specified in the maven.compiler.source or maven.compiler.release, the compile-jsp goal is not going to work correctly. Do we need to add hard-coded logic to adjust the java version specified to the nearest LTS? For example, if you specify maven.compiler.release = 22 should we automatically use javaSourceLevel="21"? This seems like it would be problematic which is why I did not address it in my PR.

OpenLiberty / ci.maven