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

[NETBEANS-3646] - Update google guava from 15.0 to 27.0 #1828

Merged
merged 2 commits into from Feb 26, 2020

Conversation

pepness
Copy link
Member

@pepness pepness commented Dec 29, 2019

Notes:

  • Several bug fixes
  • Several improvements
  • Requires Java 8
  • Added JPMS module name com.google.common for Guava
  • Fix security vulnerability CVE-2018-10237

Google Guava Web Page
Google Guava Release Notes 1
Google Guava Release Notes 2

@eirikbakke
Copy link
Contributor

eirikbakke commented Jan 3, 2020

I see a lot of "Upgrade [dependency] from [old version] to [new version]" PRs--is there a rationale for these changes?

@matthiasblaesing
Copy link
Contributor

@eirikbakke I'm not the author of the changes, but I appreciate the updates. We have several areas where libraries were not updated over years (javascript the most prominent one) and now we suffer from huge steps that need to be taken. Do you see a problem there?

@jlahoda
Copy link
Contributor

jlahoda commented Jan 3, 2020

This is not specifically against upgrading any library, but every change can potentially break things. It did in fact happen with the Gson upgrade - version 2.8.4 breaks on pre-release versions of JDK - see e.g. google/gson#1310 (my personal build of JDK has java.version "14-internal", and Gson 2.8.4 cannot chew that). So I am upgrading Gson to 2.8.5 in #1833 - but the old version (2.7) would work just fine as well, I think.

@eirikbakke
Copy link
Contributor

eirikbakke commented Jan 3, 2020

It's the concern that jlahoda pointed out--each change like this carries a small possibility that something will break. It's fine to bump library versions whenever there's a specific reason for doing so (e.g. to use a new feature), but otherwise, it's just an unnecessary risk.

As an example, the update in 4b82e6a caused a bug that ended up taking perhaps 8 man-hours of time to debug, discuss, and fix--even though the final fix was just to revert the original change (see #854 ).

(Per the old saying... "If it ain't broke, don't fix it.")

@eirikbakke
Copy link
Contributor

That being said... Guava in particular is a really high-quality library, with few complicated dependency issues (unlike, say, XML libraries, which always seem to cause problems). I think upgrading it carries a relatively low risk. But which modules are currently depending on it?

@matthiasblaesing
Copy link
Contributor

Yes things will brake, but they also break if you don't do anything. In fact the whole java platform set the direction. In @jlahoda sample two libraries were updated: Gson and the JDK (yes Java is just a library) and in Kombination broke. In #854 not a library was changed, but a different API was used.

To come back to my sample: graaljs was heavily in development and nobody cared to update. And now we have an obsolete copy, that we currently fail to update. Had we updated in time, we had small steps and would not face a cliff.

Carring obsolete, unmaintained libraries is not a solution, but a major problem. As noone else took care, I still appreciate someone taking care and can life with the fall out.

@eirikbakke
Copy link
Contributor

@matthiasblaesing Makes sense. Perhaps these kinds of changes can be merged right after each release is branched off, to make sure they are tested in dev builds for a few months before becoming part of the next release (to detect any problems).

@matthiasblaesing matthiasblaesing added this to the 12.0 milestone Jan 3, 2020
@matthiasblaesing
Copy link
Contributor

@eirikbakke good point. I marked the remaining updates as NB12.0 so that they can be integrated early in the next cycle.

@jlahoda
Copy link
Contributor

jlahoda commented Jan 3, 2020

@matthiasblaesing - in my case, there was only one library updated: Gson. The JDK was exactly the same before and after (if I had a new JDK, it would be 15-internal by now). The 2.8.4 version was apparently broken even for fairly old JDK's, based on the bug report. And it was a basically a complete showstopper for me - I was not able to continue with #1833 until I upgraded. (And #1833 is needed for ccls to work inside NetBeans at least somewhat.)

Part of the problem here is that both Gson and Guava are used by LSP4J, and as these third-party projects, it is more difficult to know what may break. Overall, I believe Gson is kept compatible - I believe compatibility is promised for Guava as well, but in the past they were making incompatible changes, so it makes me much more wary.

Of course delaying upgrades may lead to a lot of work for the migration (possibly another example is the use of Apache Lucene in our indexing infrastructure). But for every migration, we should try to make sure things still work after the upgrade. (Which, I admit, was very difficult for the Gson case - although a better investigation on why 2.8.5 is "incompatible" with the previous version would probably reveal that 2.8.4 is not a good version to use.)

@ebarboni
Copy link
Contributor

Maybe good to merge to have time to check for issue ? As we just repopen merging windows.

@eirikbakke eirikbakke merged commit 5e72bd0 into apache:master Feb 26, 2020
@eirikbakke
Copy link
Contributor

Yup! Squashed and merged.

@junichi11 junichi11 added the Upgrade Library Library (Dependency) Upgrade label Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Upgrade Library Library (Dependency) Upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants