Shade platform specific libraries into a single jar #33
Conversation
|
Companion netty PR: netty/netty#3502 |
| A Mavenized fork of Tomcat Native which incorporates various patches. | ||
|
|
||
| Building this artifact produces a native library with a classifier matching the | ||
| os/arch of the build machine. (The Java JNI bindings are exported with the 'java' classifier) |
There was a problem hiding this comment.
move the (...) stuff before the .
There was a problem hiding this comment.
actually, i'll just remove the parens
|
@cconroy please address my comments |
|
@trustin @normanmaurer one thing I meant to call out: In our discussion on the issue @trustin noted that it would make sense to keep the java classes in each architecture specific artifact to support the non-shaded use case (e.g. if a user only wants to pull in one platform). In this PR, I have split java out into a separate artifact classifier that is built on any platform. This should address that use case while avoiding duplicate class files. |
cconroy
force-pushed
the
shade-platforms
branch
from
4f1b9c1
to
e35cf2e
Compare
| </configuration> | ||
| </execution> | ||
| <execution> | ||
| <!-- Also produce a jar with 'java' classifier. This can be built on any platform. --> |
There was a problem hiding this comment.
I thought we build the JAR without native libraries with no classifier already? Could you explain why we need this?
There was a problem hiding this comment.
Under this model, the non-classified jar is produced by the shade pom to produce a jar that can be used on any platform. Here, the antrun plugin produces the platform-specific jar. This extra execution tells maven to package up the normal target results as a 'java' package.
|
I like the idea of providing a shaded JAR that contains all native libraries. How about:
One problem I see with the |
|
@trustin: i'm not too familiar with the maven release plugin. Unfortunately, maven does not let me use a property as a version tag. I'll dig around to see what I can find... At the very least we could have a clear README with release steps or perhaps a shell script to automate? |
|
"Make the netty-tcnative jar with no classifier (the default jar) include all supported native libraries. i.e. most users can just pull the default JAR and that should be all." Yep! This is what happens when you run the shaded target. I encourage you to try it out (you can test locally by commenting out the other platforms to just shade your local platform after building the platform-specific libs). |
|
"Keep the JARs with the OS classifiers as before." IMHO it's cleaner to separate the java and platform-specific artifacts for those users that don't want the shaded jar. I would personally prefer to add a second dep rather than get duplicate class files. Historically I've seen build failures and other problems (e.g. with IDEs, other build integration tools) occur with duplicate classes. Most users will probably not care about a slightly larger jar and will just use the shaded one anyways. We should add documentation to clarify how to depend on a single platform. If you're okay with keeping them separate, I can update the readme to reflect. If not, I can try to spin a version with duplicate classes. |
|
@trustin: realistically, i don't think you probably want to update the shade and platform specific numbers at the same time. You'll be releasing platform specific libs from the same platform machines. Once all of those are updated, you'll want to do a shade release. I don't see a path for doing this automatically with the release plugin. |
|
@cconroy Agreed. The build process of the all-platform JAR will be more like fetching & merging all JARs of the already-deployed JARs. I guess we don't need a pom for this. Just a shell script should do the job. Let apply your changes partially and write one. |
|
AUTOMATED MESSAGE for ADMINS: |
|
This should be good to merge? |
|
@trustin @normanmaurer ping |
|
@trustin @normanmaurer can we please merge this? |
|
Sorry will check tomorrow... Busy as hell :/
|
|
@cconroy @normanmaurer I'll take care of this as well as the |
|
Now that we have the statically linked boring ssl artifacts I'd like to bring up what we think the 'default' artifact should be. We theoretically have {platforms} X [openssl, libre, boring] X [dynamic, static] Based on the current plan a mega jar with boring-static is the library most likely to work out of the box for all features for developers when compared to a shared openssl one. Switching to this however can't be done without a certain amount of messaging. In particular the lifecycle around managing security patches to openssl interacts with this decision as sys admins may have the latent expectation that updating shared openssl is their primary tool for resolving security issues. I suppose most admins have become used to updating Java to resolve issues too so this may be less of a concern if we were to switch to boring-static as the default but its worth mentioning. |
|
Hey guys, Let's discuss. |
|
@nmittler Thank you so much for taking a stab on this! |
|
@cconroy now that @nmittler has broken the ice how would you go about adapting this pull request to support other libraries such as openssl dynamic fedora etc. I have an open PR on the netty side here netty/netty#4925 |
|
@johnou This pull request is over a year old despite having general agreement on the approach so any other changes should probably be done in a subsequent PR @normanmaurer any updates here? |
|
@nmittler I think we can close this now as we have the boringssl static uberjar. WDYT ? |
|
Yeah sgtm ... closing. |
os-arch classifiers.
artifact.
#24