[build] Upgrade JDK compile version as 11 #1197
Conversation
|
@leonardBang @wuchong , CC |
wuchong
left a comment
wuchong
left a comment
There was a problem hiding this comment.
- Let's set default java to 11.
- Add a profile for
java8 - Add a daily CI for build and test against
java8profile
loserwang1024
changed the title
loserwang1024
force-pushed
the
jdk11
branch
3 times, most recently
from
837c643 to
c798e31
Compare
done it @wuchong |
|
|
||
| jobs: | ||
| build: | ||
| compile-on-jdk8: |
There was a problem hiding this comment.
Perhaps it can be renamed to 'build-on-jdk8' here to be consistent with 'build-on-jdk11' below？
There was a problem hiding this comment.
In this test, I just want to compile on jdk8 but not run, thus I want to distingush with 'build-on-jdk11' . Because run both jdk 8 and 11 will cost too much time for each code pull request. I add a daily run test on jdk8.
| fail-fast: false | ||
| matrix: | ||
| module: [ core, flink ] | ||
| steps: |
There was a problem hiding this comment.
Could you use a workflow template to reuse the steps with ci.yaml? You can learn this from Flink: https://github.com/apache/flink/blob/997b48340d2aac1de48c0788b4204d660e34cedd/.github/workflows/template.flink-ci.yml#L129
There was a problem hiding this comment.
add a ci-template.yaml
loserwang1024
force-pushed
the
jdk11
branch
4 times, most recently
from
3d15fbe to
995537b
Compare
.github/workflows/ci-template.yaml
Outdated
| distribution: 'temurin' | ||
| - name: Build | ||
| run: | | ||
| mvn -T 1C -B clean install -DskipTests ${{ steps.profile.outputs.maven_profile }} |
There was a problem hiding this comment.
What is the variable ${{ steps.profile.outputs.maven_profile }} used for?
.github/workflows/nightly.yaml
Outdated
| on: | ||
| schedule: | ||
| # Run at 2:00 daily. | ||
| - cron: "0 2 * * *" |
There was a problem hiding this comment.
This CRON is scheduled in UTC time zone, so it's actually 10:00 in UTC+8. I suggest to run at 20:00 UTC, because it's the lowest commit traffic according to the ossinsight
pom.xml
Outdated
|
|
||
| <profiles> | ||
| <profile> | ||
| <id>java8-target</id> |
There was a problem hiding this comment.
We can name this profile java8 for simplicity.
pom.xml
Outdated
| </requireMavenVersion> | ||
| <requireJavaVersion> | ||
| <version>${target.java.version}</version> | ||
| <version>[1.8,)</version> |
There was a problem hiding this comment.
This looks strange because it allows java8 when the target/source version is java11. It's better to keep ${target.java.version} here.
|
I pushed a commit to address the comments and resolve the conflicts. Please take another look @loserwang1024 . |
|
@wuchong LGTM! |
3 participants
Purpose
Linked issue: close #1195
Brief change log
Tests
API and Format
Documentation