[Xamarin.Android.Build.Tasks] correct paths in MarshalMethodsAssemblyRewriter

[jonathanpeppers]

Context: dotnet/maui#15399 (comment)

During a PR updating AndroidX dependencies in .NET MAUI, we got the error:

Task GenerateJavaStubs
...
System.IO.IOException: The process cannot access the file 'D:\a\_work\1\s\src\Compatibility\ControlGallery\src\Android\obj\Release\net8.0-android\android\assets\Xamarin.AndroidX.CustomView.pdb' because it is being used by another process.
at System.IO.FileSystem.CopyFile(String sourceFullPath, String destFullPath, Boolean overwrite)
at Xamarin.Android.Tasks.MarshalMethodsAssemblyRewriter.Rewrite(DirectoryAssemblyResolver resolver, List`1 targetAssemblyPaths, Boolean brokenExceptionTransitions)
at Xamarin.Android.Tasks.GenerateJavaStubs.Run(DirectoryAssemblyResolver res, Boolean useMarshalMethods)
at Xamarin.Android.Tasks.GenerateJavaStubs.RunTask()
at Microsoft.Android.Build.Tasks.AndroidTask.Execute() in /Users/runner/work/1/s/xamarin-android/external/xamarin-android-tools/src/Microsoft.Android.Build.BaseTasks/AndroidTask.cs:line 25

Which has some very odd logging right before the failure:

Copying rewritten assembly: obj\Release\net8.0-android\android\assets\Xamarin.AndroidX.CustomView.PoolingContainer.dll.new -> obj\Release\net8.0-android\android\assets\Xamarin.AndroidX.CustomView.PoolingContainer.dll
Copying rewritten assembly: obj\Release\net8.0-android\android\assets\Xamarin.AndroidX.CustomView.PoolingContainer.pdb -> obj\Release\net8.0-android\android\assets\Xamarin.AndroidX.CustomView.pdb

Copying Xamarin.AndroidX.CustomView.PoolingContainer.pdb to Xamarin.AndroidX.CustomView.pdb?

I could reproduce the issue in a test with PublishTrimmed=false:

[Test]
public void SimilarAndroidXAssemblyNames ([Values(true, false)] bool publishTrimmed)
{
    var proj = new XamarinAndroidApplicationProject {
        IsRelease = true,
        AotAssemblies = publishTrimmed,
        PackageReferences = {
            new Package { Id = "Xamarin.AndroidX.CustomView", Version = "1.1.0.17" },
            new Package { Id = "Xamarin.AndroidX.CustomView.PoolingContainer", Version = "1.0.0.4" },
        }
    };
    proj.SetProperty (KnownProperties.PublishTrimmed, publishTrimmed.ToString());
    proj.MainActivity = proj.DefaultMainActivity.Replace ("//${AFTER_ONCREATE}", "AndroidX.CustomView.PoolingContainer.PoolingContainer.IsPoolingContainer (null);");
    using var builder = CreateApkBuilder ();
    Assert.IsTrue (builder.Build (proj), "Build should have succeeded.");
}

In MarshalMethodsAssemblyRewriter we write temporary assembly files to foo.new and copy to the original path at foo.dll. We next copy any symbol files if found.

I found two underlying issues here.

First, this Mono.Cecil API:

AssemblyDefinition.Write("foo.new", new WriterParameters {
    WriteSymbols = true
});

It would write a foo.pdb instead of foo.new.pdb, and so we have no way for this to write symbols to a temporary location.

I put the temporary location in a new subdirectory instead of appending .new to the path.

The second problem is this code:

target = Path.ChangeExtension (Path.Combine (targetPath, Path.GetFileNameWithoutExtension (pdb)), ".pdb");
CopyFile (pdb, target);

It appears to lose .PoolingContainer from the path, and so it uses the destination of:

obj\Release\net8.0-android\android\assets\Xamarin.AndroidX.CustomView.pdb

But using a new subdirectory instead, we bypass this issue.

After these changes, we instead get:

Copying rewritten assembly: obj\Release\android\assets\new\Xamarin.AndroidX.CustomView.PoolingContainer.dll -> obj\Release\android\assets\Xamarin.AndroidX.CustomView.PoolingContainer.dll
Copying rewritten assembly: obj\Release\android\assets\new\Xamarin.AndroidX.CustomView.PoolingContainer.pdb -> obj\Release\android\assets\Xamarin.AndroidX.CustomView.PoolingContainer.pdb
Deleting: obj\Release\android\assets\new\Xamarin.AndroidX.CustomView.PoolingContainer.dll
Deleting: obj\Release\android\assets\new\Xamarin.AndroidX.CustomView.PoolingContainer.pdb

Lastly, I removed .mdb file support -- but that is completely unrelated to the issue.

[jonpryor]

I realize that the "original" version in 8bc7a3e didn't do this, but should we verify that target exists before this CopyFile()?

@grendello?

targetPaths could contain multiple directories, such as arm64-v8a/linked and x86_64/linked, and it feels like this method could possibly copy/overwrite the "wrong" ABI?

Does MarshalMethodAssemblyRewriter properly deal with per-ABI assemblies? (See also 929e701?)

[grendello]

@jonpryor all the known target-specific assemblies are ones which we won't rewrite, since they don't have any Java.Object-derived classes. But yes, in general, the code would fail if it encountered Java.Object-derived types in a target-specific assembly, since the task (and thus MarshalMethodsAssemblyRewriter) would be passed several copies of those assemblies.

[grendello]

IOW, we're safe now with the code as-is, but it wouldn't hurt to amend it to make target-specific assemblies are handled properly. I guess the DestinationSubPath metadata item could be used to build the output path, by appending it to the new/ path?

[jonpryor]

@grendello: as per 929e701, any assembly can now become a target-specific assembly if (when) it uses IntPtr.Size. We likely shouldn't address this here, but we likely will need to address it.

[jonpryor]

We should merge this PR, and followup later with a more complete fix for per-ABI assemblies.

[grendello]

I agree, as it requires a review of all our tasks