dotnet / sdk

Core functionality needed to create .NET Core projects, that is shared between Visual Studio and CLI
https://dot.net/core
MIT License
2.72k stars 1.07k forks source link

Question about supporting runtime packs for "parent" RIDs #24077

Open pjcollins opened 2 years ago

pjcollins commented 2 years ago

The Android (32) workload currently ships four runtime packs, one for each Android architecture:

In order to reduce our installation footprint we have a desire to introduce a new runtime pack for managed assemblies that are not architecture specific, but are still only intended to be used with the parent android RID. Right now these managed assemblies are duplicated in all four packs mentioned above. Ideally, we would move the non-architecture specific assemblies into a new pack:

I tried to get this to work by adding a second RuntimePackNamePatterns value to the KnownFrameworkReference we declare:

<KnownFrameworkReference
    Include="Microsoft.Android"
    TargetFramework="net6.0"
    RuntimeFrameworkName="Microsoft.Android"
    LatestRuntimeFrameworkVersion="**FromWorkload**"
    TargetingPackName="Microsoft.Android.Ref.32"
    TargetingPackVersion="**FromWorkload**"
-   RuntimePackNamePatterns="Microsoft.Android.Runtime.32.**RID**"
+   RuntimePackNamePatterns="Microsoft.Android.Runtime.32.android;Microsoft.Android.Runtime.32.**RID**"
    RuntimePackRuntimeIdentifiers="android-arm;android-arm64;android-x86;android-x64"
    Profile="Android"
/>

The ProcessFrameworkReferences task handles this approach, but the ResolveFrameworkReferences task breaks as it only allows for a 1:1 relationship between runtime pack and framework reference:

Microsoft.NET.Sdk.FrameworkReferenceResolution.targets(326,5): error MSB4018: System.ArgumentException: An item with the same key has already been added. Key: Microsoft.Android

I'm wondering how risky or big of a breaking change it would be to change the RuntimePack* metadata on ResolvedFrameworkReferences, and do something like this instead:

--- a/src/Tasks/Microsoft.NET.Build.Tasks/ResolveFrameworkReferences.cs
+++ b/src/Tasks/Microsoft.NET.Build.Tasks/ResolveFrameworkReferences.cs
@@ -26,7 +26,6 @@ protected override void ExecuteCore()
             }

             var resolvedTargetingPacks = ResolvedTargetingPacks.ToDictionary(tp => tp.ItemSpec, StringComparer.OrdinalIgnoreCase);
-            var resolvedRuntimePacks = ResolvedRuntimePacks.ToDictionary(rp => rp.GetMetadata(MetadataKeys.FrameworkName), StringComparer.OrdinalIgnoreCase);

             var resolvedFrameworkReferences = new List<TaskItem>(FrameworkReferences.Length);

@@ -48,12 +47,11 @@ protected override void ExecuteCore()
                 resolvedFrameworkReference.SetMetadata("TargetingPackVersion", targetingPack.GetMetadata(MetadataKeys.NuGetPackageVersion));
                 resolvedFrameworkReference.SetMetadata("Profile", targetingPack.GetMetadata("Profile"));

