Skip to content

Conversation

@dbreheret
Copy link

As advised by Matt Sicker, created a jep for guava upgrade.

@jvz
Copy link
Member

jvz commented Nov 18, 2020

The main thing that may cause issues here will be in plugins. For example, searching for com.google.common in this org (note that Guice, Gson, and likely some other Google libraries may show up in this search, so you might have a better idea on what to look for than that package): https://github.com/search?q=org%3Ajenkinsci+filename%3A*.java+in%3Afile+com.google.common&type=code

In order to upgrade Guava, we'll likely need some additional glue code for backward compatibility with the version distributed in Jenkins if anything here breaks. The Spring Security upgrade JEP shows how this can potentially be non-trivial. These concerns are what you can describe in the "Compatibility" section of this JEP.


== Prototype Implementation

link:https://github.com/jenkinsci/jenkins/pull/5059[jenkins #5059] is the main change.
Copy link
Member

Choose a reason for hiding this comment

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

|link:https://plugins.jenkins.io/ant/[ant]
|To be investigated
|

Copy link
Member

Choose a reason for hiding this comment

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

@jglick jglick changed the title JEP 311 for Upgrading guava to 30.0-jre Upgrade guava to 30.0-jre Nov 19, 2020
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Structural mistakes.

@@ -0,0 +1,131 @@
= JEP-311: Upgrade guava to 30.0-jre
Copy link
Member

@jglick jglick Nov 19, 2020

Choose a reason for hiding this comment

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

Please follow the instructions in JEP-1 for drafting a JEP. In particular, you do not get to pick a number: this should be jep/0000/README.adoc and should refer to JEP-0000.

Copy link
Member

Choose a reason for hiding this comment

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

I can change it before merging

== Testing

Besides tests inside Jenkins core itself,
CloudBees will endeavor to verify that all
Copy link
Member

Choose a reason for hiding this comment

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

Uh, no, CloudBees does not make any such promise at this time. (Copy-pasta?)

dbreheret and others added 4 commits November 20, 2020 09:15
Co-authored-by: Jesse Glick <jglick@cloudbees.com>
Co-authored-by: Jesse Glick <jglick@cloudbees.com>
Co-authored-by: Jesse Glick <jglick@cloudbees.com>
Co-authored-by: Jesse Glick <jglick@cloudbees.com>
== Abstract

Jenkins uses guava version 11.0.1, which is seriously dated.
Security AppScan are not happy about this version.
Copy link
Member

Choose a reason for hiding this comment

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

Note that security scans detect a serialization-related vulnerability in Guava, but Jenkins uses JEP 200 to form an explicit list of allowed classes for deserialization, and the affected Guava class(es) are not and will never be added to the list. Getting security scanners to stop complaining about it, though, is certainly a useful goal as that was why there were recent JEPs to do all the tedious work to upgrade both Spring Security and XStream, both of which also had security scan complaints that were irrelevant in Jenkins and involved libraries that had broken backward compatibility.

However, if we can successfully upgrade Guava, then that will make it so that plugins might actually be able to more reliably use Guava themselves rather than avoiding it due to being an outdated dependency.

@oleg-nenashev oleg-nenashev self-requested a review November 21, 2020 07:05
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Thanks for starting this draft @dbreheret!

Requesting changes because of the CloudBees commitment reference. If there is no explicit and public commitment from a company contributor, it should not be mentioned like that even in the draft.

For other matters, there is a pending conversation in jenkinsci/jenkins#5059 which could be taken into account in the JEP specification. Addressing this feedback does not block the JEP from being merged as a draft

@olamy
Copy link
Member

olamy commented Feb 23, 2021

BTW we are now guava 30.1-jre :)

@dbreheret
Copy link
Author

Smashing :)

@oleg-nenashev
Copy link
Member

@jenkinsci/core @basil @batmat Taking https://groups.google.com/g/jenkinsci-dev/c/aYUJ4VuOuVc/m/I9IEEzovAgAJ , would we like to revive and continue this proposal? CC @dbreheret in case he is still interested

@dbreheret
Copy link
Author

@jenkinsci/core @basil @batmat Taking https://groups.google.com/g/jenkinsci-dev/c/aYUJ4VuOuVc/m/I9IEEzovAgAJ , would we like to revive and continue this proposal? CC @dbreheret in case he is still interested

Yes, still interested.

utility classes, google's collections, io classes, and much
much more.

Guava (complete) has only one code dependency - javax.annotation,
Copy link
Member

Choose a reason for hiding this comment

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

which we should not ship - but in this case as it is annotations we can exclude it without issue.

Refer to general information about link:README.adoc#backwards-compatibility[backwards compatibility]
for tips on searching for potentially problematic API usages.

Plugins which do not do anything special with Guava need not be listed.
Copy link
Member

Choose a reason for hiding this comment

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

what classifies as special here?

@batmat batmat changed the title Upgrade guava to 30.0-jre [JENKINS-65988] Upgrade guava to 30.0-jre Jun 25, 2021
@jtnord
Copy link
Member

jtnord commented Oct 1, 2021

obsoleted by #375?

@timja timja closed this Oct 1, 2021
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.

7 participants