Consuming Microsoft.NETCore.Native package, which brings in libSystem… #2592
Conversation
….Globalization.Native.a Replacing the dummy globalization implementation for unix with the actual one
| else | ||
| { | ||
|
|
||
| // lifted from https://github.com/dotnet/coreclr/blob/master/src/vm/comutilnative.cpp#L3009-L3032 |
There was a problem hiding this comment.
Nit: A lot of code in CoreRT CoreLib was lifted from somewhere in CoreCLR. We do not include the links to where it was lifted from. (Also, the link you have added will become bogus pretty quickly as the original file will change.)
| <Compile Include="System\Runtime\InteropServices\Marshal.cs" /> | ||
| <Compile Include="System\Runtime\InteropServices\MarshalAdapter.cs" /> | ||
| <Compile Include="System\Runtime\InteropServices\MarshalImpl.cs" /> | ||
| <Compile Include="System\Runtime\InteropServices\MarshalAsAttribute.cs" /> |
There was a problem hiding this comment.
Could you please add forwarders for these from System.Private.Interop to System.Private.CoreLib so that the existing ProjectN corefx drop does not break?
|
@shrah @yizhang82 Do you see any issues with the moved interop types? |
|
@tarekgh Could you please take a look at the globalization changes as well? |
|
LGTM otherwise |
| return IndexOfOrdinal(source, target, startIndex, count, ignoreCase: false); | ||
| } | ||
|
|
||
| if (_isAsciiEqualityOrdinal && CanUseAsciiOrdinalForOptions(options) && source.IsFastSort() && target.IsFastSort()) |
There was a problem hiding this comment.
why removed this code? my understanding this can still work.
There was a problem hiding this comment.
I guess we should keep the exact code we have in coreclr under \mscorlib\corefx\System\Globalization. right?
There was a problem hiding this comment.
do we have limitation in corert for keeping such code?
There was a problem hiding this comment.
The IsFastSort, etc. bits are not cached in object pre-header in CoreRT. I agree we should keep the CoreCLR specific code under #if CORECLR so that we can share the sources.
|
The OSX failures seem to be due to some missing libraries (libICU?). We might need to add them to Is this the fix for issue #2184? |
|
other than my comment, LGTM |
| </ItemGroup> | ||
| <!-- Dummy globalization implementation --> | ||
| <!-- Dummy globalization implementation | ||
| <ItemGroup Condition="'$(TargetsUnix)'=='true'"> |
There was a problem hiding this comment.
Could you please change the Condition to be false instead of commenting out the whole block?
It looks like that. the missing symbols are from ICU. however they are being referenced from libSystem.Globalization.Native.a. so I'm not sure about the right fix, static linking ICU to the globalization pal or app.exe
Yes!. It does fix #2184. Thanks |
Dynamically link against icucore. Same as what we are doing in the standalone shim: |
| <ItemGroup> | ||
| <AdditionalNativeLibrary Include="$(IlcPath)/sdk/libSystem.Private.CoreLib.Native.a" /> | ||
| <AdditionalNativeLibrary Include="$(IlcPath)/framework/System.Native.a" /> | ||
| <AdditionalNativeLibrary Include="$(IlcPath)/framework/libSystem.Globalization.Native.a" /> |
There was a problem hiding this comment.
Why do we have this inconsistency in library naming? We have System.Native.a, but libSystem.Globalization.Native.a. On CoreCLR, none of the shared libraries that are named with System.XXXX have the lib prefix, so it seems strange to have it for some of the static ones.
There was a problem hiding this comment.
I agree we should fix the names to be consistent.
There was a problem hiding this comment.
isn't libXXX more adopted naming convention in the industry? Why did we name those libs as such on CoreCLR in the first place? Shall we fix libSystem.Globalization.Native.a or System.Native.a ?
There was a problem hiding this comment.
We have made this choice a long time ago so that the shared library name matches the corresponding managed assembly. You can see the quite long discussion here: dotnet/coreclr#745
There was a problem hiding this comment.
Thanks for the pointer @janvorli. I'll update libSystem.Globalization.Native.a name
|
I've updated the PR according to the feedback. The only change that I didn't include is renaming System.Globalization.Native.a, which requires a change on CoreCLR and a follow-up on CoreRT. I'll handle that separately. |
|
@dotnet-bot test Windows_NT Release please |
7 participants
….Globalization.Native.a
Replacing the dummy globalization implementation for unix with the actual one
@jkotas, PTAL,
/cc @dotnet/corert-contrib