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

Build for Java 8 rather than Java 7. #2744

Merged
merged 2 commits into from
Sep 23, 2024
Merged

Conversation

eamonnmcmanus
Copy link
Member

@eamonnmcmanus eamonnmcmanus commented Sep 18, 2024

Closes #2743.

@eamonnmcmanus eamonnmcmanus added the java8 Issues related to making Java 8 the minimum supported version label Sep 18, 2024
Copy link
Member

@cpovirk cpovirk left a comment

Choose a reason for hiding this comment

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

A few notes, mostly on the Android front:

  • I see that you already have an Animal Sniffer check for Android compatibility. We seem to be getting good results in Guava from the https://github.com/open-toast/gummy-bears signatures that you mention, the ones that assume only the always-on desugaring of a smallish set of methods. Those are safe to use even if your users don't opt in to the more extensive feature that we normally refer to as "library desugaring," so you might have a look if javac starts generating usages of those methods automatically(?) or if you just want a smattering of Java 8 goodies.
  • I don't see any issues with conditional support for Optional and Instant and friends. Once you've checked that the classes exist (see sub-bullet), you'll now be able to write normal Java code to operate on those classes. Android doesn't even mind if you have random methods in a class with Optional and Instant in the signature (in contrast to Java, which would, IIUC) as long as you don't call them.
    • I imagine the best way to guard that support will be with Java reflection that looks up those classes. I would hope that that would support both the case of Android versions new enough to support the classes directly and the case of desugaring (which I hope is smart enough to rewrite strings used in reflection). I have made the mistake before of just trying to use the classes and then catching the resulting error, but I have learned one or two (see also internal thread "Android and java7 Futures.getChecked()" about log spam) lessons about that, at least one of which is unlikely to be news to you :)).
  • I have seen a handful of problems when experimenting with adding default methods in Guava inside Google. (See bug 229266760.) One of them is a result of a system that ill-advisedly tries to use a mix of the internal Guava and the external Guava (with the external Guava still lacking the default method), but I'm not sure I ever had a theory for the other. Anyway, adding the default methods would probably work fine, but I'd advise factoring in at least a little possibility that it turns out not to.
  • Lambda and method references should be entirely fine :) Maybe also remove some unnecessary final keywords from effectively final variables if you have any of those hanging around.
  • Kudos on actually using --release to avoid ByteBuffer issues (not that GSON appears to use the class) and other horrors.

Copy link
Collaborator

@Marcono1234 Marcono1234 left a comment

Choose a reason for hiding this comment

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

If you want you can also adjust the build setup to properly support JDK 21, since the only blocking thing was using Java 7 as target. So basically your changes from #2690, except:

  • Omit the JAPICMP-OLD changes (that has already been solved)
  • In build.yml also remove ${{ matrix.extra-mvn-args || '' }}
  • In the README, adjust

    JDK 17 is recommended. Newer JDKs are currently not supported for building

This would then close #2501

Or alternatively I can also do that in a separate PR if you prefer.


It seems there are also a few comments in the code suggesting refactoring for Java 8:

@eamonnmcmanus
Copy link
Member Author

OK, I'm proposing to merge this change as is and address any comments in followups. @Marcono1234 if you want to send PRs for the items you mention, I would certainly appreciate that.

@eamonnmcmanus eamonnmcmanus merged commit 58eaac3 into google:main Sep 23, 2024
11 checks passed
@eamonnmcmanus eamonnmcmanus deleted the java8 branch September 23, 2024 04:01
@Marcono1234 Marcono1234 mentioned this pull request Sep 23, 2024
9 tasks
@cpovirk
Copy link
Member

cpovirk commented Sep 30, 2024

(I forgot to mention that Animal Sniffer will be upset about any appearance of Optional or Instant in a signature or method body. We suppress similar problems in Guava. Another alternative would be to investigate using multi-release jars, except for the part that that wouldn't actually help on Android.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
java8 Issues related to making Java 8 the minimum supported version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Java 8 the minimum supported version
3 participants