Use unix as the parent for the Android RID graph #45927

jonathanpeppers · 2020-12-10T23:06:12Z

In a .NET 6 Android project, adding a NuGet package such as:

<PackageReference Include="akavache" Version="6.0.30"/>

...would include Linux native libraries from dependencies, such as:

packages\sqlitepclraw.lib.e_sqlite3.linux\1.1.11\runtimes\linux-arm64\native\libe_sqlite3.so

Comparing the RID graph for iOS and Android in
dotnet\sdk\*\RuntimeIdentifierGraph.json:

"ios": {
  "#import": [
    "unix"
  ]
},
...
"android": {
  "#import": [
    "linux"
  ]
},

It seems like Android graph should match iOS here, as changing linux
-> unix solved this issue for me locally.

Based on the changes in #45442, I updated runtimeGroups.props to
specify linux instead of unix for Android. Then I ran this command
to update the checked-in .json files:

> dotnet build .\src\libraries\pkg\Microsoft.NETCore.Platforms -p:UpdateRuntimeFiles=true

/cc @marek-safar @steveisok

Fixes: dotnet/sdk#14908 In a .NET 6 Android project, adding a NuGet package such as: <PackageReference Include="akavache" Version="6.0.30"/> ...would include Linux native libraries from dependencies, such as: packages\sqlitepclraw.lib.e_sqlite3.linux\1.1.11\runtimes\linux-arm64\native\libe_sqlite3.so Comparing the RID graph for iOS and Android in `dotnet\sdk\*\RuntimeIdentifierGraph.json`: "ios": { "#import": [ "unix" ] }, ... "android": { "#import": [ "linux" ] }, It seems like Android graph should match iOS here, as changing `linux` -> `unix` solved this issue for me locally. Based on the changes in dotnet#45442, I updated `runtimeGroups.props` to specify `linux` instead of `unix` for Android. Then I ran this command to update the checked-in `.json` files: > dotnet build .\src\libraries\pkg\Microsoft.NETCore.Platforms -p:UpdateRuntimeFiles=true

steveisok · 2020-12-11T01:03:41Z

I think there's some pretty good resistance against putting android under unix. See #34789

Perhaps this will achieve the intended result? #43347

marek-safar · 2020-12-11T08:24:57Z

Yep, parenting android to unix was rejected. We need agreement first on how we want to address this problem.

@ericstj what do you suggest as the next steps?

akoeplinger · 2020-12-11T11:37:51Z

Perhaps this will achieve the intended result? #43347

I don't think it will, at least not if we follow the linux-musl precedent which specifies linux-musl -> linux fallback paths, i.e. we'd still get the linux asset for the akavache package if we do the same for linux-bionic.

And if we don't do it then we're back to essentially the same situation with this PR where we're reparenting android to unix.

I think the underlying issue is that there are two competing concerns: allowing to reference managed-only linux RID-specific implementations that e.g. just p/invoke (which work and we want to have) versus pulling in native .so files for linux that definitely won't just work on android.

ericstj · 2020-12-11T17:10:05Z

allowing to reference managed-only linux RID-specific implementations that e.g. just p/invoke (which work and we want to have) versus pulling in native .so files for linux that definitely won't just work on android.

Do packages need to provide a different native asset in order to work on android? If so they can provide that in the android-arm64 folder.

Do packages need to not provide any native asset to work on android? If so they can make that happen by adding an empty android-arm64 folder. Can't change the package? Consumers can add ExcludeAssets="native" on the PackageReference.

There's no promise that a package that specializes for a platform will just work on a brand new platform that they've never considered. RIDs try to do their best to represent useful reuse, so that more often than not folks can avoid duplication when they specialize by platform. They can't get it right 100% of the time, but when its wrong there should be a way to make a package specialize for the new platform once the author understands it needs to. We should only be changing RID hierarchy if there is data out there showing that a majority of the time the RID graph leads to duplication rather than reuse.

jonathanpeppers · 2020-12-11T17:17:16Z

@ericstj I think the problem is if a user brings in an innocent NuGet package in an Android project:

