Use shading to build the packages

[ifrins]

This PR adds generation of JARs shading the dependencies. It shades io.netty, com.google.code.gson and org.apache.commons into com.turo.pushy.external.

This PR addresses #498. This is especially useful when used with newer versions of Spring Webflux, which use Netty with their Project Reactor, as it's more likely to produce version conflicts.

This also introduces the drawback of breaking source compatibility when using Netty extensions. For example, in my project, I had to rename the import of Netty's Future.

What are your thoughts?

[jchambers]

This also introduces the drawback of breaking source compatibility when using Netty extensions. For example, in my project, I had to rename the import of Netty's Future.

I'm not 100% sure I follow; could you please elaborate?

[jchambers]

…and, much more importantly, thank you for the contribution!

[ifrins]

Yup. Rereading now I've not made myself very clear.

As pushy exports Netty classes, when shaded, the package of the classes wil have changed, thus breaking source compatibility –albeit in a minor way–. Here's the diff when replacing pushy to the shaded version of pushy:

+ import com.turo.pushy.external.io.netty.util.concurrent.Future;
- import io.netty.util.concurrent.Future;

As a suggestion, if this is a concern, maybe there could be two pushy artifacts, the regular one (like the current) and a shaded one.

[jchambers]

Interesting. Yeah, the other (worse?) case would be that something else has Netty on the classpath, so the import io.netty.util.concurrent.Future thing would still work and risk mixing/matching versions in a way that's maybe hard to recognize or untangle.

As a suggestion, if this is a concern, maybe there could be two pushy artifacts, the regular one (like the current) and a shaded one.

If we're going to publish shaded artifacts, I think that's the way to go. Poking quickly at Maven Central, I can find a number of projects that seem to follow that pattern. It's a brave new world for me, though; are you aware of any best practices in terms of publishing shaded/unshaded artifacts?

[ifrins]

Right now, with the changes in this PR users should be able to decide if they want the shaded version by adding: <classifier>shaded</classifier> inside the <dependency> for pushy. I think this is pretty standard behaviour, for example, Spotify's docker-client does it the same way.

I'm not sure if other changes would need to be done for the deployment of the release.

[jchambers]

Ohhhhh yeah. Classifiers. Forgot about those ;)

Cool. I'll definitely have to do some prodding to get the deployment process right, but I think we're on the right track. Thanks kindly!

[justinpermar]

+1 on this PR. I was just about to start working on this one myself when I found this PR. So thank you for your efforts!

[justinpermar]

is it important that netty-tcnative-boringssl-static dependency is not shaded? I'm unclear on how netty searches the classpath at runtime to find this... update: yes, it's important (see below)

[justinpermar]

As you guys already noted, with the shaded jar, Pushy exposes netty classes in a different package, thus breaking source compatibility for users. Consequently, imo this PR needs to expand significantly and modify Pushy to completely hide Netty dependencies from clients. Here is a sample error I get when dropping the shaded jar into my project: [ERROR] error: type mismatch; [ERROR] found : io.netty.channel.nio.NioEventLoopGroup [ERROR] required: com.turo.pushy.external.io.netty.channel.EventLoopGroup [ERROR] .setEventLoopGroup(apnsNettyPool). Note that in this code apnsNettyPool is an instance of io.netty.channel.nio.NioEventLoopGroup. This error is fixed when I instead import com.turo.pushy.external.io.netty.channel.nio.NioEventLoopGroup . However, that is super awkward to do, as I shouldn't even know about this class as a user.

The good news is that these errors demonstrate that there is no need to worry about the concern that @jchambers express above wrt how these shaded classes are seen by client. Meaning that no, clients can't accidentally use these classes.

[justinpermar]

I updated the shade configuration as follows:

