Shading also violates a lot of common ClassLoader assumptions like not having multiple copies of the same class in the same visible context (even if different packages) and gets more complex as your types do. I’ve seen this problem when projects tried to shade Avro or Protobuf. As a downstream user of the library, it becomes impossible to use the upstream code without using its shaded code.
Of course, this is not always the case, but it’s usually sloppy. That’s not even counting reflection APIs. On Fri, Jul 24, 2020 at 10:34 Gary Gregory <garydgreg...@gmail.com> wrote: > On Fri, Jul 24, 2020 at 10:50 AM Gilles Sadowski <gillese...@gmail.com> > wrote: > > > Hello. > > > > Le ven. 24 juil. 2020 à 14:48, Gary Gregory <garydgreg...@gmail.com> a > > écrit : > > > > > > On Fri, Jul 24, 2020 at 6:03 AM Gilles Sadowski <gillese...@gmail.com> > > > wrote: > > > > > > > 2020-07-24 11:25 UTC+02:00, Torsten Curdt <tcu...@vafer.org>: > > > > > It still needs a person to decide to merge a PR for a new version. > > > > > So this indeed is just about the dependency upgrade policies. > > > > > > > > > > But isn't that what the version definition is for? > > > > > > > > Ideally. > > > > In practice, I think that all we can assume is that the version > > > > string is a quick-glance summary of changes, for human > > > > consumption. > > > > > > > > > I'd argue that 1.12.4 <-> 1.12.6 should be a compatible upgrade AND > > > > > downgrade, > > > > > 1.12.4 -> 1.20.0 not so much. > > > > > > > > > > But to avoid all this is why I usually try to inline dependencies > for > > > > > libraries as much as possible. Basically pretending to not have > any. > > > > > This of course depends on whether the dependency can be isolated > that > > > > way. > > > > > > > > > > Also a point I made many times. > > > > > Just wanted to mention it - again :) > > > > > > > > I think that it would be great that "Commons" has a common > > > > policy for dealing with this (so that we don't have to repeat > > > > ourselves every now and then). > > > > A long time ago, the "shade" feature seemed the perfect > > > > answer to that problem. Yet, to avoid dependencies even > > > > on another Commons component, several of us continue to > > > > support the copy/paste option. > > > > > > > > > > There are a few items I'd like to address in this thread so I'll start > > with > > > the above. > > > > > > I would like to offer my view on shading and copy/paste as - to borrow > > Jeff > > > Atwood's expression - a "coding horror". While you may think that there > > are > > > still edge cases where this may seem to some as necessary or > expedient, I > > > hope to dissuade you from these practices. > > > > > > The first and most obvious horror to me is the copying and pasting of > > bugs > > > and hard to maintain code. You may also end up taking along supporting > > > methods and classes to support the code you copy. While this may seem > to > > > follow the YAGNI principle, my claim is that you can end up with an > > iceberg > > > of new code, where the new API surface above the water does not end up > > > exercising all of the supporting code you had to drag along that lies > > > below. > > > > > > The question where I am most curious to hear back from the copy clan > is: > > Do > > > you also copy the unit tests that cover the code you copied? No? Coding > > > horror #2. > > > > > > The third horror does not affect us within Apache but its general > > practice > > > begs its adherents to answer: Did you check the license of the code you > > > copied? No? Coding horror #3. Again, this is not an issue within Apache > > > projects, but the question pops up time and time again at Apache (and > at > > > work): Can I copy this code I found outside of Apache? > > > > > > The fourth and final copy horror falls in the same category as the > first > > > and deserves its own mention: security; and can be best understood with > > > this question: How often do you check for CVEs for the library > containing > > > the code you copied? Never? Coding horror #4. > > > > Agreeing on all the above. > > > > Yet, every time there is an opportunity to engage in this wise > > path, the copying/pasting continues and slowly grows the iceberg. > > A recent example is the issue with pseudo-random numbers > > generation where maintaining duplicate functionality in [Lang] > > was preferred even though [RNG] has 100% coverage (and a > > explicit scope that entails it will never have any dependency of > > its own). > > > > Would you be willing to provide a PR that deprecates the relevant APIs and > points to their equivalent in RNG? It will be more cruft we can trim for > 4.0, whenever that happens. > > > > > Shading has its limited use in my view, and I've used it to provide > > > monolithic versions of applications, but beyond that, I've no use for > it; > > > but the pain it can cause is very real indeed. > > > > > > You can imagine all manner of jar-hell created by shading. For > instance: > > > - library L1 shades library ShadedA-1.0 and ShadedB-1.1. > > > - library L2 shades library ShadedA-1.1 and ShadedB-1.0. > > > - An app wants to use L1, L2, ShadedA-1.1, and ShadedB-1.1 but it can't > > no > > > matter what classpath ordering it uses. > > > - An app wants to use L1, L2, ShadedA-1.0, and ShadedB-1.0 but it can't > > no > > > matter what classpath ordering it uses. > > > > I didn't understand how the problem is supposed to arise. > > Fortunately, Torsten already replied to this point. :-) > > > > Unless I'm mistaken, there is one drawback to shading: Since > > it creates new classes, there will be less shared code, hence > > one could imagine a potential hit on performance (?). > > > > > Lastly, there is a special kind of shading jar hell that deserves its > own > > > section: the copied and re-packaged source/jar. In this lovely planned > > > development managed by Hades, an entire jar has been copied then > > repackaged > > > such that instead of being in its original Java package > org.foo.library, > > it > > > is now under org.other.bar.org.foo.library. Residents of this charming > > > cottage have now the proud owners of every bug and CVE for that > specific > > > version of the source jar, without, and this is the kicker, the option > of > > > overriding the jar with a new version (I'll leave aside your option to > > take > > > a new version of the jar and repackaging it yourself to put it in front > > of > > > the shaded jar.) I've seen this in various libraries that shade > > > bytecode manipulation libraries that you can't use on new Java > versions. > > > > I must be missing something: Since shaded classes are supposed > > to be "internal", if there is an impact on the public API, the only sane > > solution is a bug-fix release that shades the fixed version of the > internal > > library. No? > > > > Which is exactly the problem I am pointing out: You have no control over > when this third party will release a new version and whether that new > version contains updated shaded code. > > For example: JaCoCo 0.8.5 (current) and Java 15 yields this gorgeous stack > trace: > > Caused by: java.lang.IllegalArgumentException: Unsupported class file major > version 59 > 14951 > < > https://github.com/apache/commons-bcel/runs/899223983?check_suite_focus=true#step:4:14951 > > > at > > org.jacoco.agent.rt.internal_43f5073.asm.ClassReader.<init>(ClassReader.java:195) > > 14952 > < > https://github.com/apache/commons-bcel/runs/899223983?check_suite_focus=true#step:4:14952 > > > at > > org.jacoco.agent.rt.internal_43f5073.asm.ClassReader.<init>(ClassReader.java:176) > > 14953 > < > https://github.com/apache/commons-bcel/runs/899223983?check_suite_focus=true#step:4:14953 > > > at > > org.jacoco.agent.rt.internal_43f5073.asm.ClassReader.<init>(ClassReader.java:162) > > 14954 > < > https://github.com/apache/commons-bcel/runs/899223983?check_suite_focus=true#step:4:14954 > > > at > > org.jacoco.agent.rt.internal_43f5073.core.internal.instr.InstrSupport.classReaderFor(InstrSupport.java:280) > > 14955 > < > https://github.com/apache/commons-bcel/runs/899223983?check_suite_focus=true#step:4:14955 > > > at > > org.jacoco.agent.rt.internal_43f5073.core.instr.Instrumenter.instrument(Instrumenter.java:75) > > 14956 > < > https://github.com/apache/commons-bcel/runs/899223983?check_suite_focus=true#step:4:14956 > > > at > > org.jacoco.agent.rt.internal_43f5073.core.instr.Instrumenter.instrument(Instrumenter.java:107) > > 14957 > < > https://github.com/apache/commons-bcel/runs/899223983?check_suite_focus=true#step:4:14957 > > > ... 52 more > > Can I add the current version of ASM to my project to cure this ill? Of > course not, that would be too easy. > > Instead I have to wait for 0.8.6 which will be released whenever. I am > assuming that we would not want our POMs to depend on a snapshot... > > Gary > > > > Gilles > > > > > > > > I hope this helps someone... > > > > > > Gary > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > > For additional commands, e-mail: dev-h...@commons.apache.org > > > > > -- Matt Sicker <boa...@gmail.com>
