Skip to content
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

ICU-22116 Update CI job for ICU4J to use Java 8 instead of Java 7 #2173

Merged
merged 1 commit into from
Sep 6, 2022

Conversation

echeran
Copy link
Contributor

@echeran echeran commented Sep 2, 2022

Checklist
  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22116
  • Required: The PR title must be prefixed with a JIRA Issue number.
  • Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
  • Required: Each commit message must be prefixed with a JIRA Issue number.
  • Issue accepted (done by Technical Committee after discussion)
  • Tests included, if applicable
  • API docs and/or User Guide docs changed or added, if applicable

@echeran
Copy link
Contributor Author

echeran commented Sep 2, 2022

Note: This is a straightforward change from Java 7 to Java 8, and things seem to pass.

I can also update the CI job step that automatically installs Java for us. Newer versions of that Java setup job step want us to also specify the name of the distribution of OpenJDK that we use (not just the version number). If you want, I can do the update of the Java setup step, including that preference, here too. I don't think people had an opinion on the OpenJDK distribution when I asked previously, and given the advice of Adopt OpenJDK users to use Eclipse Temurin OpenJDK, I would just use

cc @mihnita

@markusicu
Copy link
Member

I can also update the CI job step that automatically installs Java for us. Newer versions of that Java setup job step want us to also specify the name of the distribution of OpenJDK that we use (not just the version number). If you want, I can do the update of the Java setup step, including that preference, here too. I don't think people had an opinion on the OpenJDK distribution when I asked previously, and given the advice of Adopt OpenJDK users to use Eclipse Temurin OpenJDK, I would just use

Ok. I didn't actually know that there are multiple "distributions" of OpenJDK... given the explanation, and the GH actions docs, temurin sounds fine.

markusicu
markusicu previously approved these changes Sep 2, 2022
Copy link
Member

@markusicu markusicu left a comment

Choose a reason for hiding this comment

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

lgtm tnx!

Happy to re-approve for more changes, such as the distribution.

We should probably think about some piece of code that would benefit from Java 8, to demonstrate that it actually works. Maybe something in the tests first?

@@ -43,27 +43,6 @@ jobs:
- uses: actions/setup-java@v1
with:
java-version: '8'
Copy link
Member

Choose a reason for hiding this comment

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

I am surprised that this line already said '8' ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Further down in that file we test ICU4J with java-version: '11' and java-version: '16', respectively.

gnrunge
gnrunge previously approved these changes Sep 2, 2022
@echeran echeran dismissed stale reviews from gnrunge and markusicu via 86beab8 September 4, 2022 01:22
@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

@echeran echeran requested a review from markusicu September 4, 2022 01:45
@echeran
Copy link
Contributor Author

echeran commented Sep 4, 2022

We should probably think about some piece of code that would benefit from Java 8, to demonstrate that it actually works. Maybe something in the tests first?

Done. I updated a test using the Java 8 Streams/Functions constructs.

In general, there are tradeoffs of this slightly more functional style, of course. The benefits are more readable code, less code, less code with manual iteration, so less surface area for mistakes (ex: off-by-one errors). The costs would be performance -- you're (probably?) creating and GC'ing a lot of objects. Even though a lot of the Java GCs coming out since Java 8 are tuned to perform better for lots of small objects, to help with Java 8 and the many new FP languages built on the JVM, it makes sense to start making such Java 8 changes by putting them in tests. I don't know what the perf impacts would be for such changes in hot code paths in the main code (ex: would function/closure literals get translated into local variables at compile time, and would that mitigate the risks much?)

Copy link
Member

@markusicu markusicu left a comment

Choose a reason for hiding this comment

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

lgtm / nice! / tnx!

@echeran echeran merged commit 6e3a923 into unicode-org:main Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants