-
Notifications
You must be signed in to change notification settings - Fork 451
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
test: adding java 7 check #1847
Conversation
It turned out that Maven wrapper does not work with Java 7.
|
3.6.3 failed: https://github.com/googleapis/google-http-java-client/actions/runs/4919311091/jobs/8786764114?pr=1847
|
Memo: Maven 3.5.x does not have "-ntp" option. |
3.5.4 still failed:
Where is it coming from? com/fasterxml/jackson/databind/ObjectMapper |
|
Using Maven 3.6.3,
|
After commenting out "nexus-staging-maven-plugin", now the error is on os-maven-plugin:
After that, now it's enforcer plugin:
Note that https://gist.github.com/suztomo/af1d49b2e4cebb57246dbbf5438d30d8 maven-javadoc-plugin too.
Maven resource plugin too.
|
Maven-surefire-plugin has "jvm" option https://maven.apache.org/surefire/maven-surefire-plugin/test-mojo.html#jvm. However, when I used it
|
It finally ran https://gist.github.com/suztomo/270f6fb08ed7b8683d283a3f88f6d0e8:
The surefire plugin version is set to 2.22.2. |
Warning: This pull request is touching the following templated files:
|
-Dclirr.skip=true -Denforcer.skip=true -Dmaven.javadoc.skip=true \ | ||
-Dgcloud.download.skip=true -T 1C \ | ||
-Dproject.surefire.version=2.22.2 \ | ||
-Djvm=${JAVA7_HOME}/bin/java |
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.
Memo: This is the core of this enhancement. https://maven.apache.org/surefire/maven-surefire-plugin/test-mojo.html#jvm
@@ -581,6 +581,7 @@ | |||
<project.httpcore.version>4.4.16</project.httpcore.version> | |||
<project.opencensus.version>0.31.1</project.opencensus.version> | |||
<project.root-directory>..</project.root-directory> | |||
<project.surefire.version>3.0.0-M7</project.surefire.version> |
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.
By moving the version value to the property, we can set it via -Dproject.surefire.version=2.22.2
.
Guava's warning about Java 7 tells that the test is indeed running on Java 7: |
- name: Set up Maven | ||
uses: stCarolas/[email protected] | ||
with: | ||
maven-version: 3.8.8 |
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.
I believe this is the last version of maven that supports Java 7, right? I don't think anyone is going to manually update this, but would it helpful to add a blurb as to why this set to 3.8.8
?
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.
Actually this Maven does not need to work for Java 7. In fact this check does not run Maven via Java 7. (Maven calls Java 7 when running surefire unit tests)
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.
LGTM
Using the same method as googleapis/google-http-java-client#1847 to use surefire's jvm system property
Using the same method as googleapis/google-http-java-client#1847 to use surefire's jvm system property
We haven't run the Java 7 builds for a while even though the README.md says Java 7 target. Let's clarify the current situation by installing the check again.
It turned out that the google-http-client-jackson2 and google-http-client-appengine modules require Java 8 or higher due to their dependencies. We keep the compilation target for Java 7 so that the users can tweak some old version of the dependencies.
The surefire (unit test) plugin's "jvm" option is the core of this Java 7 check. Guava's warning about Java 7 tells that the test is indeed running on Java 7:
https://github.com/googleapis/google-http-java-client/actions/runs/4927699269/jobs/8805047743?pr=1847
Fixes #1713
Closes #1721