Skip to content

[monodroid] Properly process satellite assemblies #7823

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

Merged
merged 5 commits into from
Feb 28, 2023
Merged

[monodroid] Properly process satellite assemblies #7823

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
24a7e81
[monodroid] Properly process satellite assemblies
grendello Feb 22, 2023
b475944
Test(s)
grendello Feb 23, 2023
dab54df
Fix the new test
grendello Feb 23, 2023
1b1df89
Merge branch 'main' into issue7819
grendello Feb 27, 2023
dde53ee
Use the `builder` field in the test
grendello Feb 27, 2023
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
4 changes: 3 additions & 1 deletion src/Xamarin.Android.Build.Tasks/Tasks/GeneratePackageManagerJava.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,9 @@ void AddEnvironment ()
HashSet<string> archAssemblyNames = null;
HashSet<string> uniqueAssemblyNames = new HashSet<string> (StringComparer.OrdinalIgnoreCase);
Action<ITaskItem> updateAssemblyCount = (ITaskItem assembly) => {
string assemblyName = Path.GetFileName (assembly.ItemSpec);
// We need to use the 'RelativePath' metadata, if found, because it will give us the correct path for satellite assemblies - with the culture in the path.
string? relativePath = assembly.GetMetadata ("RelativePath");
Comment on lines +268 to +269
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need to change this to use %(DestinationSubPath)? I thought we saw that architecture-specific assemblies could also hit a similar issue?

Copy link
Contributor Author

@grendello grendello Feb 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

No, architecture specific assemblies are handled correctly (if they didn't, we'd see this issue much earlier). DestinationSubPath would only make matters worse - because I would have to know which is arch-specific assembly and which is a satellite assembly, to treat the prefix differently. RelativePath appears to be unambiguous for this purpose.

string assemblyName = String.IsNullOrEmpty (relativePath) ? Path.GetFileName (assembly.ItemSpec) : relativePath;
Comment on lines +268 to +270
Copy link
Member

Choose a reason for hiding this comment

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

Do you have a .binlog of what was happening here?

In other places we used %(DestinationSubDirectory) because it was item metadata we create & control what the value is:

https://github.com/xamarin/monodroid/commit/1f52d5873960cf72685b9b08dbe262fc1ad4ca13#diff-9eaf7f76682d3de57a39276db5c6229ac0dd7ea19c9b755c11c7be5d748456a5R571

I think we should just check which one we should use everywhere. /cc @dellis1972

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are the relevant entries for statellite assemblies as passed to GeneratePackageManagerJava. DestinationDirectory or DestinationSubPath could be used as well, but I figured RelativePath was a standard MSBuild metadata item?

Task Parameter:
                         ResolvedUserAssemblies=
                             ~/.nuget/packages/microsoft.maui.controls.core/8.0.0-preview.1.7762/lib/net8.0-android33.0/ar/Microsoft.Maui.Controls.resources.dll
                                     AdditionalProperties=RuntimeIdentifier=android-arm;
                             _ComputeFilesToPublishForRuntimeIdentifiers=true
                             ;AppendRuntimeIdentifierToOutputPath=true
                             ;SkipCompilerExecution=true
                             ;_OuterIntermediateAssembly=obj/Release/net8.0-android/issue7819.dll
                             ;_OuterOutputPath=bin/Release/net8.0-android/
                             ;_OuterIntermediateOutputPath=obj/Release/net8.0-android/
                           
                                     AssetType=resources
                                     CopyLocal=true
                                     CopyToPublishDirectory=PreserveNewest
                                     Culture=ar
                                     DestinationSubDirectory=ar/
                                     DestinationSubPath=ar/Microsoft.Maui.Controls.resources.dll
                                     FrameworkAssembly=False
                                     HasMonoAndroidReference=False
                                     MSBuildSourceProjectFile=~/.../issue7819/issue7819.csproj
                                     MSBuildSourceTargetName=_ComputeFilesToPublishForRuntimeIdentifiers
                                     NuGetPackageId=Microsoft.Maui.Controls.Core
                                     NuGetPackageVersion=8.0.0-preview.1.7762
                                     OriginalItemSpec=issue7819.csproj
                                     PathInPackage=lib/net8.0-android33.0/ar/Microsoft.Maui.Controls.resources.dll
                                     RelativePath=ar/Microsoft.Maui.Controls.resources.dll
                                     RuntimeIdentifier=android-arm
                             ~/.nuget/packages/microsoft.maui.controls.core/8.0.0-preview.1.7762/lib/net8.0-android33.0/ca/Microsoft.Maui.Controls.resources.dll
                                     AdditionalProperties=RuntimeIdentifier=android-arm;
                             _ComputeFilesToPublishForRuntimeIdentifiers=true
                             ;AppendRuntimeIdentifierToOutputPath=true
                             ;SkipCompilerExecution=true
                             ;_OuterIntermediateAssembly=obj/Release/net8.0-android/issue7819.dll
                             ;_OuterOutputPath=bin/Release/net8.0-android/
                             ;_OuterIntermediateOutputPath=obj/Release/net8.0-android/
                           
                                     AssetType=resources
                                     CopyLocal=true
                                     CopyToPublishDirectory=PreserveNewest
                                     Culture=ca
                                     DestinationSubDirectory=ca/
                                     DestinationSubPath=ca/Microsoft.Maui.Controls.resources.dll
                                     FrameworkAssembly=False
                                     HasMonoAndroidReference=False
                                     MSBuildSourceProjectFile=~/.../issue7819/issue7819.csproj
                                     MSBuildSourceTargetName=_ComputeFilesToPublishForRuntimeIdentifiers
                                     NuGetPackageId=Microsoft.Maui.Controls.Core
                                     NuGetPackageVersion=8.0.0-preview.1.7762
                                     OriginalItemSpec=issue7819.csproj
                                     PathInPackage=lib/net8.0-android33.0/ca/Microsoft.Maui.Controls.resources.dll
                                     RelativePath=ca/Microsoft.Maui.Controls.resources.dll
                                     RuntimeIdentifier=android-arm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that it isn't just a path - it's the de-facto assembly name. The runtime will ask us to load an assembly named culture/Assembly.dll, so we absolutely must keep that convention

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should be ok to use RelativePath here, but DestinationSubPath should also work as we use that for fast deployment.

Copy link
Member

@jonathanpeppers jonathanpeppers Feb 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We have some logic that sets %(DestinationSubPath) if the assembly is architecture-specific:

https://github.com/xamarin/xamarin-android/blob/17916001b80e9e5a963418c14fb0254497903ab3/src/Xamarin.Android.Build.Tasks/Tasks/ProcessAssemblies.cs#L235-L239

I don't think %(RelativePath) would have a value for armeabi-v7a/Assembly.dll.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jonathanpeppers yeah, %(RelativePath) didn't have the arch-specific prefix

if (!uniqueAssemblyNames.Contains (assemblyName)) {
uniqueAssemblyNames.Add (assemblyName);
}
Expand Down
17 changes: 13 additions & 4 deletions src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/PackagingTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -807,12 +807,18 @@ public void MissingSatelliteAssemblyInLibrary ()
new BuildItem ("EmbeddedResource", "Foo.resx") {
TextContent = () => InlineData.ResxWithContents ("<data name=\"CancelButton\"><value>Cancel</value></data>")
},
new BuildItem ("EmbeddedResource", "Foo.es.resx") {
TextContent = () => InlineData.ResxWithContents ("<data name=\"CancelButton\"><value>Cancelar</value></data>")
}
}
};

