New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure multiple shaded version of the same netty artifact can be load… #8207

Merged
merged 1 commit into from Aug 21, 2018

Conversation

@normanmaurer
Member

normanmaurer commented Aug 20, 2018

…ed as long as the shaded prefix is different

Motivation:

We should support to load multiple shaded versions of the same netty artifact as netty is often used in multiple dependencies.

This is related to #7272.

Modifications:

  • Use -fvisibility=hidden when compiling and use JNIEXPORT for things we really want to have exported
  • Ensure fields are declared as static so these are not exported
  • Adjust testsuite-shading to use install_name_tool on MacOS to change the id of the lib. Otherwise the wrong may be used.

Result:

Be able to use multiple shaded versions of the same netty artifact.

@normanmaurer normanmaurer added this to the 4.1.29.Final milestone Aug 20, 2018

@normanmaurer normanmaurer self-assigned this Aug 20, 2018

@carl-mastrangelo

Can't comment too much on the pom file (not an xml programmer ;) )

The C code looks good from what i can tell. How did you find all the non-static symbols?

also LGTM

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Aug 20, 2018

Member

@carl-mastrangelo nm -gU /path/to/libname.dylib (on macOS) ... This print all exported which listed a lot of stuff that could cause strange side-effects if picked up ;)

Member

normanmaurer commented Aug 20, 2018

@carl-mastrangelo nm -gU /path/to/libname.dylib (on macOS) ... This print all exported which listed a lot of stuff that could cause strange side-effects if picked up ;)

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Aug 20, 2018

Member

Also for the record I am working on something similar for netty-tcnative atm :)

Member

normanmaurer commented Aug 20, 2018

Also for the record I am working on something similar for netty-tcnative atm :)

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Aug 20, 2018

Member

@carl-mastrangelo @Scottmitch PTAL again... I am just using JNIEXPORT now to export the stuff we need which will do the correct thing depending in the compiler 🎉

Member

normanmaurer commented Aug 20, 2018

@carl-mastrangelo @Scottmitch PTAL again... I am just using JNIEXPORT now to export the stuff we need which will do the correct thing depending in the compiler 🎉

// http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-February/014549.html
// Invoked by the JVM when statically linked
JNIEXPORT jint JNI_OnLoad_netty_transport_native_kqueue(JavaVM* vm, void* reserved) {

This comment has been minimized.

@Scottmitch

Scottmitch Aug 20, 2018

Member

so nice of jni headers to define the export we needed :)

@Scottmitch

Scottmitch Aug 20, 2018

Member

so nice of jni headers to define the export we needed :)

@ejona86

This -fvisibility=hidden + JNIEXPORT looks much better than earlier. I had glanced earlier and saw the attributes and found it to be strange. "I've always seen this use a macro," I said to myself. 😄

@ejona86

This comment has been minimized.

Show comment
Hide comment
@ejona86

ejona86 Aug 21, 2018

Contributor

@normanmaurer, don't forget to update the commit message now that you swapped to JNIEXPORT.

Contributor

ejona86 commented Aug 21, 2018

@normanmaurer, don't forget to update the commit message now that you swapped to JNIEXPORT.

@carl-mastrangelo

Still LGTM

Ensure multiple shaded version of the same netty artifact can be load…
…ed as long as the shaded prefix is different

Motivation:

We should support to load multiple shaded versions of the same netty artifact as netty is often used in multiple dependencies.

This is related to #7272.

Modifications:

- Use -fvisibility=hidden when compiling and use JNIEXPORT for things we really want to have exported
- Ensure fields are declared as static so these are not exported
- Adjust testsuite-shading to use install_name_tool on MacOS to change the id of the lib. Otherwise the wrong may be used.

Result:

Be able to use multiple shaded versions of the same netty artifact.

@normanmaurer normanmaurer merged commit ea4c315 into 4.1 Aug 21, 2018

1 check was pending

continuous-integration/teamcity Started TeamCity Build pull requests :: netty
Details

@normanmaurer normanmaurer deleted the multiple_native_shaded branch Aug 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment