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

Set release option of compileJava to Java 11 #57

Merged
merged 1 commit into from
Sep 1, 2024

Conversation

sdavids
Copy link
Contributor

@sdavids sdavids commented Aug 27, 2024

We could target release 11 due to one usage of Java 9 API:

https://docs.oracle.com/javase/9/docs/api/java/util/Objects.html#requireNonNullElse-T-T-

<redacted>/adr-j/src/main/java/org/doble/commands/CommandConfig.java:13: error: cannot find symbol
import static java.util.Objects.requireNonNullElse;
^
  symbol:   static requireNonNullElse
  location: class Objects
<redacted>/adr-j/src/main/java/org/doble/commands/CommandConfig.java:83: error: cannot find symbol
                authorEmail = requireNonNullElse(authorEmail, "").trim();
                              ^
  symbol:   method requireNonNullElse(String,String)
  location: class CommandConfig
2 errors

if we replace the requireNonNullElse call we could lower it to Java 8.

Java 9-10 were non-LTS releases, so it makes no sense to target them.


Using Java 21 with release 8 will output the following warning though:

warning: [options] source value 8 is obsolete and will be removed in a future release

$ gr releaseJar
$ javap \
  -verbose \
  -classpath \
  build/releases/adr-j.jar \
  $(
    jar -tf build/releases/adr-j.jar | \
    grep '\.class$' | \
    grep -E --invert-match '(module|package)-info' | \
    sed 's/\.class$//'
  ) | \
  grep -o 'major version: [4-9][0-9]' | \
  sed 's/major version: //' | \
  sort -u | \
  xargs -I{} sh -c 'echo "$(( {} - 44 ))"'
5
8
11

The jansi class files are Java 8 bytecode; the picocli ones Java 5; ours Java 11.


The classes in the test class path (compileTestJava) will use the configured toolchain version, i.e. Java 21.

Some test classes use text blocks (Java 17).

Java 17 is EOL and the test classes are not published, so it makes no sense to use the release 17 for compileTestJava.

Signed-off-by: Sebastian Davids <sdavids@gmx.de>
@sdavids
Copy link
Contributor Author

sdavids commented Aug 27, 2024

requireNonNullElse is inside the implementation, so refactoring it to a Java 8 compatible solution would not break any API contracts.

@adoble
Copy link
Owner

adoble commented Sep 1, 2024

Thanks for the research and also PR.

I'm quite happy to merge this .

However, I am concerned that supporting backwards compatibility right back to Java 8 will result in problems down the line (e.g. new functionality that uses other libraries, inability to use new features in Java that could lighten the coding workload). This was the reason for ADR 8.

However, as @wviana mentioned

Big enterprise projects using ADRs are probably not on the cutting edge java version.

So my proposal would be

  1. Use the the latest Java LTS version for development (as per ADR 8)
  2. Provide backwards compatibly for two LTS versions before (e.g developed using Java 21, compatible with Java 11 - exactly as you have done in the PR)
  3. The old compatibility version is signaled as being deprecated. In our current case thus means that version 11 is shown as deprecated
  4. Supersede ADR 8 to show this change.

Any thoughts on this?

@adoble adoble merged commit c1b1c75 into adoble:main Sep 1, 2024
1 check passed
@adoble
Copy link
Owner

adoble commented Sep 1, 2024

@adoble adoble mentioned this pull request Sep 1, 2024
@adoble
Copy link
Owner

adoble commented Sep 1, 2024

Just realized that my comments on the closed PR will not be easy to find, so I have repeated them in issue #58

@sdavids sdavids deleted the users/sdavids/feat/release-11 branch September 1, 2024 19:23
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.

2 participants