<PackageReference Include="akavache" Version="6.0.30"/>

How do we prevent linux .so files from being added to their app? They will not work on Android.

Right now we have this workaround, which doesn't seem right:

https://github.com/xamarin/xamarin-android/blob/59b18d0cf88c4a5b0f032a23fd2a096f57e8d685/src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets#L1874-L1878

am11 · 2020-12-11T17:23:51Z

The fallback RID is not meant to guarantee binary compatibility for any OS. e.g. there are linux-arm64 and linux-mips64 also parented by linux.. so in case of fallback, the default x64 linux binary will definitely not work there.

Android graph should match iOS here

If we examine the entire RID graph, we will see that it is actually iOS and tvOS that are at odds by not parented by OSX. It does not necessarily mean one approach is right and other one is wrong, it just shows that we are trying to achieve some concrete goals from the thing which does not guarantee them.

ericstj · 2020-12-11T17:27:38Z

How do we prevent linux .so files from being added to their app? They will not work on Android.

Who are you to violate the contract of the package author? The package author decided to include those files you can't just drop them. What's the library going to do when it tries to load them? This is a case of a package that needs to specialize for the new platform, we cannot do that for them.

jonathanpeppers · 2020-12-11T17:41:53Z

The package author decided to include those files you can't just drop them.

In the example above, the app ends up with both Linux and Android .so files and unfortunately have the same file name. The NuGet package includes the MonoAndroid TFM for Xamarin.Android

The library author could update the package to support .NET 6, but it seems like this situation will happen frequently for users migrating from Xamarin.Android to .NET 6. We need to do something, maybe all we can do is give a good error message?

ericstj · 2020-12-11T22:57:49Z

In the example above, the app ends up with both Linux and Android .so files and unfortunately have the same file name

How is this possible? Is more than one package responsible for delivering the same component? Nuget will only ever choose one folder for any asset type.

I tried restoring that package for both net5.0 and monoandroid and I only see libe_sqlite3.so and it only comes from SQLitePCLRaw.lib.e_sqlite3. So I'm not seeing this as anything more than SQLitePCLRaw.lib.e_sqlite3 didn't previously support android and needs an update to do so. Am I missing something?

jonathanpeppers · 2020-12-14T14:23:21Z

I don't know why these packages are split up so much, but I see the Android native library coming from:

https://www.nuget.org/packages/SQLitePCLRaw.lib.e_sqlite3.android/

In this package, the native library is packed as an EmbeddedResource, which is removed and put in the right place in the Android app at build time. This implementation for native libraries was implemented in Xamarin early on. NuGet might not have existed (or hit popularity yet).

Either way, let's say the library author added a net6.0-android TFM with the native library to SQLitePCLRaw.lib.e_sqlite3.android. I think we would still hit the problem here, since the Linux package would continue to be pulled in from SQLitePCLRaw.lib.e_sqlite3?

ericstj · 2020-12-14T17:01:21Z

let's say the library author added a net6.0-android TFM

Native assets must be put in one of the native folders, so they'd need to either use runtimes\android-<arch>\native or runtimes\android-<arch>\nativeassets\net6.0-android; the latter is helpful if you need to scope the applicability to a TFM to constrain where the package applies.

If you placed the native SO binary in lib\net6.0-android it would be passed to the C# compiler, it wouldn't be considered by the dotnet host for PInvokes (if that's a concern) and there is no way to encode the architecture (if that is a concern).

If the author decided to provide the native binary in a different package, SQLitePCLRaw.lib.e_sqlite3.android then they could also update SQLitePCLRaw.lib.e_sqlite3 to add a placeholder at runtimes\android\native\_._ so that the SQLitePCLRaw.lib.e_sqlite3 doesn't apply any assets to the android RID. Same would be true if you decided to split up your TFM assets in multiple packages.

jonathanpeppers · 2020-12-14T17:57:12Z

Sorry, I mistyped, I should have put the RID android-arm, etc. instead of net6.0-android

What should we do here? Emit a nice error message if linux-**\*.so files are brought in?

jonathanpeppers · 2020-12-14T17:59:23Z

