On Wed, Jul 6, 2011 at 6:09 PM, Stuart McCulloch <[email protected]> wrote:
> On 6 Jul 2011, at 17:53, Fred Faber wrote: > > I think about the trade-off between the flexibility hiding the libraries > gives with respect to the maintenance of Guice (as seen by its active > maintainers, such as Sam) versus the arguments of standardization / > transparency (note I really don't buy the "code size" argument here). > > I think it'd be rare to need a security patch for Guava if Guice continues > in the mode of not aggressively using Guava from head, or its latest > release. This implies that a security hole would need to be identified to a > stable version of Guava, relatively long after it was released. > > I do appreciate the arguments that proguarding for sake of hiding the > internals from the (savvy) users is probably unneeded. > > Bumping to the top of the thread: mcculls was talking about the 300k > difference in Guice from head versus 3.0 as a main motivator for using > proguard (over jarjar) as one motivation to move to application-level > proguard, and the fragility argument as another. About the latter: what > extensions and tests were failing here (guice tests, I'm assuming)? > > > Both Guice tests and (official) Guice extensions. This is because the > shrinking and embedding of Guava is currently done in the core sub-project, > before any of the tests or extensions have been built. > > To take a real example, Guice core doesn't use Guava's > "Lists.newArrayList(Object)" method but the grapher extension does. So when > we use ProGuard on Guice core, it thinks it can throw this method away when > embedding Guava. The build then continues on to build the extensions, and > update any references it finds there to Guava classes to match those > embedded in Guice core. Note that it doesn't check to see if the updated > package/method actually exists in the core - it just renames packages on the > bytecode level. It's only when you test the final grapher extension jar > against the final Guice core jar that you see a linkage error (because the > method the grapher expects to be there has been removed). What's worse is > that you may only see this error when you exercise the relevant code, hence > my concern about fragility (of course this could always be handled with > additional tooling to check all method references from extensions exist in > the core). > > Note that this could conceivably happen right now if an extension used a > class from Guava that wasn't referenced (directly or indirectly) from Guice > core. But the odds turn out to be much lower because jarjar only ever > removes whole classes not methods, which is why we end up embedding most of > the common Guava collection classes - which then covers the methods > referenced from the extensions. > > The cleanest approach imho would be to postpone any jarjar / Proguard until > the very end, then you can apply it to everything in one shot. Maybe also > have another round of integration tests to verify. > > Would it be sufficient to include Guava as an external dependency for the > test suite, treating them as a client of Guice? Similarly for extensions? > > > You mean make Guava an external dependency of the extensions, and not > jarjar any Guava references found in them? That would be another option, but > I think it would confuse people since they'd need to remember to add Guava > as well as the extension they actually wanted. > Yes. And mind you I'm enjoying a game of devil's advocate right now, but folks would have to remember to add Guava as a dep when using Guice proper, and so I don't think doing so when adding extensions would be much worse. Especially given extensions could declare other deps as well. > > Fred > > On Wed, Jul 6, 2011 at 11:28 AM, TJ Rothwell <[email protected]>wrote: > >> +1 Sam (use proguard to hide implementation details) >> >> -- TJ >> >> On Wed, Jul 6, 2011 at 9:41 AM, Tim Peierls <[email protected]> wrote: >> > I'm with Sam and Alen. >> > I use Guice and Guava in everything. To program otherwise, imho, is to >> tie >> > your hands behind your back. So you'd think I wouldn't mind having one >> > depend on the other, but I persist in seeing them as completely >> independent >> > tools. The fact that one uses bits of the other internally doesn't >> matter to >> > me, as long as it can be arranged that it doesn't contribute greatly to >> the >> > jar size. >> > --tim >> > >> > >> > On Wed, Jul 6, 2011 at 10:12 AM, Sam Berlin <[email protected]> wrote: >> >> >> >> So it does look like a tidal wave of support for making it a public >> >> dependency, with a few lone dissenters. I'll try to lay out the >> reasons I >> >> think making it a public dependency isn't as great as it sounds, but >> that >> >> said -- I won't stand in the way of doing it. >> >> >> >> First, the only reasons why a public dependency is good: It reduces >> code >> >> duplication. >> >> >> >> To me, that's the only reason I understand. I frankly don't understand >> >> any of the other reasons. Here's a few that were listed: >> >> >> >> a) Wanting to upgrade the packaged dependencies. >> >> In my head, this is equivalent to saying, "I'd like to take the >> Guice >> >> source and modify it a bit." The fact it uses Guava is entirely, 100% >> an >> >> implementation detail. Why would you ever need to upgrade the packaged >> >> dependency? It's not something that should matter to a user. >> >> >> >> b) A reason for wanting to upgrade dependencies: security/performance. >> >> Again, this seems like it applies equally true to the core Guice >> code >> >> itself. If you're going to be inspecting implementation details of a >> >> library, you may as well go into the source & build your own >> distribution of >> >> it. >> >> >> >> c) Not trusting Jarjar. >> >> Sorry, this just isn't a valid reason to me. JarJar works. It >> does >> >> exactly what you're saying: recursive dependency analysis. It reads >> the >> >> bytecode for classes you're asking to move and makes sure that any >> >> referenced classes are found in the right place. Of course it fails on >> >> reflection, but you just don't use it for things that need reflection. >> I >> >> haven't worked with ProGuard much, but AFAIK, a whole ton of apps use >> it, >> >> and it works fine. >> >> >> >> d) A complete copy of Guava is inside Guice >> >> If this were the case, I'd be inclined to go along with the >> prevailing >> >> sentiment.. but it's not true. Guava itself is 1.1mb. Guice after the >> >> change to use JarJar'd Guava is ~1mb. That's clearly not the whole of >> >> Guava. In fact, it's just bits of two packages: >> >> com.google.common.{base,collect} and two classes from >> com.google.common.io. >> >> A tiny fraction of Guava. >> >> >> >> The reasons why I think exposing the dependency is a bad idea: >> >> >> >> 1) Where do we stop? Should we expose cglib & asm as real dependencies >> >> that can be upgraded too (real question)? That exposes the means by >> which >> >> Guice is doing its bytecode manipulation for grabbing line numbers & >> for >> >> AOP. If Guice decides to change how it does it, then people may have a >> >> false dependency/requirement on cglib & asm. >> >> >> >> 2) It's as if com.google.inject.internal APIs were exposed -- its >> >> something that Guice should be free to change without any user having >> to >> >> take any action at all, but now we're forcing them to think & act on >> it. >> >> >> >> 3) Guice has averaged 2 year releases (for better or worse). Guava >> gives >> >> ~1.5 year leeway before removing any APIs Should we be forced to >> release a >> >> new version of Guice because a dependency changed its API? If we don't >> >> release a new one, that gets into some severe versioning hell. >> >> >> >> sam >> >> >> >> >> >> On Tue, Jul 5, 2011 at 9:30 PM, Bob Lee <[email protected]> wrote: >> >>> >> >>> +1 to making Guava a public dependency! It sounds like we're almost >> all >> >>> in agreement. >> >>> Bob >> >>> >> >>> -- >> >>> You received this message because you are subscribed to the Google >> Groups >> >>> "google-guice" group. >> >>> To post to this group, send email to [email protected]. >> >>> To unsubscribe from this group, send email to >> >>> [email protected]. >> >>> For more options, visit this group at >> >>> http://groups.google.com/group/google-guice?hl=en. >> >> >> >> -- >> >> You received this message because you are subscribed to the Google >> Groups >> >> "google-guice" group. >> >> To post to this group, send email to [email protected]. >> >> To unsubscribe from this group, send email to >> >> [email protected]. >> >> For more options, visit this group at >> >> http://groups.google.com/group/google-guice?hl=en. >> > >> > -- >> > You received this message because you are subscribed to the Google >> Groups >> > "google-guice" group. >> > To post to this group, send email to [email protected]. >> > To unsubscribe from this group, send email to >> > [email protected]. >> > For more options, visit this group at >> > http://groups.google.com/group/google-guice?hl=en. >> > >> >> -- >> You received this message because you are subscribed to the Google Groups >> "google-guice" group. >> To post to this group, send email to [email protected]. >> To unsubscribe from this group, send email to >> [email protected]. >> For more options, visit this group at >> http://groups.google.com/group/google-guice?hl=en. >> >> > > -- > You received this message because you are subscribed to the Google Groups > "google-guice" group. > To post to this group, send email to [email protected]. > To unsubscribe from this group, send email to > [email protected]. > For more options, visit this group at > http://groups.google.com/group/google-guice?hl=en. > > > -- > You received this message because you are subscribed to the Google Groups > "google-guice" group. > To post to this group, send email to [email protected]. > To unsubscribe from this group, send email to > [email protected]. > For more options, visit this group at > http://groups.google.com/group/google-guice?hl=en. > -- You received this message because you are subscribed to the Google Groups "google-guice" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/google-guice?hl=en.
