-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 NOTICE and LICENSE in the gcp-bundle jar #12144
Conversation
@amogh-jahagirdar @rdblue @Fokko This is the update/fix for |
gcp-bundle/LICENSE
Outdated
@@ -325,24 +315,24 @@ License: The Apache Software License, Version 2.0 - http://www.apache.org/licens | |||
|
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.
We need to suppress this and include the stephenc findbugs implementation instead. This one has licensing issues and the stephenc version was built to avoid them. That, or I think if this is only annotations, we can just exclude it. Annotations shouldn't be required at runtime if they are not used at runtime (and these are for compile time).
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.
Which one ? Not sure I follow you here 😄
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.
Findbugs (com.google.code.findbugs) I see some contents from META-INF
but no actual classes in the bundle bits
0 Fri Feb 01 00:00:00 MST 1980 META-INF/maven/com.google.code.findbugs/
0 Fri Feb 01 00:00:00 MST 1980 META-INF/maven/com.google.code.findbugs/jsr305/
4286 Fri Feb 01 00:00:00 MST 1980 META-INF/maven/com.google.code.findbugs/jsr305/pom.xml
121 Fri Feb 01 00:00:00 MST 1980 META-INF/maven/com.google.code.findbugs/jsr305/pom.properties
I'd be +1 on excluding it.
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.
Yes, it's what I see as well, but as I still see resources from jsr305
(even if very limited), I preferred to keep it.
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.
Sure, since these are compile time could we just exclude the bundling of this dependency entirely in the building of the shadowjar? Then we could just remove it entirely from LICENSE.
exclude(dependency('com.google.code.findbugs:jsr305'))
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.
I agree. Let me clean it up. Even if gradle keeps pom it's not a problem.
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.
Done
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.
The license of findbugs was unclear because of a reference to LGPL (see findbugsproject/findbugs#128). The implementation we use elsewhere was built to avoid the problem: https://github.com/stephenc/findbugs-annotations?tab=readme-ov-file#rational
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.
@amogh-jahagirdar I removed the mention of jsr305.
@rdblue thanks for the details (LICENSE/NOTICE is a tricky work and often different view from everyone 😄 ). I removed jsr305 as it's excluded from the gcp-bundle jar.
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.
Looks mostly good, but I flagged an issue with removing the duplicates (I don't see that they are duplicating anything).
af1914d
to
74805ce
Compare
dd65138
to
b06855b
Compare
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.
Thanks @jbonofre, I think these changes look good now
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.
I think a few are missing from opentelemetry, but apart from that it looks good to me 👍
b06855b
to
75e0be1
Compare
75e0be1
to
4f837d7
Compare
4f837d7
to
da79d81
Compare
@rdblue I think I addressed all comments. Can you please do a new pass ? Thanks ! |
Co-authored-by: Fokko Driesprong <[email protected]>
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.
Looks good, thanks again @jbonofre
This change includes:
LICENSE
andNOTICE
opencensus-proto
fromLICENSE
as not found in the runtime classpath and the jario.opentelemetry:*
inLICENSE
as the classes are in the jarNOTICE
NOTICE
contentNOTICE
contentNOTICE
content