<plugin>
    <groupId>org.apache.maven.plugins</groupId>
    <artifactId>maven-shade-plugin</artifactId>
    <version>3.1.0</version>
    <executions>
        <execution>
            <phase>package</phase>
            <goals>
                <goal>shade</goal>
            </goals>
            <configuration>
                <artifactSet>
                    <excludes>
                        <exclude>io.netty:netty-tcnative-boringssl-static</exclude>
                    </excludes>
                </artifactSet>
                <relocations>
                    <relocation>
                        <pattern>io.netty</pattern>
                        <shadedPattern>com.turo.pushy.external.io.netty</shadedPattern>
                        <excludes>
                            <exclude>io.netty.internal.tcnative.*</exclude>
                            <exclude>io.netty.util.internal.NativeLibraryLoader</exclude>
                            <exclude>io.netty.util.internal.NativeLibraryLoader$NoexecVolumeDetector</exclude>
                            <exclude>io.netty.util.internal.NativeLibraryUtil</exclude>
                            <exclude>io.netty.util.internal.NativeLibraryLoader$1</exclude>
                            <exclude>io.netty.util.internal.NativeLibraryLoader$2</exclude>
                        </excludes>
                    </relocation>
                    <relocation>
                        <pattern>com.google.gson</pattern>
                        <shadedPattern>com.turo.pushy.external.com.google.gson</shadedPattern>
                    </relocation>
                    <relocation>
                        <pattern>com.google.code.gson</pattern>
                        <shadedPattern>com.turo.pushy.external.com.google.code.gson</shadedPattern>
                    </relocation>
                    <relocation>
                        <pattern>org.apache.commons</pattern>
                        <shadedPattern>com.turo.pushy.external.org.apache.commons</shadedPattern>
                    </relocation>
                    <relocation>
                        <pattern>org.slf4j</pattern>
                        <shadedPattern>com.turo.pushy.external.org.slf4j</shadedPattern>
                    </relocation>
                </relocations>
                <shadedArtifactAttached>true</shadedArtifactAttached>
                <shadedClassifierName>shaded</shadedClassifierName>
                <transformers>
                    <transformer implementation="org.apache.maven.plugins.shade.resource.DontIncludeResourceTransformer">
                        <resources>
                            <resource>META-INF/maven/io.netty/netty-codec-http2/pom.xml</resource>
                            <resource>META-INF/maven/io.netty/netty-codec-http2/pom.properties</resource>
                            <resource>META-INF/maven/io.netty/netty-codec-http/pom.xml</resource>
                            <resource>META-INF/maven/io.netty/netty-codec-http/pom.properties</resource>
                            <resource>META-INF/maven/io.netty/netty-codec/pom.xml</resource>
                            <resource>META-INF/maven/io.netty/netty-codec/pom.properties</resource>
                            <resource>META-INF/maven/io.netty/netty-handler/pom.properties</resource>
                            <resource>META-INF/maven/io.netty/netty-handler/pom.xml</resource>
                            <resource>META-INF/maven/io.netty/netty-buffer/pom.xml</resource>
                            <resource>META-INF/maven/io.netty/netty-buffer/pom.properties</resource>
                            <resource>META-INF/maven/io.netty/netty-common/pom.properties</resource>
                            <resource>META-INF/maven/io.netty/netty-common/pom.xml</resource>
                            <resource>META-INF/maven/org.jctools/jctools-core/pom.properties</resource>
                            <resource>META-INF/maven/org.jctools/jctools-core/pom.xml</resource>
                            <resource>META-INF/maven/io.netty/netty-handler-proxy/pom.xml</resource>
                            <resource>META-INF/maven/io.netty/netty-handler-proxy/pom.properties</resource>
                            <resource>META-INF/maven/io.netty/netty-transport/pom.properties</resource>
                            <resource>META-INF/maven/io.netty/netty-transport/pom.xml</resource>
                            <resource>META-INF/maven/io.netty/netty-resolver/pom.xml</resource>
                            <resource>META-INF/maven/io.netty/netty-resolver/pom.properties</resource>
                            <resource>META-INF/maven/io.netty/netty-codec-socks/pom.xml</resource>
                            <resource>META-INF/maven/io.netty/netty-codec-socks/pom.properties</resource>
                            <resource>META-INF/maven/com.google.code.gson/gson/pom.xml</resource>
                            <resource>META-INF/maven/com.google.code.gson/gson/pom.properties</resource>
                            <resource>META-INF/maven/commons-codec/commons-codec/pom.xml</resource>
                            <resource>META-INF/maven/commons-codec/commons-codec/pom.properties</resource>
                            <resource>META-INF/maven/org.slf4j/slf4j-api/pom.xml</resource>
                            <resource>META-INF/maven/org.slf4j/slf4j-api/pom.properties</resource>
                        </resources>
                    </transformer>
                </transformers>
            </configuration>
        </execution>
    </executions>
</plugin>

