-
Notifications
You must be signed in to change notification settings - Fork 21
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
fix: exclude unnecessary modules in Caffeine #571
Conversation
Thanks @showuon! Good finding, and agree it should help to make the distribution smaller and including only needed deps. |
TBH, I don't know much detail about it. But I agree with you that as long as the e2e tests passed, it means we don't need to ship these modules.
I agree. I just used the latest main to build gcs, it didn't include these packages. I think it's because we've excluded them from gcs here: #561 |
@showuon, yes that PR removed some, but in the issue comments, it references a few others:
I think it should be safe to exclude them as well? |
exclude group: "com.google.errorprone" | ||
exclude group: "org.checkerframework" | ||
exclude group: 'com.google.errorprone', module: 'error_prone_annotations' | ||
exclude group: 'org.checkerframework', module: 'checker-qual' | ||
exclude group: 'com.google.code.findbugs', module: 'jsr305' | ||
exclude group: 'com.google.j2objc', module: 'j2objc-annotations' | ||
exclude group: 'org.codehaus.mojo', module: 'animal-sniffer-annotations' | ||
exclude group: 'com.google.guava', module: 'listenablefuture' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Include module name to be more specific.
Good point @jeqo ! PR updated. Let's see if e2e tests can pass. |
All tests passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @showuon !
In the released core zip, we included
From this discussion in Caffeine project: Caffeine 2.7.0 extra dependencies ben-manes/caffeine#300 (comment) . The author of Caffeine project:
Since we only use JAVA and it doesn't cause any errors while compiling after excluding them, I think we should remove them from the distribution zip.