GoogleCloudPlatform / appengine-java-vm-runtime

Apache License 2.0
67 stars 34 forks source link

Is there a reason for using a repacked version of guava #253

Open aozarov opened 8 years ago

aozarov commented 8 years ago

see this list:

Targets
    Occurrences of 'repackage' in Project with mask '*.java'
Found Occurrences  (25 usages found)
    AppEngineWebXml.java  (3 usages found)
        19import com.google.appengine.repackaged.com.google.common.base.CharMatcher;
        20import com.google.appengine.repackaged.com.google.common.collect.Lists;
        21import com.google.appengine.repackaged.com.google.common.collect.Maps;
    FakeableVmApiProxyDelegate.java  (1 usage found)
        18import com.google.appengine.repackaged.com.google.protobuf.MessageLite;
    MultipartMimeUtils.java  (1 usage found)
        18import com.google.appengine.repackaged.com.google.common.io.ByteStreams;
    RemoteApiSharedTests.java  (4 usages found)
        27import com.google.appengine.repackaged.com.google.common.base.Supplier;
        28import com.google.appengine.repackaged.com.google.common.collect.LinkedListMultimap;
        29import com.google.appengine.repackaged.com.google.common.collect.Lists;
        30import com.google.appengine.repackaged.com.google.common.collect.Multimap;
    Restricted.java  (2 usages found)
        19 * We repackage this class into com.google.appengine.repackaged so that
        19 * We repackage this class into com.google.appengine.repackaged so that
    SessionManager.java  (1 usage found)
        18import static com.google.appengine.repackaged.com.google.common.io.BaseEncoding.base64Url;
    TestRepackagedAccess.java  (4 usages found)
        19 * {@link Restricted} gets repackaged into <code>com.google.appengine.repackaged</code>
        19 * {@link Restricted} gets repackaged into <code>com.google.appengine.repackaged</code>
        24public class TestRepackagedAccess implements Runnable {
        28"Class " + TestRepackagedAccess.class.getName() + "$%d loaded from";
    VmApiProxyDelegate.java  (1 usage found)
        30import com.google.appengine.repackaged.com.google.common.collect.Lists;
    VmApiProxyDelegateTest.java  (1 usage found)
        26import com.google.appengine.repackaged.com.google.common.collect.ImmutableMap;
    VmAppLogsWriter.java  (1 usage found)
        18import com.google.appengine.repackaged.com.google.common.base.Stopwatch;
    VmRequestThreadFactory.java  (4 usages found)
        18import static com.google.appengine.repackaged.com.google.common.base.Preconditions.checkArgument;
        19import static com.google.appengine.repackaged.com.google.common.base.Preconditions.checkState;
        21import com.google.appengine.repackaged.com.google.common.collect.ImmutableList;
        22import com.google.appengine.repackaged.com.google.common.collect.Lists;
    VmRuntimeUtils.java  (1 usage found)
        18import static com.google.appengine.repackaged.com.google.common.base.MoreObjects.firstNonNull;
    XmlUtils.java  (1 usage found)
        18import com.google.appengine.repackaged.com.google.common.io.Files;

We already depend on many other third-party libraries including apache httpcore, httpclient, commons-logging, commons-code and also google json. Any reason for treating guava differently?

aozarov commented 8 years ago

Also, I would expect that even though the runtime may depend on guava (or any other library) the application would be able to use their own independent version as they run in a separate/clean class-loader? /cc @gregw

gregw commented 8 years ago

By default, any extra classes put on the jetty server class path are exposed to the webapplication. But it is an easy thing to configure in jetty and we have the concepts of:

So I very much prefer to use all dependencies directly and not to rebundle and/or rename them.

We probably need to do a bit of work to improve the configuration of server-classes so that such classes are hidden (either at their normal names or even any renaming we do).

gregw commented 8 years ago

So the repackaging is already done in the appengine-api-1.0-sdk-1.9.38.jar that we consume from maven repository:

com/google/appengine/api/
com/google/appengine/repackaged/com/fasterxml/jackson/
com/google/appengine/repackaged/com/google/api/client/
com/google/appengine/repackaged/com/google/api/client/util/store/
com/google/appengine/repackaged/com/google/appengine/api/search/
com/google/appengine/repackaged/com/google/common/
com/google/appengine/repackaged/com/google/datastore/
com/google/appengine/repackaged/com/google/gson/
com/google/appengine/repackaged/com/google/io/
com/google/appengine/repackaged/com/google/net/
com/google/appengine/repackaged/com/google/protobuf/
com/google/appengine/repackaged/com/google/protos
com/google/appengine/repackaged/com/google/rpc/
com/google/appengine/repackaged/com/google/storage/
com/google/appengine/repackaged/com/google/type/
com/google/appengine/repackaged/org/antlr/
com/google/appengine/repackaged/org/apache/commons/codec/
com/google/appengine/repackaged/org/codehaus/jackson/
com/google/appengine/repackaged/org/joda/time/
com/google/appengine/spi/
com/google/appengine/tools/
com/google/apphosting/api/
com/google/apphosting/base/
com/google/apphosting/client/datastoreservice/
com/google/apphosting/client/searchservice/
com/google/apphosting/client/serviceapp/
com/google/apphosting/datastore/
com/google/apphosting/utils/remoteapi/
com/google/apphosting/utils/servlet/
com/google/cloud/sql/jdbc/
com/google/cloudsearch/
com/google/protos/cloud/sql/
com/google/storage/onestore/
javax/activation/
javax/cache/
javax/mail/
javax/mail/event/
javax/mail/internet/
javax/mail/search/
javax/mail/util/
org/apache/geronimo/mail/

Note that it appears very inconsistent with what is or is not repackaged within that jar as some com.google packages are just bundled and not repackaged.

However, I'm guessing there is not much that we can do about this, unless there is a version of that artefact available in maven that does not bundle/repackage its dependencies? I've had a quick look and could not find one.

Long term it would be best to avoid bundling dependencies and impls within an API jar

Mid term it would be good to ask for a pur api jar artefact without bundled dependencies, so we could at least pull in and manage dependencies ourselves.

Short term we can probably not do much to avoid the repackaging, unless we are prepared to strip the jar and rewrite classes back to using the unrepackaged versions. So I will put this on hold for now and work on #259 to at least hide the dependencies from the webapp