-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Use lib prefix in all native library names #33236
Conversation
IIRC, the DllImport loader will probe for the specified name + suffix (e.g. ".so") before it probes for the "lib" prefix + name + suffix. Given that, we might want to also update https://github.com/dotnet/runtime/blob/master/src/libraries/Common/src/Interop/Unix/Interop.Libraries.cs to include the "lib" prefix in each name, to skip one of the probing steps. |
cc @tarekgh |
Would the cost of |
Probing requires disk access. So, no :) But I'm also not sure why IsOSPlatform is relevant to this issue? The runtime is built for the target platform/OS; the ".dylib" vs ".so" is hardcoded. |
cc @tarekgh since he was on the issue |
internal const string CryptoNative = "System.Security.Cryptography.Native.OpenSsl"; | ||
internal const string CompressionNative = "System.IO.Compression.Native"; | ||
internal const string IOPortsNative = "System.IO.Ports.Native"; | ||
internal const string SystemNative = "libSystem.Native"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would lib.System.Native
(and so forth) be better? more readable than libSystem.Native
perhaps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, this is not the Linux convention. usually `lib' is prefixed without dots.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plus adding the .
might make it such that the loader won't find the library automatically as for anybody PInvoking to System.Native, I believe the runtime will try either System.Native or preappending lib, but it won't find it if it also needs to pre-append a dot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense
Thanks @danmosemsft for tagging me. @jkotas already tagged me before too. |
I'll take a look at the failures on the builds to see what is wrong |
@joperezr, thank you. I am investigating macOS build failure locally, if you could point out issue with Windows failure, that would help a lot. :) |
The command we are sending to crossgen seem fine to me though:
|
set_target_properties(System.Security.Cryptography.Native.OpenSsl PROPERTIES PREFIX "") | ||
|
||
# Disable the "lib" prefix and override default name | ||
set_target_properties(System.Security.Cryptography.Native.OpenSsl-Static PROPERTIES PREFIX "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not too familiar with this so probably @bartonjs should take a look, but looks like in here you are also changing the override of the default name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and override default name
this part of the comment refers to the line below. There are two variants of this library produced by this script: shared and static. We have prefix override for each which this delta is removing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this change is correct. We could however preserve the override default name
part of the comment for the next line which sets the OUTPUT_NAME on the static lib.
set_target_properties(System.Security.Cryptography.Native.Apple PROPERTIES PREFIX "") | ||
|
||
# Disable the "lib" prefix and override default name | ||
set_target_properties(System.Security.Cryptography.Native.Apple-Static PROPERTIES PREFIX "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here, this is not only adding lib as a prefix, it is removing our override
this is the issue on macOS, @joperezr. crossgen's response file is picking up $ otool -L ./artifacts/bin/coreclr/OSX.x64.Debug/sharedFramework/libclrjit.dylib
./artifacts/bin/coreclr/OSX.x64.Debug/sharedFramework/libclrjit.dylib:
@rpath/libclrjit.dylib (compatibility version 0.0.0, current version 0.0.0)
/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation (compatibility version 150.0.0, current version 1673.126.0)
/System/Library/Frameworks/CoreServices.framework/Versions/A/CoreServices (compatibility version 1.0.0, current version 1069.11.0)
/System/Library/Frameworks/Security.framework/Versions/A/Security (compatibility version 1.0.0, current version 59306.41.2)
/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1281.0.0)
/usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 800.7.0) Interestingly, the error message doesn't show the actual filename that was requested (and failed) to load: |
internal const string SystemNative = "libSystem.Native"; | ||
internal const string GlobalizationNative = "libSystem.Globalization.Native"; | ||
internal const string NetSecurityNative = "libSystem.Net.Security.Native"; | ||
internal const string CryptoNative = "libSystem.Security.Cryptography.Native.OpenSsl"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change, overall, makes me sad. We intentionally stripped off the lib prefix as part of "these aren't general purpose libraries, don't look here". But if we need to, we need to, I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alternative may be to statically link these libraries into the runtime for Android where it is causing problems. We are going to have this option for single file flavor of CoreCLR anyway, so doing it for Android as well (or even everywhere) should not be a big deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CC @akoeplinger and @marek-safar to be aware of the suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
statically link these libraries into the runtime
Xamarin.Android does not support this mode. I think this would be nice but we won't be in a position to deliver this for .net5. This looks more like .net6 enhancement because we would need to do native linking during publish phase (this depends on Android NDK, etc).
/cc @jonpryor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For CoreCLR, the plan is to statically link the libraries as part of the dotnet/runtime build (see #32823). We are not planning to do native linking during publish phase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That won't work for us. The thinking right now is to copy the dynamic libs which are used (during publish) and extend that to full static linking for Android as we plan to do for iOS (and wasm) during publish
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious - why it does not work for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Primary because of size. If an app uses no crypto we should not link libSystem.Net.Security.Native
and you can apply that to all native libs where System.IO.Compression.Native
is probably the biggest one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The moment someone brings in any managed HTTP implementation, client or server, it's going to end up requiring both of those. Presumably then the size concern would only be if they were using a platform-specific implementation (e.g. AndroidHttpHandler) where the platform provided all of that support?
Also consolidated related MSBuild properties in `/eng/native/naming.props` file.
@@ -12,6 +12,9 @@ | |||
<!-- build the transport package which includes product and symbols in addition to standard packages --> | |||
<CreatePackedPackage Condition="'$(CreatePackedPackage)' == ''">true</CreatePackedPackage> | |||
|
|||
<!-- indicates that lib prefix is not added to library names for Unix-like operating systems --> | |||
<SkipLibraryPrefixFromUnix>true</SkipLibraryPrefixFromUnix> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the reason for this property? why would we want to skip the prefix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case of coreclr, the prefixes were not added in the original version. Apparently, there is a job on CI that crosscompile clrjit.so (Linux version) on Windows. That one was failing, so I restored it by skipping the prefix here. It will require a bit more investigation and it can be avoided (at least in theory) 😅
all reviewers, do you have any objection proceeding with this PR? any more feedback? For the issue linking the native library statically, we can track it as a different work item. |
CC @janvorli |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM too
I have created the issue #33311 to track the static linking of the native libraries and I am going to merge this one. |
Thanks @am11 for your help with this issue. |
Also consolidated related MSBuild properties in
/eng/native/naming.props
file.Fixes #33118