var languages = new string[] {"es", "de", "fr", "he", "it", "pl", "pt", "ru", "sl" };
foreach (string lang in languages) {
lib.OtherBuildItems.Add (
new BuildItem ("EmbeddedResource", $"Foo.{lang}.resx") {
TextContent = () => InlineData.ResxWithContents ($"<data name=\"CancelButton\"><value>{lang}</value></data>")
}
);
}

var app = new XamarinAndroidApplicationProject {
IsRelease = true,
};
Expand All @@ -829,7 +835,10 @@ public void MissingSatelliteAssemblyInLibrary ()
var apk = Path.Combine (Root, appBuilder.ProjectDirectory,
app.OutputPath, $"{app.PackageName}-Signed.apk");
var helper = new ArchiveAssemblyHelper (apk);
Assert.IsTrue (helper.Exists ($"assemblies/es/{lib.ProjectName}.resources.dll"), "Apk should contain satellite assemblies!");

foreach (string lang in languages) {
Assert.IsTrue (helper.Exists ($"assemblies/{lang}/{lib.ProjectName}.resources.dll"), $"Apk should contain satellite assembly for language '{lang}'!");
}
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/Xamarin.Android.Build.Tasks/Utilities/MarshalMethodsNativeAssemblyGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -816,7 +816,8 @@ void WriteHashes<T> () where T: struct
uint index = 0;

