Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jun 28, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2560,5 +2560,22 @@ public void CheckLintResourceFileReferencesAreFixed ()
StringAssertEx.DoesNotContain (errorFilePath, b.LastBuildOutput, $"Path {errorFilePath} should have been replaced.");
}
}

[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.");
}
}
}
27 changes: 8 additions & 19 deletions src/Xamarin.Android.Build.Tasks/Utilities/MarshalMethodsAssemblyRewriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,12 @@ public void Rewrite (DirectoryAssemblyResolver resolver, List<string> targetAsse
foreach (AssemblyDefinition asm in uniqueAssemblies) {
foreach (string path in GetAssemblyPaths (asm)) {
var writerParams = new WriterParameters {
WriteSymbols = (File.Exists (path + ".mdb") || File.Exists (Path.ChangeExtension (path, ".pdb"))),
WriteSymbols = File.Exists (Path.ChangeExtension (path, ".pdb")),
};


string output = $"{path}.new";
string directory = Path.Combine (Path.GetDirectoryName (path), "new");
Directory.CreateDirectory (directory);
string output = Path.Combine (directory, Path.GetFileName (path));
log.LogDebugMessage ($"Writing new version of assembly: {output}");

// TODO: this should be used eventually, but it requires that all the types are reloaded from the assemblies before typemaps are generated
Expand All @@ -137,36 +138,23 @@ public void Rewrite (DirectoryAssemblyResolver resolver, List<string> targetAsse
// versions around.
foreach (string path in newAssemblyPaths) {
string? pdb = null;
string? mdb = null;

string source = Path.ChangeExtension (Path.Combine (Path.GetDirectoryName (path), Path.GetFileNameWithoutExtension (path)), ".pdb");
string source = Path.ChangeExtension (path, ".pdb");
if (File.Exists (source)) {
pdb = source;
}

source = $"{path}.mdb";
if (File.Exists (source)) {
mdb = source;
}

foreach (string targetPath in targetAssemblyPaths) {
string target = Path.Combine (targetPath, Path.GetFileNameWithoutExtension (path));
string target = Path.Combine (targetPath, Path.GetFileName (path));
CopyFile (path, target);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if (!String.IsNullOrEmpty (pdb)) {
target = Path.ChangeExtension (Path.Combine (targetPath, Path.GetFileNameWithoutExtension (pdb)), ".pdb");
CopyFile (pdb, target);
}

if (!String.IsNullOrEmpty (mdb)) {
target = Path.Combine (targetPath, Path.ChangeExtension (Path.GetFileName (path), ".mdb"));
CopyFile (mdb, target);
CopyFile (pdb, Path.ChangeExtension (target, ".pdb"));
}
}

RemoveFile (path);
RemoveFile (pdb);
RemoveFile (mdb);
}

void CopyFile (string source, string target)
Expand All @@ -182,6 +170,7 @@ void RemoveFile (string? path)
}

try {
log.LogDebugMessage ($"Deleting: {path}");
File.Delete (path);
} catch (Exception ex) {
log.LogWarning ($"Unable to delete source file '{path}'");
Expand Down