Pin compilation to JDK8 via Maven Toolchains#617
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #617 +/- ##
=========================================
Coverage 65.38% 65.38%
Complexity 211 211
=========================================
Files 34 34
Lines 988 988
Branches 142 142
=========================================
Hits 646 646
Misses 290 290
Partials 52 52 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The maven-toolchains-plugin requires JDK 8 to be declared in ~/.m2/toolchains.xml. The smoke test Docker agent has Corretto 8 installed but was missing this file, causing the build to fail.
|
|
||
| # Declare JDK 8 in toolchains.xml so maven-toolchains-plugin can resolve it | ||
| RUN mkdir -p /root/.m2 && \ | ||
| printf '<?xml version="1.0" encoding="UTF-8"?>\n<toolchains>\n <toolchain>\n <type>jdk</type>\n <provides>\n <version>8</version>\n </provides>\n <configuration>\n <jdkHome>/usr/lib/jvm/java-1.8.0-amazon-corretto</jdkHome>\n </configuration>\n </toolchain>\n</toolchains>\n' > /root/.m2/toolchains.xml |
There was a problem hiding this comment.
Two points:
-
Minor nit -> hardcoding the jdkHome path -> Unlikely, but possible that base image changes where it installs Corretto 8 (different version suffix or directory). Then this would break as the path won't resolve - is there a better way to ensure coupling?
-
Readability of the printf line -> You can use a heredoc (
<<) instead of a one-line unformatted xml string. That way the xml would appear properly formatted
Functionally, everything looks okay though
rudraroop
left a comment
There was a problem hiding this comment.
Looks okay. Only one actionable feedback for readability
| <build> | ||
| <plugins> | ||
| <plugin> | ||
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-toolchains-plugin</artifactId> | ||
| <version>3.2.0</version> | ||
| <configuration> | ||
| <toolchains> | ||
| <jdk> | ||
| <!-- Range matches both "8" (e.g. actions/setup-java) | ||
| and "1.8" (legacy / hand-written toolchains). --> | ||
| <version>[1.8,9)</version> | ||
| </jdk> | ||
| </toolchains> | ||
| </configuration> | ||
| <executions> | ||
| <execution> | ||
| <goals> | ||
| <goal>toolchain</goal> | ||
| </goals> | ||
| </execution> | ||
| </executions> | ||
| </plugin> | ||
| </plugins> | ||
| </build> | ||
|
|
There was a problem hiding this comment.
Nit: i see this is repeated across all the pom files, is it worth considering having a shared parent pom and have all the common stuff in there?
There was a problem hiding this comment.
we have considered this. it might make sense when we automate the deployment, but for now each module is independently versioned and released to Maven Central and the CI builds them in isolation. a parent POM introduces coupling (any parent change needs to be published to maven central before any child module can be released). for this PR it feels safer to keep the modules self contained
|
|
||
| # Declare JDK 8 in toolchains.xml so maven-toolchains-plugin can resolve it | ||
| RUN mkdir -p /root/.m2 && \ | ||
| printf '<?xml version="1.0" encoding="UTF-8"?>\n<toolchains>\n <toolchain>\n <type>jdk</type>\n <provides>\n <version>8</version>\n </provides>\n <configuration>\n <jdkHome>/usr/lib/jvm/java-1.8.0-amazon-corretto</jdkHome>\n </configuration>\n </toolchain>\n</toolchains>\n' > /root/.m2/toolchains.xml |
There was a problem hiding this comment.
I'd create a file and do a copy instead of print if, that is better for maintainability
There was a problem hiding this comment.
Also let's add an example tollchains.xml (toolchains.xml.example) for contributors
There was a problem hiding this comment.
@benrkia I suggested a heredoc for readability instead of having a separate file + copy as it's only 10 lines of xml. What do you think?
There was a problem hiding this comment.
I'd create a file and do a copy instead of print if, that is better for maintainability
I believe that his Dockerfile is built via stdin (
.PHONY: setup-codebuild-agent
setup-codebuild-agent:
docker build -t codebuild-agent \
--build-arg ARCHITECTURE=$(ARCHITECTURE_ALIAS) \
- < test/integration/codebuild-local/Dockerfile.agent
in the Makefile), which means there is no build context to COPY for local files won't work. we would need to refactor the Makefile to use -f with a build context directory to enable that.
right now i have switched to a heredoc with ${JAVA_HOME}, but happy to do the Makefile refactor if you think it would be nicer
There was a problem hiding this comment.
I'm fine with that but let's add an example file and mention that in contribution.md so that people don't get failure because toolchain definition is missing
Combine the maven-release-plugin/SCM/autoPublish config (this branch) with the maven-toolchains-plugin added on main (PR #617) so every module carries both plugins in a single <build> section.
Add .github/workflows/release.yml, a workflow_dispatch pipeline that builds, tests, and publishes a single module to Maven Central in one pinned JDK 8 environment, replacing the manual mvn deploy from a developer workstation. The release step prepares locally, publishes, then pushes the commits and module-scoped tag atomically, so a failed publish never leaves an orphan tag on the remote; a failure path rolls back the runner state. Releases are serialized repo-wide via a shared concurrency group. Signing/publishing secrets are not wired yet, so only the dry-run path (skip_publish) works without credentials for now. Set all module versions to -SNAPSHOT as the release plugin's expected starting point, and merge a duplicate top-level <build> section in the events POM introduced when the toolchains change (PR #617) met the release-plugin config on this branch.
Issue #, if available:
Description of changes:
Add maven-toolchains-plugin to all packages requiring JDK [1.8,9). Builds now fail fast if no matching JDK 8 toolchain is configured, preventing silent issues like missing annotation processor output when building with higher JDKs.
Target (OCI, Managed Runtime, both): both
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.