The artifactSet exclusion is critical. Netty needs to find the boringssl on the classpath. This modifies the shaded jar build output to the following: [INFO] Excluding io.netty:netty-tcnative-boringssl-static:jar:2.0.7.Final from the shaded jar. I also excluded the classes from the io.netty relocation declaration as well (to resolve IllegalAccessExceptions thrown from shaded netty classes trying to load the boringssl classes which are, due to the shading, in different packages). I also excluded the pom.xml and pom.properties files from the shaded jar (as they are included in each library's original jar and would conflict).

Also, pointing out one technical wrinkle with this configuration... the exclusions of NativeLibraryLoader and friends means that the shaded jar includes those classes in their original package. E.g.,

io/netty/util/internal/NativeLibraryUtil.class
io/netty/util/internal/NativeLibraryLoader$2.class
io/netty/util/internal/NativeLibraryLoader$1.class
io/netty/util/internal/NativeLibraryLoader.class
io/netty/util/internal/NativeLibraryLoader$NoexecVolumeDetector.class

Therefore, there will still be conflicts with the original jar they come from (netty-common-.jar). I can't think of an easy solution to this problem, but perhaps one solution is for users of the shaded jar only to specify the version of netty common they want to use and for the shading config to exclude the this artifact (so the classes will not be in the resulting jar and must be supplied by the user).

[justinpermar]

Also, users will need to exclude the dependencies from the shaded jar, as the default POM is published to maven (and used by clients) (and the default POM lists the netty, etc. deps):

<dependency>
    <groupId>com.turo</groupId>
    <artifactId>pushy</artifactId>
    <version>${pushy.version}</version>
    <classifier>shaded</classifier>
    <exclusions>
        <exclusion>
            <groupId>io.netty</groupId>
            <artifactId>*</artifactId>
        </exclusion>
        <exclusion>
            <groupId>com.google.code.gson</groupId>
            <artifactId>gson</artifactId>
        </exclusion>
        <exclusion>
            <groupId>commons-codec</groupId>
            <artifactId>commons-codec</artifactId>
        </exclusion>
        <exclusion>
            <groupId>org.slf4j</groupId>
            <artifactId>slf4j-api</artifactId>
        </exclusion>
    </exclusions>
</dependency>

Also, users will need to explicitly depend on the boringssl dep:

<dependency>
    <groupId>io.netty</groupId>
    <artifactId>netty-tcnative-boringssl-static</artifactId>
    <version>2.0.7.Final</version>
</dependency>

[justinpermar]

FWIW, I have a project that uses Pushy and Twitter Finagle (which also uses Netty). I am able to compile my project and run everything successfully with the shaded jar (using my versions/changes I put in comments above). Having thought about this a bit more, I think the only way to do this properly is to update Pushy API to completely hide Netty. Then a shaded jar can be used without source modification (and Pushy's dependencies will be completely hidden from users, which is a huge benefit to everyone already using Netty elsewhere).

[jchambers]

…I think the only way to do this properly is to update Pushy API to completely hide Netty.

I'm coming to the same conclusion, but am worried that the cure is worse than the disease. The two big things that we expose publicly (and deliberately) are:

1. io.netty.util.concurrent.Future, which notably allows users to attach listeners to futures. Deriving from that is the set of ...FutureListener interfaces. My enthusiasm for reinventing that set of wheels is a little low.
2. io.netty.channel.EventLoopGroup, which I recognize is simultaneously most useful and most problematic for users combining Pushy with other Netty-based things. It's also important for users with lots of clients in play at the same time.

I guess we can resolve the first problem as things stand now, though I'm not entirely convinced the benefit is worth the cost. To address the "many clients" part of the second problem, I think we'd need to get #540 done so callers would still have a clear mechanism for controlling overall thread usage.

In all, I recognize that this is valuable for many users, but I think the costs are high and there are some big tasks to finish before we can seriously consider merging it. I say this not to shut down the effort, but to be clear that it will probably take significant time and effort before we can merge it.

Seem legit? Am I missing anything?

[jchambers]

Some new upstream developments on the shading front:

• https://github.com/netty/netty-tcnative/pull/382/
• https://github.com/netty/netty/issues/7272
• https://github.com/netty/netty/pull/6995

[jchambers]

io.netty.util.concurrent.Future, which notably allows users to attach listeners to futures. Deriving from that is the set of ...FutureListener interfaces. My enthusiasm for reinventing that set of wheels is a little low.

Now that we've moved to Java 8 in #746, I think we have a reasonable path to getting around this obstacle. We might be able to use CompletableFuture for all public methods that currently return an io.netty.util.concurrent.Future. It offers all of the same affordances, but is more Java-y. I think moving to CompletableFuture is probably a good move regardless, but did want to mention it here since I think it would knock down one of the two remaining obstacles to shading.

[jchambers]

We might be able to use CompletableFuture for all public methods…

This is covered in #757.