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

Upgrade ASM and Error Prone #2351

Merged
merged 1 commit into from
Mar 23, 2020
Merged

Upgrade ASM and Error Prone #2351

merged 1 commit into from
Mar 23, 2020

Conversation

gliptak
Copy link
Contributor

@gliptak gliptak commented Mar 20, 2020

No description provided.

@chanseokoh
Copy link
Member

Thanks for the contribution. 2 things:

@gliptak
Copy link
Contributor Author

gliptak commented Mar 20, 2020

@chanseokoh thanks for the pointers. I will update PR

@chanseokoh
Copy link
Member

Oh, and seems like Error Prone has issues with Java 12+. Unclear if they are all fixed.

@gliptak
Copy link
Contributor Author

gliptak commented Mar 20, 2020

google/error-prone#1107

@chanseokoh
Copy link
Member

Yeah, unfortunately, looks like we cannot add 12+ builds until they support them.

@don-vip
Copy link

don-vip commented Mar 20, 2020

Oh, and seems like Error Prone has issues with Java 12+. Unclear if they are all fixed.

Nothing is fixed. I am struggling for 2 years now to submit pull requests that are ignored by error_prone developers, see google/error-prone#1106

For JOSM we apply around 5/6 patches to make it work with recent versions of Java.

@gliptak
Copy link
Contributor Author

gliptak commented Mar 20, 2020

@chanseokoh gradle 5.x only works for 12-

this morphed into something else. Travis build is green

@don-vip maybe time to look for another tool ...

@don-vip
Copy link

don-vip commented Mar 20, 2020

@gliptak for now we manage to update the tool with the patches. Maybe one day we'll drop it if Google continues to ignore recent Java. Are you working at Google, or just using their tools?

@gliptak
Copy link
Contributor Author

gliptak commented Mar 20, 2020

@don-vip just run into this while trying to bump Java version for jib

@chanseokoh
Copy link
Member

@don-vip maybe try the mailing list? https://groups.google.com/g/error-prone-discuss

@loosebazooka are we okay to upgrade Gradle, or it's safer to stick to the current 5.2.1 to prevent accidental API usage?

@chanseokoh chanseokoh changed the title Add OpenJDK 12/13/14 to Travis Upgrade Gradle wrapper, ASM, Error Prone Mar 20, 2020
@loosebazooka
Copy link
Member

loosebazooka commented Mar 22, 2020

I mean I would like to stick to 5.2.1 if we could. Which part of this PR requires gradle 5.6.4? Is it the asm upgrade or the errorprone upgrade?

@gliptak
Copy link
Contributor Author

gliptak commented Mar 22, 2020

@chanseokoh I'm not following the "accidental API usage" concern

@loosebazooka the Gradle upgrade was in preparation for adding current JDKs

@loosebazooka
Copy link
Member

We try to stay on as low a gradle version as possible so we don't use API from newer versions of gradle -- If XYZ is only in gradle 5.6, we break all users who are on a version lower than 5.6. We have some testing for this, but it's not super comprehensive.

So staying at 5.2.1 keeps us compatible with more users. We generally try to give users a year for moving up gradle version. However there is nothing keeping the user from using 5.6.X in their build -- that should be fully compatible with anything we use from 5.2.1

@gliptak
Copy link
Contributor Author

gliptak commented Mar 22, 2020

Isn't the point of https://docs.gradle.org/current/userguide/gradle_wrapper.html that the pinned Gradle version is used (and automagically downloaded if needed) regardless of what Gradle (if any) is already installed by the user?

@loosebazooka
Copy link
Member

loosebazooka commented Mar 23, 2020

Ah so I think you're misunderstanding here. When building a gradle plugin you build with some version of the gradle api (in jib's case 5.2.1), however the gradle API is a provided dependency as is not packaged with the project, it's whatever is available at runtime -- so as long as user who has jib applied on their project is using gradle 5.2.1 or higher, it will work.

Pinning the version on the jib build creates no inherent restrictions on the version of gradle the user uses to build their project except for API compatibility and the code we have here: https://github.com/GoogleContainerTools/jib/blob/master/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/JibPlugin.java#L85

So if the user needs 5.6.2 to build java 12 or 13, that's fine, we may not need to make this build use 5.6.2 unless jib introduces some explicit support for java 12 or 13 via code that uses references to features in gradle 5.6.2.

Signed-off-by: Gábor Lipták <gliptak@gmail.com>
@gliptak
Copy link
Contributor Author

gliptak commented Mar 23, 2020

@loosebazooka thank you for the explanation. Remove Gradle bump from PR

@chanseokoh chanseokoh changed the title Upgrade Gradle wrapper, ASM, Error Prone Upgrade ASM and Error Prone Mar 23, 2020
@chanseokoh
Copy link
Member

chanseokoh commented Mar 23, 2020

@gliptak thanks for checking these! Merging.

@chanseokoh chanseokoh merged commit 1465fe9 into GoogleContainerTools:master Mar 23, 2020
@gliptak gliptak deleted the patch-1 branch April 6, 2020 16:47
@gliptak gliptak mentioned this pull request Jun 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants