Declare byte-buddy as 'implementation' dependency instead of shading. #4266
Conversation
sellmair
force-pushed
the
sellmair/byte-buddy
branch
from
8eba785 to
2a0b02d
Compare
There was a problem hiding this comment.
I don't have comments to the overall idea of not using shading, but then, https://github.com/Kotlin/kotlinx.coroutines/tree/master/kotlinx-coroutines-debug#build-failures-due-to-duplicate-resource-files needs to be updated so that it no longer mentions the issues related to shading.
|
Updated the readme ✅ |
|
@dkhalanskyjb waiting for your review as well here, as you already started viewing the mechanical aspect of the change |
| @@ -170,9 +170,6 @@ conflicts, but: | |||
| * `kotlinx-coroutines-debug` transitively depends on JNA and JNA-platform, both of which include license files in their | |||
| META-INF directories. Trying to merge these files leads to conflicts, which means that any Android project that | |||
| depends on JNA and JNA-platform will experience build failures. | |||
| * Additionally, `kotlinx-coroutines-debug` embeds `byte-buddy-agent` and `byte-buddy`, along with their resource files. | |||
| Then, if the project separately depends on `byte-buddy`, merging the resources of `kotlinx-coroutines-debug` with ones | |||
| from `byte-buddy` and `byte-buddy-agent` will lead to conflicts as the resource files are duplicated. | |||
There was a problem hiding this comment.
With this change, the list above only has a single bullet point (so it's strange that it's still a list), and also, in addition to JNA and JNA-platform mentioned above, kotlinx-coroutines-debug now depends on byte-buddy and byte-buddy-agent.
There was a problem hiding this comment.
At this point, I think one of two ways is more efficient than ping-ponging in this review.
Either we merge the contribution as is and you can quickly edit the readme to your liking in a new commit, or you might just want to edit the commit directly pre merge.
WDYT?
There was a problem hiding this comment.
There's also a third way, the one we usually take in this library: the contributor carefully looks at the places affected by the proposed change, considers the consequences, and then introduces sweeping, coordinated edits. In other words, instead of making a small isolated change, they consider the bigger picture. A reviewer's job then boils down to highlighting the affected areas the contributor hasn't thought of / didn't know about (without enumerating every specific line of code to change), after which the contributor goes back to researching what the proposed change entails and introduces further sweeping edits. No ping-ponging results, as the list of such areas to consider is typically limited.
Although the third way is preferred, both of the options you've proposed are okay: we're also glad and thankful to accept incomplete contributions if the contributor says: "I'm no longer interested in working on this, you do it." In this scenario, the credit still goes to the contributor.
There was a problem hiding this comment.
I pushed an update, please provide feedback
There was a problem hiding this comment.
Nice, thanks! I've also left a comment with my review, it's possible that you've missed it:
With this change, we are no longer shading anything, so could you please also remove all mentions of the shadow plugin from our library?
Shading byte buddy might have negative effects: - It increases binary size by ~3MB w/o an easy option to opt-out - It causes JPMS issues (because of the re-location itself) The used APIs from the dependency can be considered stable.
sellmair
force-pushed
the
sellmair/byte-buddy
branch
from
ee48309 to
c10a35d
Compare
Shading byte buddy might have negative effects:
The used APIs from the dependency can be considered stable.