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

Figure out when to drop Java 8 support #6614

Open
cpovirk opened this issue Jun 30, 2023 · 3 comments
Open

Figure out when to drop Java 8 support #6614

cpovirk opened this issue Jun 30, 2023 · 3 comments
Labels
P3 package=general type=other Miscellaneous activities not covered by other type= labels

Comments

@cpovirk
Copy link
Member

cpovirk commented Jun 30, 2023

Probably not soon :) I just had another note I wanted to make about this, so now seems like the time to start collecting those notes.

@cpovirk cpovirk added type=other Miscellaneous activities not covered by other type= labels package=general P3 labels Jun 30, 2023
@cpovirk
Copy link
Member Author

cpovirk commented Jul 5, 2023

I think I failed to actually mention the new "note I wanted to make about this," which was:

cpovirk added a commit to google/truth that referenced this issue Jul 24, 2023
Notes:

- The GWT bump requires changing the browser setting from "FF38" to "FF."
- I skipped the `jsinterop-annotations` bump to 2.1.0 because it's built
  with `-target 11`, while we still support 8.
  (Closes #1147)
  (I've noted this in google/guava#6614)
- The command I used is:
  ```
  mvn org.codehaus.mojo:versions-maven-plugin:2.16.0:{update-properties,use-latest-releases} -DgenerateBackupPoms=false -Dexcludes=com.google.guava:guava
  ```
  That tried to update protobuf to a release candidate. I had thought
  that perhaps this was the result of [a protobuf change from "-rc1" to
  "-rc-1"](protocolbuffers/protobuf#6522), but
  I'm wondering if it's actually just that `versions-maven-plugin` (in
  contrast to Dependabot) counts release candidates [as
  releases](https://www.mojohaus.org/versions/versions-maven-plugin/use-latest-releases-mojo.html)?
  I'd have to investigate further and perhaps [use
  `rulesUri`](https://stackoverflow.com/a/46935363/28465). But since we
  normally rely on Dependabot (and I was using `versions-maven-plugin`
  here only "to make things easier"... :)), I'm not going to worry about
  it now.
cpovirk added a commit to google/truth that referenced this issue Jul 24, 2023
Notes:

- The GWT bump requires changing the browser setting from "FF38" to "FF."
- I skipped the `jsinterop-annotations` bump to 2.1.0 because it's built
  with `-target 11`, while we still support 8.
  (Closes #1147)
  (I've noted this in google/guava#6614)
- The command I used is:
  ```
  mvn org.codehaus.mojo:versions-maven-plugin:2.16.0:{update-properties,use-latest-releases} -DgenerateBackupPoms=false -Dexcludes=com.google.guava:guava
  ```
  That tried to update protobuf to a release candidate. I had thought
  that perhaps this was the result of [a protobuf change from "-rc1" to
  "-rc-1"](protocolbuffers/protobuf#6522), but
  I'm wondering if it's actually just that `versions-maven-plugin` (in
  contrast to Dependabot) counts release candidates [as
  releases](https://www.mojohaus.org/versions/versions-maven-plugin/use-latest-releases-mojo.html)?
  I'd have to investigate further and perhaps [use
  `rulesUri`](https://stackoverflow.com/a/46935363/28465). But since we
  normally rely on Dependabot (and I was using `versions-maven-plugin`
  here only "to make things easier"... :)), I'm not going to worry about
  it now.
  ...
  Also, it looks like protobuf 4 might remove the `hasPresence()` method
  that we'd migrated to back in cl/508716698. If so, there's a good
  chance that the protobuf team will fix things for us :) If not, I'm
  assuming that this will relate to
  ["Editions."](https://protobuf.dev/editions/overview/)
cpovirk added a commit to google/truth that referenced this issue Jul 24, 2023
Notes:

- The GWT bump requires changing the browser setting from "FF38" to "FF."
- I skipped the `jsinterop-annotations` bump to 2.1.0 because it's built
  with `-target 11`, while we still support 8.
  (Closes #1147)
  (I've noted this in google/guava#6614)
- The command I used is:
  ```
  mvn org.codehaus.mojo:versions-maven-plugin:2.16.0:{update-properties,use-latest-releases} -DgenerateBackupPoms=false -Dexcludes=com.google.guava:guava
  ```
  That tried to update protobuf to a release candidate. I had thought
  that perhaps this was the result of [a protobuf change from "-rc1" to
  "-rc-1"](protocolbuffers/protobuf#6522), but
  I'm wondering if it's actually just that `versions-maven-plugin` (in
  contrast to Dependabot) counts release candidates [as
  releases](https://www.mojohaus.org/versions/versions-maven-plugin/use-latest-releases-mojo.html)?
  I'd have to investigate further and perhaps [use
  `rulesUri`](https://stackoverflow.com/a/46935363/28465). But since we
  normally rely on Dependabot (and I was using `versions-maven-plugin`
  here only "to make things easier"... :)), I'm not going to worry about
  it now.
  ...
  Also, it looks like protobuf 4 might remove the `hasPresence()` method
  that we'd migrated to back in cl/508716698. If so, there's a good
  chance that the protobuf team will fix things for us :) If not, I'm
  assuming that this will relate to
  ["Editions."](https://protobuf.dev/editions/overview/)

Fixes #1149
Fixes #1148
Fixes #1146
Fixes #1145
@cpovirk
Copy link
Member Author

cpovirk commented Jul 24, 2023

It turns out that the latest release of jsinterop-annotations is built to require Java 11. That said:

  • We somehow don't declare a dep on that library, even though we use things from that package in guava-gwt. Are we relying on users to provide it themselves? (This is another case in which it would be nice for our external GWT tests to be as thorough as our internal ones: b/169936479.) Ah, might the GWT plugin provide it automatically? Are the effects any different on J2CL than on GWT? (Perhaps J2CL users are even more likely to provide the dependency themselves?)
  • I don't know how much the annotations artifact changes over time. I guess @JsNonNull was new in 2.0 or so, but I haven't looked at what changes in 2.1.0. Maybe we can get away with not updating for a while.
  • Even if we eventually need to require Java 11 for this reason, we might be able to require it only for guava-gwt.
  • Maybe things actually kinda sorta work if only annotations are built for Java 11, given how Java is usually of missing annotations?
  • Oh, and this is all just in sources seen only by the GWT and J2CL compilers. Maybe we should just increase the -target for our GWT build? Unfortunately, I think that might affect both client (JavaScript) and server (bytecode), which would mean that users do need to upgrade. Hmm, I thought guava-gwt contained actual compiled classes (not just sources to be compiled by the GWT/J2CL compiler later), but maybe that is only for GWT-RPC, which we don't support anymore. If we do still need to provide server sources, perhaps it's possible to compile them with a different -target than the others.
  • Or I guess we don't even need to change our -target: We just need to use a compiler that would recognize the newer bytecode (assuming that everything works OK for users at runtime). That's more like Require Java 11+ to *build* Guava but continue to support Java 8 at runtime #6549. I'll note this there.

copybara-service bot pushed a commit to google/truth that referenced this issue Jul 24, 2023
Notes:

- The GWT bump requires changing the browser setting from "FF38" to "FF."
- I skipped the `jsinterop-annotations` bump to 2.1.0 because it's built
  with `-target 11`, while we still support 8.
  (Closes #1147)
  (I've noted this in google/guava#6614)
- The command I used is:
  ```
  mvn org.codehaus.mojo:versions-maven-plugin:2.16.0:{update-properties,use-latest-releases} -DgenerateBackupPoms=false -Dexcludes=com.google.guava:guava
  ```
  That tried to update protobuf to a release candidate. I had thought
  that perhaps this was the result of [a protobuf change from "-rc1" to
  "-rc-1"](protocolbuffers/protobuf#6522), but
  I'm wondering if it's actually just that `versions-maven-plugin` (in
  contrast to Dependabot) counts release candidates [as
  releases](https://www.mojohaus.org/versions/versions-maven-plugin/use-latest-releases-mojo.html)?
  I'd have to investigate further and perhaps [use
  `rulesUri`](https://stackoverflow.com/a/46935363/28465). But since we
  normally rely on Dependabot (and I was using `versions-maven-plugin`
  here only "to make things easier"... :)), I'm not going to worry about
  it now.
  ...
  Also, it looks like protobuf 4 might remove the `hasPresence()` method
  that we'd migrated to back in cl/508716698. If so, there's a good
  chance that the protobuf team will fix things for us :) If not, I'm
  assuming that this will relate to
  ["Editions."](https://protobuf.dev/editions/overview/)

Fixes #1149
Fixes #1148
Fixes #1146
Fixes #1145

Fixes #1150

RELNOTES=n/a
PiperOrigin-RevId: 550553262
copybara-service bot pushed a commit to google/truth that referenced this issue Jul 24, 2023
Notes:

- The GWT bump requires changing the browser setting from "FF38" to "FF."
- I skipped the `jsinterop-annotations` bump to 2.1.0 because it's built
  with `-target 11`, while we still support 8.
  (Closes #1147)
  (I've noted this in google/guava#6614)
- The command I used is:
  ```
  mvn org.codehaus.mojo:versions-maven-plugin:2.16.0:{update-properties,use-latest-releases} -DgenerateBackupPoms=false -Dexcludes=com.google.guava:guava
  ```
  That tried to update protobuf to a release candidate. I had thought
  that perhaps this was the result of [a protobuf change from "-rc1" to
  "-rc-1"](protocolbuffers/protobuf#6522), but
  I'm wondering if it's actually just that `versions-maven-plugin` (in
  contrast to Dependabot) counts release candidates [as
  releases](https://www.mojohaus.org/versions/versions-maven-plugin/use-latest-releases-mojo.html)?
  I'd have to investigate further and perhaps [use
  `rulesUri`](https://stackoverflow.com/a/46935363/28465). But since we
  normally rely on Dependabot (and I was using `versions-maven-plugin`
  here only "to make things easier"... :)), I'm not going to worry about
  it now.
  ...
  Also, it looks like protobuf 4 might remove the `hasPresence()` method
  that we'd migrated to back in cl/508716698. If so, there's a good
  chance that the protobuf team will fix things for us :) If not, I'm
  assuming that this will relate to
  ["Editions."](https://protobuf.dev/editions/overview/)

Fixes #1149
Fixes #1148
Fixes #1146
Fixes #1145

Fixes #1150

RELNOTES=n/a
PiperOrigin-RevId: 550560426
@cpovirk
Copy link
Member Author

cpovirk commented Oct 9, 2023

Java 11 brought nestmates, which would let us make fields like this one private without any performance hit from the need for bridge methods. That hit is likely to be unnoticeable for most methods, small even for CharMatcher, and avoidable by assigning the result of the CharMatcher accessor call to a field (rather than calling the accessor over and over). Still, it would be nice for a single innocent-looking keyword not to have the potential to affect performance. And making things private is nice. (It's even required for similar constructs in Kotlin.)

Admittedly, making nested classes' members private is less important when the class that the API is part of is final, since that makes clear that any package-private methods aren't being called more widely through a subtype. And there are additional arguments for omitting private: One is that private doesn't further restrict visibility, so we'd rather write leave it out for brevity. Another (somewhat contradictory) argument is that the lack of private hints that the API is being used from the nested class's enclosing class (though of course Java would permit the call even with private, just with the possible performance hit discussed above).

Anyway, the point here is that we'd pick up this improvement (and others, like better string concatenation) if we didn't have to target Java 8. (Hmm, I guess we could theoretically provide a multi-release jar with the exact same classes built for both versions, just with different -source -target flags! This would even provide yet another partial defense against problems like that from #3990. But that would double the size of Guava (and possibly upset tools that get confused by mrjars) for very little upside.)

(I was tempted to add that we could consider targeting a newer version of Java for guava-android, since newer bytecode gets converted to Android bytecode that supports even old versions of Android. But of course we say that guava-android is usable under a JVM, too, so that wouldn't work.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 package=general type=other Miscellaneous activities not covered by other type= labels
Projects
None yet
Development

No branches or pull requests

1 participant