I think you would currently hit:

Native library 'packages\sqlitepclraw.lib.e_sqlite3.linux\1.1.11\runtimes\linux-arm64\native\libe_sqlite3.so' will not be bundled because it has an unsupported ABI. Move this file to a directory with a valid Android ABI name such as 'libs/armeabi-v7a/'.

https://github.com/xamarin/xamarin-android/blob/97bbdd4a46fb8c2c882e610d6a45254267ef23b1/src/Xamarin.Android.Build.Tasks/Properties/Resources.resx#L650

ericstj · 2020-12-14T19:48:00Z

Will the check be based off of scanning the binary? If so then that seems fine. If the check is based off checking the folder name it seems like it could have false positives.

I've noticed the `_IncludeNativeSystemLibraries` MSBuild target can take 40ms because it is running this workaround:  <FrameworkNativeLibrary Remove="@(ResolvedFileToPublish)" Condition=" $([System.Text.RegularExpressions.Regex]::IsMatch('%(FullPath)', '.*linux-.*')) " /> The `_IncludeNativeSystemLibraries` also can be split up between "legacy" Xamarin.Android and .NET 6. The .NET 6 version can simply check the `@(ResolvedFileToPublish)` item group, as the other parts support the intepreter, profiler, etc., which are not available in .NET 6 yet. I created a new `<ProcessNativeLibraries/>` MSBuild task to simplify the work in C#: * We can iterate over all `.so` files from `@(ResolvedFileToPublish)`. * We can fix up `libmonodroid.so` as needed. * We can emit a warning if the ABI is unknown without using a `Regex`. It is possible for Linux `.so` files to be brought in from NuGet packages as discussed in: dotnet/runtime#45927 The warning is a new alternate message for XA4301, such as: Cannot determine ABI of native library 'C:\src\xamarin-android\packages\sqlitepclraw.lib.e_sqlite3.linux\1.1.11\runtimes\linux-arm64\native\libe_sqlite3.so'. The RuntimeIdentifier 'linux-arm64' does not target Android platforms. As part of this change, .NET 6 will no longer need to do: <PropertyGroup> <_Assemblies>@(_ResolvedFrameworkAssemblies)</_Assemblies> So we can avoid creating a ~14K character string. ~~ Results ~~ Timing the project fromm the `DotNetBuild(android.21-arm,false)` test, in a build with no changes: Before: 40 ms _IncludeNativeSystemLibraries 1 calls After: 4 ms _IncludeNativeSystemLibraries 1 calls This should save ~36ms from all .NET 6 builds.

jonathanpeppers · 2020-12-15T14:56:17Z

Closing this in favor of the latest changes in dotnet/android#5386

I added a new warning message that reports if %(RuntimeIdentifier) is not an Android platform and excludes the library.

I've noticed the `_IncludeNativeSystemLibraries` MSBuild target can take 40ms because it is running this workaround:  <FrameworkNativeLibrary Remove="@(ResolvedFileToPublish)" Condition=" $([System.Text.RegularExpressions.Regex]::IsMatch('%(FullPath)', '.*linux-.*')) " /> The `_IncludeNativeSystemLibraries` also can be split up between "legacy" Xamarin.Android and .NET 6. The .NET 6 version can simply check the `@(ResolvedFileToPublish)` item group, as the other parts support the intepreter, profiler, etc., which are not available in .NET 6 yet. I created a new `<ProcessNativeLibraries/>` MSBuild task to simplify the work in C#: * We can iterate over all `.so` files from `@(ResolvedFileToPublish)`. * We can fix up `libmonodroid.so` as needed. * We can emit a warning if the ABI is unknown without using a `Regex`. It is possible for Linux `.so` files to be brought in from NuGet packages as discussed in: dotnet/runtime#45927 The warning is a new alternate message for XA4301, such as: warning XA4301: Cannot determine ABI of native library 'C:\src\xamarin-android\packages\sqlitepclraw.lib.e_sqlite3.linux\1.1.11\runtimes\linux-arm64\native\libe_sqlite3.so'. Remove the 'SQLitePCLRaw.lib.e_sqlite3.linux' NuGet package, or notify the library author. As part of this change, .NET 6 will no longer need to do: <PropertyGroup> <_Assemblies>@(_ResolvedFrameworkAssemblies)</_Assemblies> So we can avoid creating a ~14K character string. ~~ Results ~~ Timing the project fromm the `DotNetBuild(android.21-arm,false)` test, in a build with no changes: Before: 40 ms _IncludeNativeSystemLibraries 1 calls After: 4 ms _IncludeNativeSystemLibraries 1 calls This should save ~36ms from all .NET 6 builds.