-                ITaskItem runtimePack;
-                if (resolvedRuntimePacks.TryGetValue(frameworkReference.ItemSpec, out runtimePack))
-                {
-                    resolvedFrameworkReference.SetMetadata("RuntimePackPath", runtimePack.GetMetadata(MetadataKeys.PackageDirectory));
-                    resolvedFrameworkReference.SetMetadata("RuntimePackName", runtimePack.GetMetadata(MetadataKeys.NuGetPackageId));
-                    resolvedFrameworkReference.SetMetadata("RuntimePackVersion", runtimePack.GetMetadata(MetadataKeys.NuGetPackageVersion));
+                var matchingPacks = ResolvedRuntimePacks.Where(rp => rp.GetMetadata(MetadataKeys.FrameworkName).Equals(frameworkReference.ItemSpec, StringComparison.OrdinalIgnoreCase));
+                if (matchingPacks.Any()) {
+                    resolvedFrameworkReference.SetMetadata("RuntimePackPath", string.Join (";", matchingPacks.Select (mp => mp.GetMetadata(MetadataKeys.PackageDirectory))));
+                    resolvedFrameworkReference.SetMetadata("RuntimePackName", string.Join (";", matchingPacks.Select (mp => mp.GetMetadata(MetadataKeys.NuGetPackageId))));
+                    resolvedFrameworkReference.SetMetadata("RuntimePackVersion", string.Join (";", matchingPacks.Select (mp => mp.GetMetadata(MetadataKeys.NuGetPackageVersion))));
                 }

                 resolvedFrameworkReferences.Add(resolvedFrameworkReference);

We may also want to update the metadata names, but that might cause more breakage. These changes would produce ; separated RuntimePack* metadata values, for example:

RuntimePackName = Microsoft.Android.Runtime.32.android;Microsoft.Android.Runtime.32.android-x64

These changes to ResolveFrameworkReferences appear to work as needed for the Android use case. I've also got something working on the workload side by introducing a new FrameworkReference for Android, but I am not convinced that this FrameworkReference addition is the most sane or desirable approach.

Maybe there are other approaches that would support a "generic RID" runtime pack that I am not considering?

dotnet-issue-labeler[bot] commented 2 years ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

pjcollins commented 2 years ago

Adding @dsplaisted and @jonathanpeppers to help weigh in on this, I think it would be something we may want to do for .NET 7.

dsplaisted commented 2 years ago

This makes sense to me, and I think the multiple runtime pack approach per FrameworkReference approach is preferrable to the multiple FrameworkReference approach.

baronfel commented 2 years ago

We'd love to see this for 7.0! Of course we'd accept a PR, but otherwise the team will work it in as time/scheduling allows.

jonathanpeppers commented 2 years ago

@pjcollins it might work without any code changes if we did:

<KnownFrameworkReference
    Include="Microsoft.Android"
    TargetFramework="net6.0"
    RuntimeFrameworkName="Microsoft.Android"
    LatestRuntimeFrameworkVersion="**FromWorkload**"
    TargetingPackName="Microsoft.Android.Ref.32"
    TargetingPackVersion="**FromWorkload**"
    RuntimePackNamePatterns="Microsoft.Android.Runtime.32.**RID**"
    RuntimePackRuntimeIdentifiers="android"
    Profile="Android"
/>

Then put native libraries for every RID inside? Release apps use all 4 architectures by default anyway -- don't need them split into separate packs?

This is similar to what Maui does as there are no native libraries:

https://github.com/dotnet/maui/blob/4a0f92458da62006b347f5eb7e29e06e8d7b5f97/src/Workload/Microsoft.Maui.Sdk/Sdk/BundledVersions.in.targets#L18-L31

%(RuntimePackRuntimeIdentifiers) is just android for this one.

pjcollins commented 2 years ago

Thanks that is a good point given how small our architecture specific libraries are, I will try to play around with that option as well. I can probably open a PR against ResolveFrameworkReferences at some point next week if it ends up still being needed.

pjcollins commented 2 years ago

I spent some time seeing if we could fit everything into a single runtime pack:

31.0.200-ci.runtime-dedupe-prototype.107
│
├───data
│       RuntimeList.xml
│
└───runtimes
    ├───android
    │   └───lib
    │       └───net6.0
    │               Java.Interop.dll
    │               Java.Interop.pdb
    │               Mono.Android.dll
    │               Mono.Android.Export.dll
    │               Mono.Android.Export.pdb
    │               Mono.Android.pdb
    │
    ├───android-arm
    │   └───native
    │           libmono-android.debug.so
    │           libmono-android.release.so
    │           libxamarin-debug-app-helper.so
    │
    ├───android-arm64
    │   └───native
    │           libmono-android.debug.so
    │           libmono-android.release.so
    │           libxamarin-debug-app-helper.so
    │
    ├───android-x64
    │   └───native
    │           libmono-android.debug.so
    │           libmono-android.release.so
    │           libxamarin-debug-app-helper.so
    │
    └───android-x86
        └───native
                libmono-android.debug.so
                libmono-android.release.so
                libxamarin-debug-app-helper.so

With this approach I've ran into an issue with the ResolveRuntimePackAssets task, which generally doesn't support nested files with the same name:

"C:\Users\Peter\test\android\android.csproj" (_ComputeFilesToPublishForRuntimeIdentifiers target) (1:11) ->
    C:\Users\Peter\android-toolchain\dotnet\sdk\6.0.300-preview.22128.2\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.FrameworkReferenceResolution.targets(427,5): error NETSDK1110: More than one asset in the runtime pack has the same destination sub-path of 'libmono-android.debug.so'. Report this error to the .NET team here: https://aka.ms/dotnet-sdk-issue.

To move forward with the single runtime pack suggestion we could try to update this task to handle this case, or alternatively rename the architecture specific components so that they are unique and add custom processing tasks/targets to our Android SDK.

I think I am still leaning towards the approach of adding a new runtime pack for managed assemblies and adding support for multiple runtime packs per framework reference.

marcpopMSFT commented 1 year ago

@pjcollins is this issue still something needing investigation?

jonathanpeppers commented 1 year ago

@marcpopMSFT if we could solve this one (maybe .NET 9?), it would help us cut down the on-disk size of the Android workload by about 35MB x 3 = 105MB.

The iOS & Apple workloads might have similar savings, too.

pjcollins commented 1 year ago

As @jonathanpeppers mentioned we do still have scenarios where we would benefit from being able to use both architecture specific and non architecture specific (but still platform specific) runtime packs as part of our workload.

jeromelaban commented 1 year ago

As there's a bit of discussion around workload sizes, though I'm not sure if this is the appropriate issue to discuss this, I've been wondering if having library-only (reference maybe?) workloads would be possible?

It would save a lot build time for package authors, as we're still around 3 minutes of install time during each build.

jonathanpeppers commented 1 year ago

I think there is a tradeoff here between what the average user wants installed (everything) and what a CI build might want. We could create lots of tiny workloads, but then dotnet workload search starts to be a long list.

With the android workload as an example, you could remove these lines and probably still build class libraries?

https://github.com/xamarin/xamarin-android/blob/554eb9e6dd9944c68707fbe216aa91f2df14fc8a/src/Xamarin.Android.Build.Tasks/Microsoft.NET.Sdk.Android/WorkloadManifest.in.json#L17-L22

Maybe you could give that a test during your build, remove this extends json array? If everything still works, that might be 50% of the install time.

jeromelaban commented 1 year ago

Thanks for the hints, I will try that!