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.

Sign up for GitHub

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

Jump to bottom

Animal-sniffer-annotation 1.18 #6488

Merged
merged 4 commits into from
Dec 5, 2019
Merged

Animal-sniffer-annotation 1.18 #6488

merged 4 commits into from
Dec 5, 2019

Conversation

suztomo
Copy link
Contributor

@suztomo suztomo commented Dec 4, 2019

Upgrading animal-sniffer-annotation to the latest 1.18.

There was a problem in upperBoundCheck in google-cloud-firestore: googleapis/java-firestore#34.

@creamsoup creamsoup added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Dec 5, 2019
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Dec 5, 2019
@creamsoup
Copy link
Contributor

bazel dep needs to be updated, see repositories.bzl.

nit: please update comments mentioning the old version

api/build.gradle:        // prefer 1.17 from libraries instead of 1.14
protobuf-lite/build.gradle:        // prefer 1.17 from libraries instead of 1.14
protobuf/build.gradle:        // prefer 1.17 from libraries instead of 1.14

@creamsoup creamsoup self-requested a review December 5, 2019 01:46
suztomo
suztomo commented Dec 5, 2019
View reviewed changes
Copy link
Contributor Author

@suztomo suztomo left a comment

Choose a reason for hiding this comment

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

@creamsoup Thank you for checking. Updated bzl file.

api/build.gradle Outdated
@@ -20,7 +20,7 @@ dependencies {
exclude group: 'com.google.errorprone', module: 'error_prone_annotations'
// prefer 3.0.2 from libraries instead of 3.0.1
exclude group: 'com.google.code.findbugs', module: 'jsr305'
// prefer 1.17 from libraries instead of 1.14
// prefer our own version from libraries instead of Guava's dependency
Copy link
Contributor Author

@suztomo suztomo Dec 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Guava 28.1 has dependency animal-sniffer-annotation 1.18, not 1.14. The old comment was outdated; it must have been hard to maintain the comment precisely by reading the Guava's pom.xml.

@creamsoup I updated this comment in more generic way, and now I feel these comments "prefer XXX from libraries instead of YYY" are hard to maintain and also my "prefer our own version from libraries instead of Guava's dependency" looks unnecessary. Don't you think we should remove these comments?

Copy link
Contributor

@creamsoup creamsoup Dec 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

yeah it is hard to maintain, i like your new comment (or no comment is also good enough).
i am going to make the cleanup PR. thanks for the suggestion!

repositories.bzl
server_urls = ["http://central.maven.org/maven2"],
artifact_sha256 = "92654f493ecfec52082e76354f0ebf87648dc3d5cec2e3c3cdb947c016747a53",
artifact_sha256 = "47f05852b48ee9baefef80fa3d8cea60efa4753c0013121dd7fe5eef2e5c729d",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

suztomo-macbookpro44:grpc-java suztomo$ shasum -a 256 ~/Downloads/animal-sniffer-annotations-1.18.jar 
47f05852b48ee9baefef80fa3d8cea60efa4753c0013121dd7fe5eef2e5c729d  /Users/suztomo/Downloads/animal-sniffer-annotations-1.18.jar

creamsoup
creamsoup approved these changes Dec 5, 2019
View reviewed changes
Copy link
Contributor

@creamsoup creamsoup left a comment

Choose a reason for hiding this comment

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

LGTM

@creamsoup creamsoup added the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Dec 5, 2019
@grpc-kokoro grpc-kokoro removed the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Dec 5, 2019
@suztomo
Copy link
Contributor Author

suztomo commented Dec 5, 2019

Updated the comments to one generic comment for the occurrences of animal-sniffer-annotations exclusions.

suztomo
suztomo commented Dec 5, 2019
View reviewed changes
api/build.gradle
@@ -16,11 +16,9 @@ dependencies {
libraries.jsr305,
libraries.animalsniffer_annotations
compile (libraries.guava) {
// prefer 2.3.3 from libraries instead of 2.1.3
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment has been outdated; Guava 28.1 uses error_prone_annotations:2.3.2.

api/build.gradle
exclude group: 'com.google.errorprone', module: 'error_prone_annotations'
// prefer 3.0.2 from libraries instead of 3.0.1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment has been outdated; Guava 28.1 uses jsr305:3.0.2.

@suztomo suztomo mentioned this pull request Dec 5, 2019
Merged
@suztomo
Copy link
Contributor Author

suztomo commented Dec 5, 2019

Created another PR that for files that are irrelevant to animal-sniffer-annotations.
#6492

@ericgribkoff ericgribkoff added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Dec 5, 2019
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Dec 5, 2019
@creamsoup creamsoup merged commit 80699f8 into grpc:master Dec 5, 2019
@creamsoup
Copy link
Contributor

merged, thanks @suztomo!

@suztomo
Copy link
Contributor Author

suztomo commented Dec 5, 2019

Thank you.

@suztomo suztomo deleted the animal_sniffer branch December 5, 2019 21:17
@lock lock bot locked as resolved and limited conversation to collaborators Mar 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@creamsoup creamsoup creamsoup approved these changes

@ericgribkoff ericgribkoff ericgribkoff approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@suztomo @creamsoup @ericgribkoff @grpc-kokoro