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. > 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.
