feat: maven plugin enhancement #1050

yanksyoon · 2025-03-21T07:23:23Z

Have you signed the CLA?
Have you added an entry to the changelog (docs/reference/changelog.rst)?

yanksyoon · 2025-03-25T02:23:07Z

tests/integration/plugins/test_maven/mvnw

This file is autogenerated.

yanksyoon · 2025-03-25T02:24:00Z

uv.lock

Added defusedxml lib because the xml lib's parsing method has security issues.

What are the security issues? I think in this case we're parsing the XML on the user's machine so they should not have a vulnerable XML there

jdkandersson

Reviewed the code so far, will leave the rest for the moment since there might be a few changes required

jdkandersson · 2025-03-31T09:32:10Z

craft_parts/plugins/java_plugin.py

+    def _get_system_openjdk_java_major_version(self) -> int:
+        try:
+            system_java_version_output = self._execute("java --version")
+            # Java versions < 8 have syntax 1.<version>


Is this the right place for the comment? Perhaps it is better lower down where the version parsing is happening?

jdkandersson · 2025-03-31T09:38:48Z

craft_parts/plugins/java_plugin.py

+                reason=f"failed to check java version: {err}",
+            ) from err
+        version_output_match = re.match(
+            r"openjdk (1\.(\d+)|(\d+))", system_java_version_output


if I understand the output, I think openjdk (1\.)?(\d+) should work and then you will get the major version in the same group

jdkandersson · 2025-03-31T09:41:27Z

craft_parts/plugins/java_plugin.py

+            2
+        ) or version_output_match.group(1)
+        try:
+            return int(system_java_major_version)


I wonder how you would construct a test that raises a ValueError here since the regex guarantees that you will get only digits?

jdkandersson · 2025-03-31T09:45:13Z

craft_parts/plugins/maven_plugin.py

 from urllib.parse import urlparse

+# defusedxml does not contain library stubs or py.typed marker
+import defusedxml.ElementTree  # type: ignore[import-untyped]


Is this a new dependncy being introduced? Python has XML inbuilt: https://docs.python.org/3/library/xml.etree.elementtree.html

Yup, I've written the rationale for introducing a new dependency as a PR comment on uv.lock file, perhaps I can update the comment to include that information :)

jdkandersson · 2025-03-31T09:48:07Z

craft_parts/plugins/maven_plugin.py

+        if system_java_major_version < project_java_major_version:
+            raise errors.PluginEnvironmentValidationError(
+                part_name=self._part_name,
+                reason="system Java version is less than project Java version."


I think the point here is to give more details to the user so they can fix this. E.g., should we point to the docs here? Or explain more about how to get a different Java version?

jdkandersson · 2025-03-31T10:08:57Z

craft_parts/plugins/maven_plugin.py

+            )
+            project_java_major_version = _parse_project_java_version(effective_pom_path)
+            return _extract_java_version(project_java_major_version)
+        except subprocess.CalledProcessError as err:


Hmm, to me it is a bit confusing how CalledProcessError could be raised here since it isn't directly in the code here. I guess that it could come from the _execute method? In general, I would rewrap the exception though I'm not sure what the style is in this project

jdkandersson · 2025-03-31T10:10:47Z

craft_parts/plugins/maven_plugin.py

+                reason=f"failed to generate effective pom: {err}",
+            ) from err
+        finally:
+            effective_pom_path.unlink(missing_ok=True)


I think this can be skipped if it is in the tmp directory

jdkandersson · 2025-03-31T10:12:55Z

craft_parts/plugins/maven_plugin.py

+    java_version_element: ET.Element | None = root.find(".//{*}java.version")
+    maven_compiler_release_element: ET.Element | None = root.find(
+        ".//{*}maven.compiler.release"
+    )
+    maven_compiler_plugin_release_element: ET.Element | None = None
+    plugins_element = root.find(".//{*}plugins")


What does the * do here please?

This matches the plugins element in all(*) namespaces

I'm not that familiar with it, what is a namepspace in this case? Is it, for example, the name of the project?

It would depend on the project, for spring-boot-initializr projects, the namespace is: http://maven.apache.org/POM/4.0.0.