foreach (string name in uniqueAssemblyNames) {
string clippedName = Path.GetFileNameWithoutExtension (name);
// We must make sure we keep the possible culture prefix, which will be treated as "directory" path here
string clippedName = Path.Combine (Path.GetDirectoryName (name) ?? String.Empty, Path.GetFileNameWithoutExtension (name));
ulong hashFull = HashName (name, is64Bit);
ulong hashClipped = HashName (clippedName, is64Bit);

Expand Down
12 changes: 4 additions & 8 deletions src/monodroid/jni/xamarin-android-app-context.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ MonodroidRuntime::get_method_name (uint32_t mono_image_index, uint32_t method_to
{
uint64_t id = (static_cast<uint64_t>(mono_image_index) << 32) | method_token;

log_debug (LOG_ASSEMBLY, "Looking for name of method with id 0x%llx, in mono image at index %u", id, mono_image_index);
log_debug (LOG_ASSEMBLY, "MM: looking for name of method with id 0x%llx, in mono image at index %u", id, mono_image_index);
size_t i = 0;
while (mm_method_names[i].id != 0) {
if (mm_method_names[i].id == id) {
Expand Down Expand Up @@ -58,19 +58,15 @@ MonodroidRuntime::get_function_pointer (uint32_t mono_image_index, uint32_t clas
// We need to do that, as Mono APIs cannot be invoked from threads that aren't attached to the runtime.
mono_thread_attach (mono_get_root_domain ());

// We don't check for valid return values from image loader, class and method lookup because if any
// of them fails to find the requested entity, they will return `null`. In consequence, we can pass
// these pointers without checking all the way to `mono_method_get_unmanaged_callers_only_ftnptr`, after
// which call we check for errors. This saves some time (not much, but definitely more than zero)
MonoImage *image = MonoImageLoader::get_from_index (mono_image_index);
MarshalMethodsManagedClass &klass = marshal_methods_class_cache[class_index];
if (klass.klass == nullptr) {
klass.klass = mono_class_get (image, klass.token);
klass.klass = image != nullptr ? mono_class_get (image, klass.token) : nullptr;
}

MonoMethod *method = mono_get_method (image, method_token, klass.klass);
MonoMethod *method = klass.klass != nullptr ? mono_get_method (image, method_token, klass.klass) : nullptr;
MonoError error;
void *ret = mono_method_get_unmanaged_callers_only_ftnptr (method, &error);
void *ret = method != nullptr ? mono_method_get_unmanaged_callers_only_ftnptr (method, &error) : nullptr;

if (XA_LIKELY (ret != nullptr)) {
if constexpr (NeedsLocking) {
Expand Down
47 changes: 47 additions & 0 deletions tests/MSBuildDeviceIntegration/Tests/InstallAndRunTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,56 @@ public void Teardown ()
Directory.Delete (builder.ProjectDirectory, recursive: true);

builder?.Dispose ();
builder = null;
proj = null;
}

[Test]
public void NativeAssemblyCacheWithSatelliteAssemblies ()
{
var path = Path.Combine ("temp", TestName);
var lib = new XamarinAndroidLibraryProject {
ProjectName = "Localization",
OtherBuildItems = {
new BuildItem ("EmbeddedResource", "Foo.resx") {
TextContent = () => InlineData.ResxWithContents ("<data name=\"CancelButton\"><value>Cancel</value></data>")
},
}
};

var languages = new string[] {"es", "de", "fr", "he", "it", "pl", "pt", "ru", "sl" };
foreach (string lang in languages) {
lib.OtherBuildItems.Add (
new BuildItem ("EmbeddedResource", $"Foo.{lang}.resx") {
TextContent = () => InlineData.ResxWithContents ($"<data name=\"CancelButton\"><value>{lang}</value></data>")
}
);
}

proj = new XamarinAndroidApplicationProject {
IsRelease = true,
};
proj.References.Add (new BuildItem.ProjectReference ($"..\\{lib.ProjectName}\\{lib.ProjectName}.csproj", lib.ProjectName, lib.ProjectGuid));
proj.SetAndroidSupportedAbis ("armeabi-v7a", "arm64-v8a", "x86", "x86_64");

using (var libBuilder = CreateDllBuilder (Path.Combine (path, lib.ProjectName))) {
builder = CreateApkBuilder (Path.Combine (path, proj.ProjectName));
Assert.IsTrue (libBuilder.Build (lib), "Library Build should have succeeded.");
Assert.IsTrue (builder.Install (proj), "Install should have succeeded.");

var apk = Path.Combine (Root, builder.ProjectDirectory, proj.OutputPath, $"{proj.PackageName}-Signed.apk");
var helper = new ArchiveAssemblyHelper (apk);

foreach (string lang in languages) {
Assert.IsTrue (helper.Exists ($"assemblies/{lang}/{lib.ProjectName}.resources.dll"), $"Apk should contain satellite assembly for language '{lang}'!");
}

Assert.True (builder.RunTarget (proj, "_Run"), "Project should have run.");
Assert.True (WaitForActivityToStart (proj.PackageName, "MainActivity",
Path.Combine (Root, builder.ProjectDirectory, "logcat.log"), 30), "Activity should have started.");
}
}

[Test]
public void GlobalLayoutEvent_ShouldRegisterAndFire_OnActivityLaunch ([Values (false, true)] bool isRelease)
{
Expand Down