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.

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

[XABT] Create separate assembly preparation tasks. #9637

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Dec 19, 2024

Break down the CollectAssemblyFilesForArchive task further into individual tasks.

These tasks each handle a separate concern about preparing the assemblies for the apk:

  • CompressAssemblies - Compresses assemblies using LZ4 compression (independent of ZIP compression).
  • CreateAssemblyStore - Bundles assemblies into a single .blob file.
  • WrapAssembliesAsSharedLibraries - Wraps the assemblies as shared libraries so they can go in the lib directory of the APK.

This should allow them to be moved to separate incremental targets in the future. For example, we do not need to compress all assemblies on every build if they haven't changed since the previous build.

{
var value = item.GetMetadata (name);

if (string.IsNullOrWhiteSpace (value))
return defaultValue;

return value;
return (T?)Convert.ChangeType (value, typeof (T));
Copy link
Contributor

Choose a reason for hiding this comment

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

This throws https://learn.microsoft.com/en-us/dotnet/api/system.convert.changetype?view=net-9.0. We should handle that case? and return the default.

Copy link
Contributor Author

@jpobst jpobst Jan 6, 2025

Choose a reason for hiding this comment

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

I think this only throws if the input cannot be converted to the requested type, like say you passed in "tru" and T is bool.

Passing in an invalid input is a bug that should be fixed, and it feels like we should indeed throw an exception here rather than silently swallow the error and return the default.

@jonpryor
Copy link
Member

Recording what was mentioned on an earlier call:

We need to simplify our build system (and related) to reduce maintenance costs, up to and including removing infrequently/rarely used features.

While we need to continue to support $(EmbedAssembliesIntoApk)=True, that does not meant that we need to continue to support "one file per assembly within the .apk" (86260ed), which is Yet Another Codepath that we need to support for minimal gain. $(EmbedAssembliesIntoApk)=True should instead be an Assembly Store, even for Debug builds.

The appropriate fix for #9455 is not to "make $(EmbedAssembliesIntoApk)=True faster"; it is instead to make Fast Deployment more reliable. If Fast Deployment is only successful 40% of the time, we need more information (adb logcat output, MSBuild .binlog files around dotnet build -t:Install) so that we can fix those reliability issues.

@jpobst
Copy link
Contributor Author

jpobst commented Jan 14, 2025

$(EmbedAssembliesIntoApk)=True should instead be an Assembly Store, even for Debug builds.

One caveat with this today is that assembly stores do not contain .pdb files, so you can't debug when using assembly stores. I would assume this is fixable but may require runtime loader changes. 🤷

void DoAddAssembliesFromArchCollection (TaskLoggingHelper log, AndroidTargetArch arch, ITaskItem assembly, DSOWrapperGenerator.Config dsoWrapperConfig, PackageFileListBuilder files, bool debug, bool compress, IDictionary<AndroidTargetArch, Dictionary<string, CompressedAssemblyInfo>>? compressedAssembliesInfo, string compressedOutputDir, AssemblyStoreBuilder? storeBuilder)
{
// In the "all assemblies are per-RID" world, assemblies, pdb and config are disguised as shared libraries (that is,
// their names end with the .so extension) so that Android allows us to put them in the `lib/{ARCH}` directory.
// For this reason, they have to be treated just like other .so files, as far as compression rules are concerned.
// Thus, we no longer just store them in the apk but we call the `GetCompressionMethod` method to find out whether
// or not we're supposed to compress .so files.
var sourcePath = CompressAssembly (assembly, compress, compressedAssembliesInfo, compressedOutputDir);
if (UseAssemblyStore) {
storeBuilder!.AddAssembly (sourcePath, assembly, includeDebugSymbols: debug);
return;
}
// Add assembly
(string assemblyPath, string assemblyDirectory) = GetInArchiveAssemblyPath (assembly);
string wrappedSourcePath = DSOWrapperGenerator.WrapIt (Log, dsoWrapperConfig, arch, sourcePath, Path.GetFileName (assemblyPath));
files.AddItem (wrappedSourcePath, assemblyPath);
// Try to add config if exists
var config = Path.ChangeExtension (assembly.ItemSpec, "dll.config");
AddAssemblyConfigEntry (dsoWrapperConfig, files, arch, assemblyDirectory, config);
// Try to add symbols if Debug
if (!debug) {
return;
}
string symbols = Path.ChangeExtension (assembly.ItemSpec, "pdb");
if (!File.Exists (symbols)) {
return;
}
string archiveSymbolsPath = assemblyDirectory + MonoAndroidHelper.MakeDiscreteAssembliesEntryName (Path.GetFileName (symbols));
string wrappedSymbolsPath = DSOWrapperGenerator.WrapIt (Log, dsoWrapperConfig, arch, symbols, Path.GetFileName (archiveSymbolsPath));
files.AddItem (wrappedSymbolsPath, archiveSymbolsPath);
}

We default assembly stores to false if using debug symbols:

<AndroidUseAssemblyStore Condition=" '$(AndroidUseAssemblyStore)' == '' and ('$(EmbedAssembliesIntoApk)' != 'true' or '$(AndroidIncludeDebugSymbols)' == 'true') ">false</AndroidUseAssemblyStore>
<AndroidUseAssemblyStore Condition=" '$(AndroidUseAssemblyStore)' == '' ">true</AndroidUseAssemblyStore>

@jpobst jpobst force-pushed the separate-compress branch from 1ce7fb1 to 33cee50 Compare January 14, 2025 21:34
@dotnet dotnet deleted a comment from azure-pipelines bot Jan 14, 2025
@jpobst jpobst force-pushed the separate-compress branch from 33cee50 to ea10b6f Compare January 15, 2025 18:45
@dotnet dotnet deleted a comment from azure-pipelines bot Jan 15, 2025
@jpobst jpobst changed the title [XABT] Create separate compress assemblies task. [XABT] Create separate assembly preparation tasks. Jan 15, 2025
@jpobst jpobst marked this pull request as ready for review January 15, 2025 20:35

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 8 changed files in this pull request and generated no comments.

Files not reviewed (3)
  • src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets: Language not supported
  • src/Xamarin.Android.Build.Tasks/Tasks/CollectAssemblyFilesForArchive.cs: Evaluated as low risk
  • src/Xamarin.Android.Build.Tasks/Tasks/CompressAssemblies.cs: Evaluated as low risk
Comments suppressed due to low confidence (1)

src/Xamarin.Android.Build.Tasks/Tasks/CreateAssemblyStore.cs:79

  • The metadata value may not be a valid boolean. Consider using bool.TryParse to ensure the metadata value is a valid boolean.
var should_skip = asm.GetMetadataOrDefault ("AndroidSkipAddToPackage", false);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants