-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Use lib prefix in all native library names #33236
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| <Project> | ||
| <!-- Add path globs specific to native binaries to exclude unnecessary files from packages. --> | ||
| <Choose> | ||
| <When Condition=" '$(_runtimeOSFamily)' == 'win' OR $(RuntimeIdentifier.StartsWith('win')) "> | ||
| <PropertyGroup> | ||
| <ApplicationFileExtension>.exe</ApplicationFileExtension> | ||
| <LibraryFilePrefix></LibraryFilePrefix> | ||
| <LibraryFileExtension>.dll</LibraryFileExtension> | ||
| <SymbolFileExtension>.pdb</SymbolFileExtension> | ||
| </PropertyGroup> | ||
| </When> | ||
| <When Condition=" '$(_runtimeOSFamily)' == 'osx' OR $(RuntimeIdentifier.StartsWith('osx')) "> | ||
| <PropertyGroup> | ||
| <LibraryFilePrefix Condition=" '$(SkipLibraryPrefixFromUnix)' == '' ">lib</LibraryFilePrefix> | ||
| <LibraryFileExtension>.dylib</LibraryFileExtension> | ||
| <SymbolFileExtension>.dwarf</SymbolFileExtension> | ||
| </PropertyGroup> | ||
| </When> | ||
| <When Condition=" '$(_runtimeOSFamily)' == 'android' "> | ||
| <PropertyGroup> | ||
| <LibraryFilePrefix Condition=" '$(SkipLibraryPrefixFromUnix)' == '' ">lib</LibraryFilePrefix> | ||
| <LibraryFileExtension>.so</LibraryFileExtension> | ||
| <!--symbols included in .so, like Linux, but can be generated externally and if so, uses .debug ext--> | ||
| <SymbolFileExtension>.debug</SymbolFileExtension> | ||
| </PropertyGroup> | ||
| </When> | ||
| <Otherwise> | ||
| <PropertyGroup> | ||
| <LibraryFilePrefix Condition=" '$(SkipLibraryPrefixFromUnix)' == '' ">lib</LibraryFilePrefix> | ||
| <LibraryFileExtension>.so</LibraryFileExtension> | ||
| <SymbolFileExtension>.dbg</SymbolFileExtension> | ||
| </PropertyGroup> | ||
| </Otherwise> | ||
| </Choose> | ||
|
|
||
| <ItemGroup> | ||
| <AdditionalLibPackageExcludes Condition="'$(SymbolFileExtension)' != ''" Include="%2A%2A\%2A$(SymbolFileExtension)" /> | ||
| <AdditionalSymbolPackageExcludes Condition="'$(LibraryFileExtension)' != ''" Include="%2A%2A\%2A.a;%2A%2A\%2A$(LibraryFileExtension)" /> | ||
| </ItemGroup> | ||
|
|
||
| </Project> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,12 +7,12 @@ internal static partial class Interop | |
| internal static partial class Libraries | ||
| { | ||
| // Shims | ||
| internal const string SystemNative = "System.Native"; | ||
|
akoeplinger marked this conversation as resolved.
|
||
| internal const string GlobalizationNative = "System.Globalization.Native"; | ||
| internal const string NetSecurityNative = "System.Net.Security.Native"; | ||
| 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"; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. plus adding the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense |
||
| internal const string GlobalizationNative = "libSystem.Globalization.Native"; | ||
| internal const string NetSecurityNative = "libSystem.Net.Security.Native"; | ||
| internal const string CryptoNative = "libSystem.Security.Cryptography.Native.OpenSsl"; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CC @akoeplinger and @marek-safar to be aware of the suggestion.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just curious - why it does not work for you?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||
| internal const string CompressionNative = "libSystem.IO.Compression.Native"; | ||
| internal const string IOPortsNative = "libSystem.IO.Ports.Native"; | ||
| internal const string Libdl = "libdl"; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,11 +32,6 @@ add_library(System.Security.Cryptography.Native.Apple-Static | |
| ${NATIVECRYPTO_SOURCES} | ||
| ) | ||
|
|
||
| # Disable the "lib" prefix. | ||
| 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 "") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| set_target_properties(System.Security.Cryptography.Native.Apple-Static PROPERTIES OUTPUT_NAME System.Security.Cryptography.Native.Apple CLEAN_DIRECT_OUTPUT 1) | ||
|
|
||
| target_link_libraries(System.Security.Cryptography.Native.Apple | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,11 +75,6 @@ add_library(System.Security.Cryptography.Native.OpenSsl-Static | |
| $<TARGET_OBJECTS:objlib> | ||
| ) | ||
|
|
||
| # Disable the "lib" prefix. | ||
| 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 "") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes this change is correct. We could however preserve the |
||
| set_target_properties(System.Security.Cryptography.Native.OpenSsl-Static PROPERTIES OUTPUT_NAME System.Security.Cryptography.Native.OpenSsl CLEAN_DIRECT_OUTPUT 1) | ||
|
|
||
| if (FEATURE_DISTRO_AGNOSTIC_SSL) | ||
|
|
||
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) 😅