Re: Re: Re: RE: Re: unvendoring bytebuddy
Thanks for doing so much work verifying this and analyzing it! It really seems like we did all this for mockito, so it has very little risk of impacting users. And you've verified it is working with mockito now. So I think I'm in favor of unvendoring. This will make it much easier to get bugfixes, etc, with less toil. Kenn On Tue, Apr 12, 2022 at 12:35 PM Liam Miller-Cushon wrote: > I have created a PR that unvendors bytebuddy as a proof of concept and to > start testing, and the initial results look good: > https://github.com/apache/beam/pull/17317 > > I also did some analysis of the output of `./gradlew dependencyReport`. I > have attached a graph showing all dependency paths to bytebuddy that result > in the version being overridden, and the only affected paths are test > dependencies on mockito and powermock. > > So based on what I'm seeing, switching to an unvendored bytebuddy would > work today, and the compatibility guarantees provided by bytebuddy (and > ASM) should help ensure it keeps working in the future. > > Additional thoughts and feedback are welcome! Does this address some of > the concerns that have been raised so far? Is there additional testing or > dependency analysis that might be helpful? > > On 2022/04/05 18:59:06 Kenneth Knowles wrote: > > Hmm. Too bad the information on the jira is inadequate to explain or > > justify the change. TBH if faced with a conflict between bytebuddy and > > mockito, working to use mocks less, or in more straightforward ways, > would > > have been my preference. This isn't actually a diamond dep problem that > > impacts users, or I would feel differently. > > > > I guess we want to have some thorough testing & survey of the versions of > > bytebuddy and asm used by transitive deps and any possible breaking > > changes. Should be pretty easy since unvendoring is much easier > > to experiment with. Apologies if this already exists in some other thread > > or on the ticket. > > > > Kenn > > > > On Wed, Mar 23, 2022 at 11:16 AM Reuven Lax wrote: > > > > > We also use ASM directly. If we unshade Bytebuddy, does that also > unshade > > > ASM? Does ASM provide similar stability guarantees? > > > > > > On Wed, Mar 23, 2022 at 10:50 AM Liam Miller-Cushon > > > wrote: > > > > > >> On 2022/03/21 19:36:29 Robert Bradshaw wrote: > > >> > My understanding was that Bytebuddy was originally unvendored, and > we > > >> > vendored it in reaction to version incompatibility issues (despite > the > > >> > promise of API stability). I think we should have a good > justification > > >> > for why we won't get bitten by this again before moving back to > > >> > unvendored. > > >> > > >> Does anyone have context on the specific issues that > > >> https://issues.apache.org/jira/browse/BEAM-1019 was a reaction to? > > >> > > >> The bug was filed in 2016 and mentions upgrading to mockito 2.0. One > > >> potential justification for trying to unvendor is that the issue is > fairly > > >> old, and the library has continued to stabilize, so it might be safer > to > > >> unvendor today than it was in 2016. > > >> > > > > > >
Re: Re: RE: Re: unvendoring bytebuddy
Hmm. Too bad the information on the jira is inadequate to explain or justify the change. TBH if faced with a conflict between bytebuddy and mockito, working to use mocks less, or in more straightforward ways, would have been my preference. This isn't actually a diamond dep problem that impacts users, or I would feel differently. I guess we want to have some thorough testing & survey of the versions of bytebuddy and asm used by transitive deps and any possible breaking changes. Should be pretty easy since unvendoring is much easier to experiment with. Apologies if this already exists in some other thread or on the ticket. Kenn On Wed, Mar 23, 2022 at 11:16 AM Reuven Lax wrote: > We also use ASM directly. If we unshade Bytebuddy, does that also unshade > ASM? Does ASM provide similar stability guarantees? > > On Wed, Mar 23, 2022 at 10:50 AM Liam Miller-Cushon > wrote: > >> On 2022/03/21 19:36:29 Robert Bradshaw wrote: >> > My understanding was that Bytebuddy was originally unvendored, and we >> > vendored it in reaction to version incompatibility issues (despite the >> > promise of API stability). I think we should have a good justification >> > for why we won't get bitten by this again before moving back to >> > unvendored. >> >> Does anyone have context on the specific issues that >> https://issues.apache.org/jira/browse/BEAM-1019 was a reaction to? >> >> The bug was filed in 2016 and mentions upgrading to mockito 2.0. One >> potential justification for trying to unvendor is that the issue is fairly >> old, and the library has continued to stabilize, so it might be safer to >> unvendor today than it was in 2016. >> >
Re: unvendoring bytebuddy
+1 Probably worth to check if we have dependencies that rely on Byte Buddy that can produce conflicts but I doubt it. My only worry was ASM leaking into the classpath, but it seems that Byte Buddy already shades ASM so that should not be an issue. Ismaël On Thu, Mar 17, 2022 at 5:09 PM Liam Miller-Cushon wrote: > > Hello, > > I wanted to raise the possibility of using the upstream version of bytebuddy > in beam, instead of vendoring it, from BEAM-14117. > > Vendoring the bytebuddy dep was introduced in BEAM-1019: > >> We encountered backward incompatible changes in bytebuddy during upgrading >> to Mockito 2.0. Shading bytebuddy helps to address them and future issues. > > > Vendoring makes it harder to upgrade the bytebuddy version, and upgrading > bytebuddy is one of the first things that needs to happen to support new Java > versions (e.g. BEAM-14065, BEAM-12241). > > Vendoring or shading bytebuddy is discouraged by the upstream owners of the > library, see e.g. https://github.com/assertj/assertj-core/issues/2470 where > assertj was migrated off a shaded version: > >> As Byte Buddy retains compatibility, not shading the library would allow >> running recent JVMs without an update of assertj but only BB. Other >> libraries like Mockito or Hibernate do not shade BB and there are no known >> issues with this approach. > > > Does anyone have additional context about the issues encountered during the > mockito 2.0 upgrade, or concerns with trying to unvendor bytebuddy? > > Thanks, > Liam
