[question] supporting multiple $(RuntimeIdentifiers) in Xamarin

[jonathanpeppers]

This might also need changes in dotnet/runtime, but I'll file here since it is mostly build related.

In dotnet/android#4767, I have an implementation that would enable:

<RuntimeIdentifiers>android.21-arm;android.21-arm64;android.21-x86;android.21-x64</RuntimeIdentifiers>

And you could build the project via:

> dotnet build HelloAndroid.csproj -p:Configuration=Release

This produces an Android HelloAndroid.apk file with contents such as:

assemblies\HelloAndroid.dll
assemblies\armeabi-v7a\System.Private.CoreLib.dll
assemblies\arm64-v8a\System.Private.CoreLib.dll
assemblies\x86\System.Private.CoreLib.dll
assemblies\x86_64\System.Private.CoreLib.dll
lib\armeabi-v7a\libmonosgen-2.0.so
lib\arm64-v8a\libmonosgen-2.0.so
lib\x86\libmonosgen-2.0.so
lib\x86_64\libmonosgen-2.0.so

You can install and run this .apk file on Android devices/emulators of any of the 4 architectures.

What I actually had to do to make this work, brings up several questions.

Is the mechanism I'm using to process multiple $(RuntimeIdentifier) the right idea?

At a high level, after Build we do something like:

<ItemGroup>
  <_RIDs Include="$(RuntimeIdentifiers)" />
</ItemGroup>
<MSBuild
    Projects="$(MSBuildProjectFile)"
    Targets="_ComputeFilesToPublishForRuntimeIdentifiers"
    Properties="RuntimeIdentifier=%(_RIDs.Identity)">
  <Output TaskParameter="TargetOutputs" ItemName="ResolvedFileToPublish" />
</MSBuild>

Against a new target we define:

<Target Name="_ComputeFilesToPublishForRuntimeIdentifiers"
    DependsOnTargets="ResolveReferences;ComputeFilesToPublish"
    Returns="@(ResolvedFileToPublish)">
</Target>

So in the example above with 4 architectures:

1. Build occurs. Microsoft.Android.Sdk defaults $(AppendRuntimeIdentifierToOutputPath) to false. We only need a single HelloAndroid.dll and HelloAndroid.apk file.
2. The <MSBuild/> task call runs for each RID.
3. ILLink will run 4 times (once per RID). We get linker output into 4 distinct directories such as $(IntermediateOutputPath)$(RuntimeIdentifier)\linked\.
4. --deterministic is passed to ILLink, so almost all of the BCL assemblies are duplicate across architectures.
5. We have an MSBuild task that uses System.Reflection.Metadata to read the mvid of all assemblies. Any duplicates are removed, architecture-specific assemblies go in a sub-directory as described above.

Is this the right idea, in general? Is there a change in dotnet/sdk that could simplify this?

If we should deduplicate .NET assemblies, should the assemblies be identical across all runtime packs?

In a Debug build (or ILLink disabled), the deduplication does not work. The BCL assemblies in the runtime pack for each architecture are different. This results in a ~130MB .apk file.

But if I compare them:

$ monodis packages/microsoft.netcore.app.runtime.android-arm/5.0.0-preview.6.20264.1/runtimes/android-arm/lib/net5.0/System.IO.dll --module
Module Table (1..1)
1: System.IO.dll 1 {1D745C86-CC08-41A8-ABA5-F221F7BAD20D}
$ monodis packages/microsoft.netcore.app.runtime.android-arm64/5.0.0-preview.6.20264.1/runtimes/android-arm64/lib/net5.0/System.IO.dll --module
Module Table (1..1)
1: System.IO.dll 1 {B582D0AC-1AE7-4E41-8DF0-6938725B4ADB}

It looks like only the mvid differs, and none of the IL differs. Is this intentional or are these packages built with /p:Deterministic=false?

If a fix is needed for the runtime packs, I think this would be in dotnet/runtime.

/cc @dsplaisted @marek-safar @steveisok

[dsplaisted]

1. Build occurs. Microsoft.Android.Sdk defaults $(AppendRuntimeIdentifierToOutputPath) to false. We only need a single HelloAndroid.dll and HelloAndroid.apk file.

I do not think the Android SDK should set this property to false. It sounds like you have 4 separate builds (or possibly five, if you include the entry point build) outputting to the same folder, which among other things will lead to failures with multiprocess builds when the different builds try to write to the same file concurrently.

Other than that, the flow looks good, though I think @mhutch may have had a different view about how we should be handling multiple architectures, but I can't remember the details. Also it would be nice to avoid compiling the project an extra time for each RuntimeIdentifier if that's what it's doing now, but I'm not sure if there's a straightforward way to do that.

For the question about whether the MVIDs from different runtime packs should match, tagging @jeffschwMSFT, @swaroop-sridhar, and @ericstj to loop in the right people.

[jonathanpeppers]

@dsplaisted do you know what task/targets might output the same files if $(AppendRuntimeIdentifierToOutputPath) is false? I tried to make sure none of the targets in Microsoft.Android.Sdk that generate files will run in the nested build.

Here is a .binlog: msbuild.zip

That said, I'm unsure about $(AppendRuntimeIdentifierToOutputPath) and what it should actually do.

Should $(RuntimeIdentifier) be set to an empty string if $(RuntimeIdentifiers) is used? That would work the same way, I think.

[marek-safar]

@jonathanpeppers I think we don't have the deterministic option enabled for illinker process libraries during runtime pack build step and that's what is causing the different outputs.

[dsplaisted]

Should $(RuntimeIdentifier) be set to an empty string if $(RuntimeIdentifiers) is used? That would work the same way, I think.

RuntimeIdentifier will be an empty string in the "outer" build, so it won't be appended to the path even if AppendRuntimeIdentifierToOutputPath is true in that build. The RuntimeIdentifier will be set in the nested "inner" build that is called from the _ResolveAssemblies target. In that build, the output path would be modified if you don't change the value of AppendRuntimeIdentifierToOutputPath.

I tried to make sure none of the targets in Microsoft.Android.Sdk that generate files will run in the nested build.

It is hard to ensure that nothing in the target dependency graph of _ComputeFilesToPublishForRuntimeIdentifiers writes to the output path. Currently that includes the _RunILLink target which includes the build output as its input, but doesn't actually depend on the Build target that generates that output. I can easily imagine that target's dependencies being updated, and then you would have multiple inner builds writing to the same output path.

So if you don't override AppendRuntimeIdentifierToOutputPath, then you are more protected against problems when more happens in the inner builds than you expected. However, you would need to figure out how to find the right ILLink inputs from the outer build output folder. It would probably also be good to have test coverage ensuring that you're not doing extra useless work in the inner builds (for example, if it was compiling the project in each inner build and putting it in a RID-specific output folder, but that was never used).

[jonathanpeppers]

@dsplaisted thanks, I'm hitting this without $(AppendRuntimeIdentifierToOutputPath) set to false:

C:\Program Files\dotnet\sdk\5.0.100-preview.7.20304.6\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.ILLink.targets(149,5): error MSB4018: The "ComputeManagedAssemblies" task failed unexpectedly.
System.IO.FileNotFoundException: Could not find file 'C:\src\net5-samples\HelloAndroid\obj\Release\net5.0\android.21-x86\HelloAndroid.dll'.
File name: 'C:\src\net5-samples\HelloAndroid\obj\Release\net5.0\android.21-x86\HelloAndroid.dll'
   at System.IO.FileStream.ValidateFileHandle(SafeFileHandle fileHandle)
   at System.IO.FileStream.CreateFileOpenHandle(FileMode mode, FileShare share, FileOptions options)
   at System.IO.FileStream..ctor(String path, FileMode mode, FileAccess access, FileShare share, Int32 bufferSize, FileOptions options)
   at System.IO.FileStream..ctor(String path, FileMode mode, FileAccess access, FileShare share)
   at Mono.Cecil.ModuleDefinition.GetFileStream(String fileName, FileMode mode, FileAccess access, FileShare share)
   at Mono.Cecil.ModuleDefinition.ReadModule(String fileName, ReaderParameters parameters)
   at Mono.Cecil.ModuleDefinition.ReadModule(String fileName)
   at Utils.IsManagedAssembly(String fileName)
   at ILLink.Tasks.ComputeManagedAssemblies.<>c.<Execute>b__8_0(ITaskItem f)
   at System.Linq.Enumerable.WhereArrayIterator`1.ToArray()
   at ILLink.Tasks.ComputeManagedAssemblies.Execute()
   at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute()
   at Microsoft.Build.BackEnd.TaskBuilder.ExecuteInstantiatedTask(ITaskExecutionHost taskExecutionHost, TaskLoggingContext taskLoggingContext, TaskHost taskHost, ItemBucket bucket, TaskExecutionMode howToExecuteTask) [C:\src\net5-samples\HelloAndroid\HelloAndroid.csproj]

The file produced by the outer build is obj\Release\net5.0\HelloAndroid.dll.

It looks like I just need to fix up @(IntermediateAssembly) in the inner builds, and I think everything will work 👍 .

[jonathanpeppers]

I think we can close this, as I think the changes here will work: dotnet/android#4767

@marek-safar would this fix the runtime packs?

<ILLinkArgs>$(ILLinkArgs) --deterministic true</ILLinkArgs>

added in here:

https://github.com/dotnet/runtime/blob/e2ed3631b9a2a97de10991e4b548f63086cdc67a/eng/illink.targets#L141-L142

I can send a PR if it's that easy (and this is the right file).

[marek-safar]

@jonathanpeppers I think it should be we cannot enable it as it exposes Cecil bug we didn't resolve yet