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

Only load native transport if running architecture match the compiled… #7163

Closed
wants to merge 1 commit into
base: 4.1
from

Conversation

@normanmaurer
Member

normanmaurer commented Aug 29, 2017

… library architecture.

Motivation:

We should only try to load the native artifacts if the architecture we are currently running on is the same as the one the native libraries were compiled for.

Modifications:

Include architecture in native lib name and append the current arch when trying to load these. This will fail then if its not the same as the arch of the compiled arch.

Result:

Fixes [#7150].

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer
Member

normanmaurer commented Aug 29, 2017

@normanmaurer normanmaurer self-assigned this Aug 29, 2017

@normanmaurer normanmaurer added the defect label Aug 29, 2017

@normanmaurer normanmaurer added this to the 4.0.52.Final milestone Aug 29, 2017

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer
Member

normanmaurer commented Aug 29, 2017

also @johnou

@doom369

This comment has been minimized.

Show comment
Hide comment
@doom369

doom369 Aug 29, 2017

Contributor

Thanks! Will try to check soon.

Contributor

doom369 commented Aug 29, 2017

Thanks! Will try to check soon.

@@ -111,7 +111,8 @@ private static void loadNativeLibrary() {
if (!name.startsWith("mac") && !name.contains("bsd") && !name.startsWith("darwin")) {

This comment has been minimized.

@michaelandrepearce

michaelandrepearce Aug 29, 2017

Contributor

Should this be replaced with calls to PlatformDependent to get this information? And missing ones added there?

@michaelandrepearce

michaelandrepearce Aug 29, 2017

Contributor

Should this be replaced with calls to PlatformDependent to get this information? And missing ones added there?

This comment has been minimized.

@normanmaurer

normanmaurer Aug 29, 2017

Member

maybe... I did want to keep the change focused

@normanmaurer

normanmaurer Aug 29, 2017

Member

maybe... I did want to keep the change focused

This comment has been minimized.

@michaelandrepearce

michaelandrepearce Aug 29, 2017

Contributor

Ok no worries any obj if i go do this after then? new pr post merge of this?

@michaelandrepearce

michaelandrepearce Aug 29, 2017

Contributor

Ok no worries any obj if i go do this after then? new pr post merge of this?

This comment has been minimized.

@normanmaurer

normanmaurer Aug 29, 2017

Member

let us merge this one first and then we can see if it makes sense to move some of the stuff (I think it may make sense tho ;))

@normanmaurer

normanmaurer Aug 29, 2017

Member

let us merge this one first and then we can see if it makes sense to move some of the stuff (I think it may make sense tho ;))

@@ -196,7 +196,8 @@ private static void loadNativeLibrary() {
if (!name.startsWith("linux")) {

This comment has been minimized.

@michaelandrepearce

michaelandrepearce Aug 29, 2017

Contributor

Should this be replaced with calls to PlatformDependent to get this information (duplicated with TC native)? And missing ones added there?

@michaelandrepearce

michaelandrepearce Aug 29, 2017

Contributor

Should this be replaced with calls to PlatformDependent to get this information (duplicated with TC native)? And missing ones added there?

This comment has been minimized.

@normanmaurer

normanmaurer Aug 29, 2017

Member

maybe... I did want to keep the change focused ;)

@normanmaurer

normanmaurer Aug 29, 2017

Member

maybe... I did want to keep the change focused ;)

Set<String> libNames = new LinkedHashSet<String>(4);
// First, try loading the platform-specific library. Platform-specific
// libraries will be available if using a tcnative uber jar.
libNames.add("netty_tcnative_" + os + '_' + arch);
if (LINUX.equalsIgnoreCase(os)) {
if ("linux".equalsIgnoreCase(os)) {

This comment has been minimized.

@michaelandrepearce

michaelandrepearce Aug 29, 2017

Contributor

Should this be replaced with calls to PlatformDependent to get if linux?

@michaelandrepearce

michaelandrepearce Aug 29, 2017

Contributor

Should this be replaced with calls to PlatformDependent to get if linux?

This comment has been minimized.

@normanmaurer

normanmaurer Aug 29, 2017

Member

maybe... I did want to keep the change focused ;)

@normanmaurer

normanmaurer Aug 29, 2017

Member

maybe... I did want to keep the change focused ;)

@michaelandrepearce

Some comments on some possible improvements in removing re-locating some code to PlatformDependent class. But if disagree with that, the code looks good. And does what i was thinking.

@@ -179,7 +179,7 @@
<addDefaultImplementationEntries>true</addDefaultImplementationEntries>
</manifest>
<manifestEntries>
<Bundle-NativeCode>META-INF/native/libnetty_transport_native_epoll.so; osname=linux; processor=x86_64,*</Bundle-NativeCode>
<Bundle-NativeCode>META-INF/native/libnetty_transport_native_epoll_${os.detected.arch}.so; osname=linux; processor=${os.detected.arch},*</Bundle-NativeCode>

This comment has been minimized.

@johnou

johnou Aug 29, 2017

Contributor

👌

@johnou

johnou Aug 29, 2017

Contributor

👌

This comment has been minimized.

@Scottmitch

Scottmitch Aug 31, 2017

Member

This will break folks that are shading ... I don't have a problem with this but just good to note.

@Scottmitch

Scottmitch Aug 31, 2017

Member

This will break folks that are shading ... I don't have a problem with this but just good to note.

This comment has been minimized.

@normanmaurer

normanmaurer Aug 31, 2017

Member

@Scottmitch imho how these files are named is an implementation detail so it should be ok ?

@normanmaurer

normanmaurer Aug 31, 2017

Member

@Scottmitch imho how these files are named is an implementation detail so it should be ok ?

This comment has been minimized.

@ejona86

ejona86 Aug 31, 2017

Contributor

When shading you have to rename them, that means having some level of knowledge how they are named.

The recent change from - to _ broke existing configuration for shading as well. Breakages like that should be "okay," but avoiding useless breakages would be good. This breakage seems unavoidable.

@ejona86

ejona86 Aug 31, 2017

Contributor

When shading you have to rename them, that means having some level of knowledge how they are named.

The recent change from - to _ broke existing configuration for shading as well. Breakages like that should be "okay," but avoiding useless breakages would be good. This breakage seems unavoidable.

This comment has been minimized.

@normanmaurer

normanmaurer Aug 31, 2017

Member

@ejona86 agree, we should only do if there is a good reason. I think this is one.

@normanmaurer

normanmaurer Aug 31, 2017

Member

@ejona86 agree, we should only do if there is a good reason. I think this is one.

This comment has been minimized.

@johnou

johnou Aug 31, 2017

Contributor

@normanmaurer worth while a mention in the release notes then 👍

@johnou

johnou Aug 31, 2017

Contributor

@normanmaurer worth while a mention in the release notes then 👍

@@ -179,7 +179,7 @@
<addDefaultImplementationEntries>true</addDefaultImplementationEntries>
</manifest>
<manifestEntries>
<Bundle-NativeCode>META-INF/native/libnetty_transport_native_epoll.so; osname=linux; processor=x86_64,*</Bundle-NativeCode>
<Bundle-NativeCode>META-INF/native/libnetty_transport_native_epoll_${os.detected.arch}.so; osname=linux; processor=${os.detected.arch},*</Bundle-NativeCode>

This comment has been minimized.

@michaelandrepearce

michaelandrepearce Aug 29, 2017

Contributor

an alternative option is, could remove <platform>.</platform> hawtjni will then place these in folders correctly based on the arch. What is being done though also works.

@michaelandrepearce

michaelandrepearce Aug 29, 2017

Contributor

an alternative option is, could remove <platform>.</platform> hawtjni will then place these in folders correctly based on the arch. What is being done though also works.

This comment has been minimized.

@normanmaurer

normanmaurer Aug 29, 2017

Member

this would have been more complex as we would also need to adjust our code that loads the native libs etc.

@normanmaurer

normanmaurer Aug 29, 2017

Member

this would have been more complex as we would also need to adjust our code that loads the native libs etc.

This comment has been minimized.

@michaelandrepearce

michaelandrepearce Aug 29, 2017

Contributor

Fair enough, as noted this approach works +1

@michaelandrepearce

michaelandrepearce Aug 29, 2017

Contributor

Fair enough, as noted this approach works +1

Only load native transport if running architecture match the compiled…
… library architecture.

Motivation:

We should only try to load the native artifacts if the architecture we are currently running on is the same as the one the native libraries were compiled for.

Modifications:

Include architecture in native lib name and append the current arch when trying to load these. This will fail then if its not the same as the arch of the compiled arch.

Result:

Fixes [#7150].
@johnou

johnou approved these changes Aug 29, 2017

}
if (value.startsWith("os400")) {
// Avoid the names such as os4000
if (value.length() <= 5 || !Character.isDigit(value.charAt(5))) {

This comment has been minimized.

@netkins

netkins Aug 29, 2017

MAJOR Merge this if statement with the enclosing one. rule

@netkins

netkins Aug 29, 2017

MAJOR Merge this if statement with the enclosing one. rule

This comment has been minimized.

@normanmaurer

normanmaurer Aug 29, 2017

Member

I would like to keep it as it is for now as the code was just moved and it kind of mirrors what is in the maven-os-plugin.

@normanmaurer

normanmaurer Aug 29, 2017

Member

I would like to keep it as it is for now as the code was just moved and it kind of mirrors what is in the maven-os-plugin.

This comment has been minimized.

@Scottmitch

Scottmitch Aug 31, 2017

Member

It would be nice to consolidate this platform logic somewhere ... seems like we keep copying/moving it around.

@Scottmitch

Scottmitch Aug 31, 2017

Member

It would be nice to consolidate this platform logic somewhere ... seems like we keep copying/moving it around.

This comment has been minimized.

@normanmaurer

normanmaurer Aug 31, 2017

Member

agree but the os plugin is not part of netty :)

@normanmaurer

normanmaurer Aug 31, 2017

Member

agree but the os plugin is not part of netty :)

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Sep 4, 2017

Member

Cherry-picked into 4.1 (0fffc84) and 4.0 (1ee2930).

I will ensure we mention it in the release notes.

Member

normanmaurer commented Sep 4, 2017

Cherry-picked into 4.1 (0fffc84) and 4.0 (1ee2930).

I will ensure we mention it in the release notes.

@normanmaurer normanmaurer deleted the load_lib_arch branch Sep 4, 2017

normanmaurer added a commit to netty/netty-tcnative that referenced this pull request Oct 24, 2017

Include architecture in native lib name.
Motivation:

We should include the architecture in the native lib name to allow not load it if the architecture not match.
This is related to netty/netty#7163.

Modifications:

Add architecture to native lib name.

Result:

Fixes [#307].

normanmaurer added a commit to netty/netty-tcnative that referenced this pull request Oct 24, 2017

Include architecture in native lib name.
Motivation:

We should include the architecture in the native lib name to allow not load it if the architecture not match.
This is related to netty/netty#7163.

Modifications:

Add architecture to native lib name.

Result:

Fixes [#307].

normanmaurer added a commit to netty/netty-tcnative that referenced this pull request Oct 24, 2017

Include architecture in native lib name.
Motivation:

We should include the architecture in the native lib name to allow not load it if the architecture not match.
This is related to netty/netty#7163.

Modifications:

Add architecture to native lib name.

Result:

Fixes [#307].

normanmaurer added a commit to netty/netty-tcnative that referenced this pull request Nov 1, 2017

Include architecture in native lib name.
Motivation:

We should include the architecture in the native lib name to allow not load it if the architecture not match.
This is related to netty/netty#7163.

Modifications:

Add architecture to native lib name.

Result:

Fixes [#307].
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment