-
Notifications
You must be signed in to change notification settings - Fork 241
Pin compilation to JDK8 via Maven Toolchains #617
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
Changes from 2 commits
1901fac
4acf4aa
8a3267a
3bdc951
d539586
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,4 +11,8 @@ COPY --from=docker/buildx-bin:latest /buildx /usr/libexec/docker/cli-plugins/doc | |
| ENV PATH="$PATH:/apache-maven/bin" | ||
| RUN mkdir /apache-maven && \ | ||
| curl https://archive.apache.org/dist/maven/maven-3/3.8.7/binaries/apache-maven-3.8.7-bin.tar.gz | \ | ||
| tar -xz -C /apache-maven --strip-components 1 | ||
| tar -xz -C /apache-maven --strip-components 1 | ||
|
|
||
| # 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two points:
Functionally, everything looks okay though
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd create a file and do a copy instead of print if, that is better for maintainability
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also let's add an example tollchains.xml (toolchains.xml.example) for contributors
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I believe that his Dockerfile is built via stdin ( 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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