I've noticed that the `_IncludeNativeSystemLibraries` MSBuild target can take 40ms because it is running this workaround:  <FrameworkNativeLibrary Remove="@(ResolvedFileToPublish)" Condition=" $([System.Text.RegularExpressions.Regex]::IsMatch('%(FullPath)', '.*linux-.*')) " /> The `_IncludeNativeSystemLibraries` also can be split up between "legacy" Xamarin.Android and .NET 6. The .NET 6 version can simply check the `@(ResolvedFileToPublish)` item group, as the other parts support the interpreter, profiler, etc., which are not available in .NET 6 yet. I created a new `<ProcessNativeLibraries/>` MSBuild task to simplify the work in C#: * We can iterate over all `.so` files in `@(ResolvedFileToPublish)`. * We can fix up `libmonodroid.so` as needed. * We can emit a warning if the ABI is unknown without using a `Regex`. It is possible for Linux `.so` files to be brought in from NuGet packages as discussed in: dotnet/runtime#45927 The warning is a new alternate message for XA4301, such as: warning XA4301: Cannot determine ABI of native library 'C:\src\xamarin-android\packages\sqlitepclraw.lib.e_sqlite3.linux\1.1.11\runtimes\linux-arm64\native\libe_sqlite3.so'. Remove the 'SQLitePCLRaw.lib.e_sqlite3.linux' NuGet package, or notify the library author. As part of this change, .NET 6 will no longer need to do: <PropertyGroup> <_Assemblies>@(_ResolvedFrameworkAssemblies)</_Assemblies> So we can also avoid creating a ~14K character string. ~~ Results ~~ Timing the project fromm the `DotNetBuild(android.21-arm,false)` test, in a build with no changes: * Before: 40 ms _IncludeNativeSystemLibraries 1 calls * After: 4 ms _IncludeNativeSystemLibraries 1 calls This should save ~36ms from all .NET 6 builds.

Dotnet-GitSync-Bot added the area-Meta label Dec 10, 2020

marek-safar added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Dec 11, 2020

jonathanpeppers mentioned this pull request Dec 15, 2020

[One .NET] simplify native library inclusion dotnet/android#5386

Merged

jonathanpeppers closed this Dec 15, 2020

ghost locked as resolved and limited conversation to collaborators Jan 14, 2021

Use unix as the parent for the Android RID graph #45927

Use unix as the parent for the Android RID graph #45927

Uh oh!

Conversation

jonathanpeppers commented Dec 10, 2020

Uh oh!

steveisok commented Dec 11, 2020

Uh oh!

marek-safar commented Dec 11, 2020

Uh oh!

akoeplinger commented Dec 11, 2020

Uh oh!

ericstj commented Dec 11, 2020

Uh oh!

jonathanpeppers commented Dec 11, 2020

Uh oh!

am11 commented Dec 11, 2020

Uh oh!

ericstj commented Dec 11, 2020

Uh oh!

jonathanpeppers commented Dec 11, 2020

Uh oh!

ericstj commented Dec 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonathanpeppers commented Dec 14, 2020

Uh oh!

ericstj commented Dec 14, 2020

Uh oh!

jonathanpeppers commented Dec 14, 2020

Uh oh!

jonathanpeppers commented Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericstj commented Dec 14, 2020

Uh oh!

jonathanpeppers commented Dec 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ericstj commented Dec 11, 2020 •

edited

Loading

jonathanpeppers commented Dec 14, 2020 •

edited

Loading