Here's something that might be relevant as to why the namespace exist.

In general it's a good style to add namespace information to your xml file. So it is good style to add those information to your pom file which can be helpful in particular if you are in tools like Eclipse etc.

ok, makes sense that we would use * because we have no way of knowing what the namespace might be

jdkandersson · 2025-03-31T10:15:54Z

craft_parts/plugins/maven_plugin.py

+def _extract_java_version(java_version_output: str | None) -> int | None:
+    if java_version_output is None:
+        return None
+    match = re.match(r"(1\.(\d+)|(\d+))", java_version_output)


Similar to before, I think you can restructure this regex so that you always get it in the same group

jdkandersson · 2025-03-31T10:17:13Z

craft_parts/plugins/maven_plugin.py

+    return None
+
+
+def _extract_java_version(java_version_output: str | None) -> int | None:


Ok I understand what this does now. Perhaps call it extract major version?

vpa1977 · 2025-03-31T19:58:34Z

craft_parts/plugins/java_plugin.py

+                part_name=self._part_name,
+                reason=f"failed to check java version: {err}",
+            ) from err
+        version_output_match = re.match(


Should we lock in openjdk project? Oracle build outputs java 24 2025-03-18.

Supporting other installations of Java is out of the scope of this PR since there is no package that provides Oracle's Java. I believe that the Ubuntu philosophy is that if we need such packages, we should onboard them :)

The issue is that this check now prevents the user from using the Oracle build of openjdk, when it was allowed previously.

vpa1977 · 2025-03-31T19:59:42Z

craft_parts/plugins/java_plugin.py

+
+    def _get_system_openjdk_java_major_version(self) -> int:
+        try:
+            system_java_version_output = self._execute("java --version")


I think maven plugin uses java from JAVA_HOME rather than the one found on PATH.
Actually the way PATH is built (staging directory first), we will check the version of jlink runtime rather than maven JDK.

Yup, this can be updated after confirming that the project sources are guaranteed to be available during environment validation.

vpa1977 · 2025-03-31T20:06:56Z

craft_parts/plugins/java_plugin.py

 logger = logging.getLogger(__name__)


+class JavaPluginEnvironmentValidator(validator.PluginEnvironmentValidator):


Do we need this validator at all?

Gradle provide toolchain[1] support same as maven[2]. They decouple host jdk used to run the build tool from the build jdk. In this case the only validation that we want to do for maven is if the tool itself can run, e.g. mvn -v succeeds.

If we are running on the full jdk the error is obvious (unsupported -target or -release or compile error for a newer feature, e.g. record).

[1] https://docs.gradle.org/current/userguide/toolchains.html
[2] https://maven.apache.org/guides/mini/guide-using-toolchains.html

I do think we would need this validator if we decide to do project Java version validation during the Environment validation step.
The Java based projects can derive this as base class then check for any project incompatibilities.

Gradle provide toolchain[1] support same as maven[2]. They decouple host jdk used to run the build tool from the build jdk. In this case the only validation that we want to do for maven is if the tool itself can run, e.g. mvn -v succeeds.

I'm not entirely sure this is correct - the feature enhancement includes a check whether the project and build system Java versions are compatible. Since we should have JAVA_HOME environment variable available during the environment validation time, we are able to get the build system Java version. Then, by using the mvn tooling, we can also fetch the project java version that it is intended for. We can now fail-fast during the environment validation step.

With the toolchain feature Gradle will download the required JDK during the build and place it under GRADLE_USER_HOME. Maven will look into ~/.m2/toolchains.xml file and use JDK specified in java.version (i think?).

The check here checks the version of the host Java is greater of equal that the one required by the build, which might be already wrong, e.g. building Java 17 project with Java 21 will fail due to the warning as errors specified.
Worse, host java might not be used in the build at all and we will forbid running the java plugin.

I'm wondering what the need for the environment variable JAVA_HOME might be here in this case.

If we've specified JAVA_HOME with a specific Java version to use and the toolchain downloads a different set of Java version and uses that.

I understand the ("java --version" implementation is wrong but I could still modify this part to detect JAVA_VERSION environment variable (perhaps from the environment builder which sets JAVA_HOME).

building Java 17 project with Java 21 will fail due to the warning as errors specified

I wasn't quite aware of this - does this mean that version check isn't possible / correct in the first place?

I'm wondering what the need for the environment variable JAVA_HOME might be here in this case.

This will define the JDK that runs maven/gradle itself. Forgive me for horrible ascii:

[host] -> [ maven runs with JAVA_HOME JDK ] -> [ compiler runs with Toolchain JDK]

The check for JAVA_HOME jdk is not really necessary - both maven and gradle can run with Java 8, but this is not true for the plugins (e.g. a few gradle plugins are built with -target 17, some with -target 21), so we might have an issue anyway.

I wasn't quite aware of this - does this mean that version check isn't possible / correct in the first place?

Oh, just to give an idea - list of bugs related to Java 17 -> 21 default upgrade: https://bugs.debian.org/cgi-bin/pkgreport.cgi?tag=default-java21;[email protected]

We need to check that major version of JDK used for compilation is the one specified in project properties.

I am trying to say, that we might write a lot of code, but still run into some corner-cases, e.g. javadoc.

I am wondering if we should say that Java plugin validates that there is a JDK in JAVA_HOME or PATH capable of compiling hello world and stop at that?

[host] -> [ maven runs with JAVA_HOME JDK ] -> [ compiler runs with Toolchain JDK]

Ah I see, I understand it better now, thank you!

We need to check that major version of JDK used for compilation is the one specified in project properties.

If the toolchain already downloads/provides the JDK, I'm wondering what is:

the need for project java version compatibility (would it be better that we trust that the toolchain does what it says it will do?)

check for "major version of JDK used for compilation is the one specified in project properties." (or for this point, would you mean we should check as in we should verify that this is the behavior?)

I am wondering if we should say that Java plugin validates that there is a JDK in JAVA_HOME or PATH capable of compiling hello world and stop at that?

From what I understand, we've already got the detection of javac using this method. Would you be able to help me understand how compiling a hello world would verify that the project would be compatible with the java version being provided?

We need to check that major version of JDK used for compilation is the one specified in project properties.

This feels like a post-build validation step and we're trying to achieve it in a pre-condition check.

I'm guessing that the hello world compilation is a check to make sure that all the requirements defined in the project can be verified by running a simple hello world with given project settings. Would this be correct? If so - I feel like we should just let the project build fail. If for some reason the hello world does not compile, I think it wouldn't be safe to assume that it failed because of the Java version incompatibility.

From what I understand, we've already got the detection of javac using this method. Would you be able to help me understand how compiling a hello world would verify that the project would be compatible with the java version being provided?

I think you are right, what we really want to check is that jdk contains the full list of required modules. Compiling kind of proves that we have a "good" image, but a more appropriate check would be java --list-modules.

The comprehensive version check would be something like

- check that we can run maven/gradle with configured plugins (gradle clean | mvn clean) or similar task that forces the project to be configured - check if we are using toolchain plugins: - - if yes, ignore host version. For maven check that toolchain.xml lists existing JDK (optional) - - if no, do the strict match of the major version

vpa1977 · 2025-03-31T20:09:11Z

craft_parts/plugins/maven_plugin.py

+        options = cast(MavenPluginProperties, self._options)
        version = self.validate_dependency(
-            dependency="mvn",
+            dependency="./mvnw" if options.maven_use_mvnw else "mvn",


wishful thinking for the future PR: If we are going down this route it would be nice to provide debug: true flag which would cause mvnDebug to be executed.

vpa1977 · 2025-03-31T20:12:34Z

craft_parts/plugins/maven_plugin.py

+            effective_pom_path.unlink(missing_ok=True)
+
+
+def _parse_project_java_version(effective_pom_path: Path) -> str | None:


We also have -source and -target to check here.

I am wondering if this check is beneficial to the user, as the build will fail with an obvious build error, unless the developer working on snap is not familiar with Java.

My experiment tells me that -source and -target versions are ignored - I've modified:

<properties> <java.version>21</java.version> <maven.compiler.release>17</maven.compiler.release> </properties> ... <plugin> <artifactId>maven-compiler-plugin</artifactId> <configuration> <release>1.8</release> </configuration> </plugin>

And found that non of the -source and -target version impact the final Jar output.

specifying -release overrides -source/-target, but without -release those are taken into the account

Even if release is not specified, iirc -source/-target was ignored. Let me reconfirm this behavior

vpa1977 · 2025-03-31T21:24:49Z

General comment - we have 2 features here:

support for the maven wrapper
java version validation.

Would it be possible to split out PR for the maven wrapper as it can be very easily merged as a separate feature?

yanksyoon added 30 commits March 12, 2025 04:59

feat: check for mvnw executable

3a69a1a

feat: java version compatibility check

a6a6b51

feat: mvnw support

6123a48

chore: refactor dependency validation

61902f2

fix: regex java version matching

3f2b0bd

chore: refactor project java version detection

739ef09

chore: minor refactor changes

b76454c

test: unit tests for environment validation

348984d

test: deduplicate test case and refactor test case ordering

97ad5b0

test: build command test

c1846ea

test: mvnw integration testing

0f1323c

chore: rename use-mvnw to maven-use-mvnw to follow convention

13945a0

test: integration test modify testing dir

3e8e757

docs: add maven plugin explanation

c68e930

docs: add maven plugin java version check

17e6fa3

docs: add maven plugin maven-use-mvnw parameter

4873baf

docs: improve documentation on proxy environment variables

0d069e8

Merge remote-tracking branch 'origin' into feat/maven_plugin_enhancement

dc0d6e1

docs: add changelog for maven plugin

4d234de

fix: lint errors

82f02e8

test: update plugin parameter maven-use-mvnw

1b91a7c

fix: code format

aca4839

chore: use defusedxml to parse xml

41753cd

fix: type hint for lint

06fbc44

fix: use specific error

7bd0ba2

docs: fix underline too short

198baaf

docs: add maven plugin to toctree

5a399f4

docs: spellcheck

3bce0e0

test: fix run dir

c7abc10

fix: lint fix

3a0599e

docs: add changelog

0258755

yanksyoon marked this pull request as ready for review March 24, 2025 05:05

yanksyoon added 8 commits March 24, 2025 06:33

chore: refactor common java validator

edfc3ad

chore: remove unused vars

f8823ff

fix: tests (due to refactor)

5d9cb92

fix: lint

76d7133

test: fix effective pom path

35df972

test: fix java version extract function return type

465e5f0

fix: unlink order

7f133a3

fix: add JavaPluginEnvironmentValidator to wordlist

1ccec22

yanksyoon commented Mar 25, 2025

View reviewed changes

tests/integration/plugins/test_maven/mvnw

Copy link

Member Author

yanksyoon Mar 25, 2025

Choose a reason for hiding this comment

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

This file is autogenerated.

yanksyoon commented Mar 25, 2025

View reviewed changes

Merge remote-tracking branch 'origin' into feat/maven_plugin_enhancement

97ce0d3

jdkandersson reviewed Mar 31, 2025

View reviewed changes

vpa1977 suggested changes Mar 31, 2025

View reviewed changes

		return None


		def _extract_java_version(java_version_output: str \| None) -> int \| None:

		logger = logging.getLogger(__name__)


		class JavaPluginEnvironmentValidator(validator.PluginEnvironmentValidator):

		effective_pom_path.unlink(missing_ok=True)


		def _parse_project_java_version(effective_pom_path: Path) -> str \| None:

feat: maven plugin enhancement #1050

Are you sure you want to change the base?

feat: maven plugin enhancement #1050

Conversation

yanksyoon commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdkandersson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanksyoon Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vpa1977 Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanksyoon Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanksyoon commented Mar 21, 2025 •

edited

Loading

yanksyoon Apr 1, 2025 •

edited

Loading

vpa1977 Apr 1, 2025 •

edited

Loading

yanksyoon Apr 1, 2025 •

edited

Loading

vpa1977 Apr 1, 2025 •

edited

Loading