[dotnet] Add support for universal apps. #11983

rolfbjarne · 2021-06-18T08:45:36Z

Add support for universal / fat apps for iOS, macOS and Mac Catalyst.

We detect if we need to build a universal app by checking if RuntimeIdentifiers (plural) is set, and in that case we do a complete inner build for every RuntimeIdentifier, and then once those inner builds are done, we merge the resulting .app bundles together (using a new MSBuild task called MergeAppBundles).

When merging app bundles together, we'll run into files that exist in both apps. Depending on the file type, we do different things:

MachO flies: lipo'ed together.
Managed assemblies: we do a binary comparison, if the assemblies are different, we put them in a RID-specific subdirectory. At runtime we know to look for assemblies in this directory.
runtimeconfig.bin, icudt.dat: put in a RID-specific subdirectory.
Info.plist: computed in the outer (fat) build, the one from the inner build is ignored.
Other files: for identical files we just copy one, otherwise we show an error.

If we run into files that are different between apps, but we should handle somehow, then we'll have to decide on a case-to-case basis what to do.

Some code shuffling was required to increase code sharing between the tools/ code, the msbuild/ code, and tests.

I've also added support for a default RuntimeIdentifier.

@spouliot FYI this PR incorporates most of #11320 (it was needed to resolve the location of icudt.dat at runtime).

Fixes #10294.
Fixes #10861.

This PR is best reviewed commit-by-commit.

…rectory.

This makes writing tests a bit easier, since we can use the same test project name for all platforms.

…an 8 bytes

…make it usable in more places

…crease code sharing

…figs.

…alid RuntimeIdentifier. Fixes xamarin#10861. Also add tests: * Remove RuntimeIdentifier from a few sample projects. These projects should continue to build just fine. * Add tests for invalid RuntimeIdentifiers. Fixes xamarin#10861.

… build'

…he outer build of a multi-rid build. There's nothing to publish (except the merged app bundle, which we'll handle ourselves).

…s into a single universal/fat app.

* Do an inner build per RuntimeIdentifier. * In the outer build, execute the new MergeAppBundles task to merge the resulting app bundles from the inner builds.

It's not clear what we're supposed to do if both RuntimeIdentifier and RuntimeIdentifiers are set, so just handle this case as if only RuntimeIdentifiers is set, by clearing out the RuntimeIdentifier value. Also show a warning for this scenario.

…kTest.

…ake which may call 'dotnet build'.

Add a _MapRuntimeIdentifierToTargetArchitecture target that computes the TargetArchitectures property from either the RuntimeIdentifier or the RuntimeIdentifiers properties. Also make sure this target is executed before _ComputeTargetArchitectures. This is required so that we have a correct TargetArchitectures value for multi-rid builds when compiling the app manifest (Info.plist).

spouliot

❤️

spouliot · 2021-06-23T17:31:03Z

tools/common/MachO.cs

+					return false;
+				using (var reader = new BinaryReader (fs)) {
+					var magic = reader.ReadUInt32 ();
+					reader.BaseStream.Position = 0;


why reset the position ?
as the reader is not used before being disposed

This was copied from somewhere else - I'll remove this line.

spouliot · 2021-06-23T17:52:50Z

msbuild/Xamarin.MacDev.Tasks.Core/ErrorHelper.msbuild.cs

+
+		internal static string Prefix {
+			get {
+				return "MSB";


can we [re-]use this prefix ?

spouliot · 2021-06-23T17:55:23Z

tools/common/MachO.cs

+
+		public static bool IsStaticLibrary (string filename, bool throw_if_error = false)
+		{
+			using (var fs = new FileStream (filename, FileMode.Open, FileAccess.Read, FileShare.Read)) {


File.OpenRead (filename) is simpler (and identical)
https://docs.microsoft.com/en-us/dotnet/api/system.io.file.openread?view=net-5.0

spouliot · 2021-06-23T17:56:22Z

tools/common/PathUtils.cs

+				throw new Exception (string.Format ("Could not readlink '{0}': {1}", path, Marshal.GetLastWin32Error ()));
+
+			return Encoding.UTF8.GetString (buffer, 0, rv);
+		}


was not that moved (in a previous commit) to share that code ?

Are you referring to 7aec86f?

If so, this is new code (to get the target of a symlink), the code that moved was to check if a file is a symlink or not.

spouliot · 2021-06-23T18:13:52Z

runtime/runtime.m

+	char *icu_dat_file_path = NULL;
+
+	char path [1024];
+	if (!xamarin_locate_app_resource (xamarin_icu_dat_file_name, path, sizeof (path))) {


That looks like ios-arm and ios-arm64 each as a copy of the icu data file ?
I don't recall if those are identical (or not) between architectures... but if they are then it's a big opportunity for sharing it

Yes, the icu data file is different between architectures (the arm one is bigger):

ls -la [...]/xamarin-macios/tests/dotnet/packages/microsoft.netcore.app.runtime.mono.ios-arm*/6.0.0-preview.6.21276.13/runtimes/ios-arm*/native/icudt.dat -rwxr--r-- 1 rolf wheel 1713152 May 18 21:54 [...]/xamarin-macios/tests/dotnet/packages/microsoft.netcore.app.runtime.mono.ios-arm/6.0.0-preview.6.21276.13/runtimes/ios-arm/native/icudt.dat -rwxr--r-- 1 rolf wheel 1636880 May 18 21:54 [...]/xamarin-macios/tests/dotnet/packages/microsoft.netcore.app.runtime.mono.ios-arm64/6.0.0-preview.6.21276.13/runtimes/ios-arm64/native/icudt.dat

The difference is unintentional. I don't think there's a tracking issue for it but I already mentioned it few times in some comments. The ICU data are generated from same input using the same tools. My theory is that there is some compression involved and the zlib library consumed by different builds is slightly different version with different defaults.

Yeah that's unintended, they should be identical. Filed dotnet/runtime#55637

Thanks, I was about to file an issue and look into it.

@filipnavara I wonder if ICU data for 64bit is a bit bigger due to some 64bit paddings etc for performance sake 🙂

@EgorBo Unlikely. The arm64 ones on Android and iOS are different size (dotnet/designs#225 (comment)). I don't think it's even memory mapped so the micro-optimizations would not make much difference.

vs-mobiletools-engineering-service2 · 2021-06-24T02:50:38Z

❌ [PR Build] Tests failed on Build ❌

Tests failed on Build.

API diff

✅ API Diff from stable

View API diff

iOS vsdrops gist
macOS vsdrops gist
macCat vsdrops gist
macCatiOS gist
tvOS vsdrops gist
watchOS vsdrops gist

API & Generator diff

✅ API Diff (from PR only) (no change)
✅ Generator Diff (no change)

GitHub pages

Results can be found in the following github pages (it might take some time to publish):

Test results

3 tests failed, 218 tests passed.

Failed tests

monotouch-test/Mac Catalyst/Debug [dotnet]: Failed (Tests run: 2622 Passed: 2489 Inconclusive: 35 Failed: 1 Ignored: 132)
MSBuild tests/Tasks: Failed (Execution failed with exit code 7)
DotNet tests: Failed (Execution failed with exit code 1)

Pipeline on Agent XAMBOT-1098.BigSur'
Merge 7f505c3 into 1f84aaa

…d code.

Fixes this error: > error : Unable to merge the file 'Contents/Resources/MonoTouchDebugConfiguration.txt', it's different between the input app bundles.

…rms. Fixes this error: > Unable to merge the file 'Contents/Info.plist', it's different between the input app bundles.

* Don't use a fake target, it'll always be executed.

…lds as well.

emaf · 2021-06-24T14:05:16Z

dotnet/targets/Xamarin.Shared.Sdk.targets

+		Returns="@(_AssemblyPublishDirectory)"
+		>
+		<ItemGroup>
+			<_AssemblyPublishDirectory Include="$(MSBuildProjectDirectory)/$(_AppBundlePath)">


I'm not sure if using full path here will work from Windows, because we'll end up passing a Windows full path to the task that will be executed on macOS.

@emaf I've changed this to be a relative path now.

emaf · 2021-06-24T14:08:01Z

dotnet/targets/Xamarin.Shared.Sdk.targets

+			ArchitectureSpecificFiles="@(_ArchitectureSpecificFiles)"
+			IgnoreFiles="Info.plist"
+			InputAppBundles="@(_AssemblyPublishDirectories)"
+			OutputAppBundle="$(MSBuildProjectDirectory)/$(_AppBundlePath)"


And we could potentially have a similar problem here.

@emaf same, I've changed this to be a relative path now.

…NET.

vs-mobiletools-engineering-service2 · 2021-06-24T15:21:51Z

🔥 Tests failed catastrophically on Build (no summary found). 🔥

Result file $(TEST_SUMMARY_PATH) not found.

Pipeline on Agent
Merge 31b48eb into 4914136

rolfbjarne · 2021-06-24T15:28:57Z

/azp run

azure-pipelines · 2021-06-24T15:29:15Z

Azure Pipelines successfully started running 1 pipeline(s).

To not cause problems on Windows.

vs-mobiletools-engineering-service2 · 2021-06-25T00:06:29Z

❌ [PR Build] Tests failed on Build ❌

Tests failed on Build.

API diff

✅ API Diff from stable

View API diff

iOS vsdrops gist
macOS vsdrops gist
macCat vsdrops gist
macCatiOS gist
tvOS vsdrops gist
watchOS vsdrops gist

API & Generator diff

✅ API Diff (from PR only) (no change)
✅ Generator Diff (no change)

GitHub pages

Results can be found in the following github pages (it might take some time to publish):

Test results

1 tests failed, 220 tests passed.

Failed tests

MSBuild tests/Tasks: Failed (Execution failed with exit code 7)

Pipeline on Agent XAMBOT-1096.BigSur'
Merge 31b48eb into 4914136

…ssembly, it may cause make to deadlock.

vs-mobiletools-engineering-service2 · 2021-06-25T16:18:14Z

✅ [PR Build] Tests passed on Build. ✅

Tests passed on Build.

API diff

✅ API Diff from stable

View API diff

iOS vsdrops gist
macOS vsdrops gist
macCat vsdrops gist
macCatiOS gist
tvOS vsdrops gist
watchOS vsdrops gist

API & Generator diff

✅ API Diff (from PR only) (no change)
✅ Generator Diff (no change)

GitHub pages

Results can be found in the following github pages (it might take some time to publish):

🎉 All 221 tests passed 🎉

Pipeline on Agent XAMBOT-1104.BigSur'
Merge 8aff15a into 69698be

rolfbjarne added 16 commits June 18, 2021 10:24

[runtime] Add support for finding assemblies in an RID-specific subdi…

87de6ea

…rectory.

[dotnet] Only trim when building a specific RuntimeIdentifier.

6fe54cb

[tests] Add unified simple test app for all platforms we support.

9a9562c

This makes writing tests a bit easier, since we can use the same test project name for all platforms.

[MachO] Make StaticLibrary.IsStaticLibrary work with files shorter th…

0021c2b

…an 8 bytes

�[tools] Create a PathUtils class and move some shareable code there.

7aec86f

[MachO] Add a MachO.IsMachOFile method

0af6ec5

[tools] Move parts of the binary file comparison to a helper file to …

ba83560

…make it usable in more places

[tools] Move another IsUptodate overload to the FileCopier file to in…

f864ff3

…crease code sharing

[tests] Add a complex test library project with resources and app con…

5955cd1

…figs.

[tests] Quote parameters containing semicolons when passed to 'dotnet…

e1f31c6

… build'

[dotnet] Don't execute ComputeResolvedFileToPublishList if we're in t…

003cf30

…he outer build of a multi-rid build. There's nothing to publish (except the merged app bundle, which we'll handle ourselves).

[msbuild] Implement a MergeAppBundles task to merge two (or more) app…

5c50a2f

…s into a single universal/fat app.

[tests] Add new tests for merging app bundles

4f8e9a8

[dotnet] Implement support for multiple RuntimeIdentifiers.

91311a0

* Do an inner build per RuntimeIdentifier. * In the outer build, execute the new MergeAppBundles task to merge the resulting app bundles from the inner builds.

rolfbjarne added not-notes-worthy Ignore for release notes run-dotnet-tests Run all the .NET tests run-all-tests Run all our tests. skip-device-tests Skip device tests labels Jun 18, 2021

This comment has been minimized.

Sign in to view

rolfbjarne added 7 commits June 21, 2021 11:36

Merge remote-tracking branch 'origin/main' into HEAD

62865cb

[msbuild] Fix symlink check to check for file presence first.

a32d605

[msbuild] Copy directories correctly.

4526a5e

[tests] Updated list of not-yet-localized error messages.

bc9e007

[tests] Improve error reporting when SetUp fails in MergeAppBundleTas…

718d954

…kTest.

[tests] Update tests according to the actual error messages.

f1cc00c

[dotnet] Set the default RuntimeIdentifier earlier.

7179cac

This comment has been minimized.

Sign in to view

[tests] Remove MSBuildSDKsPath from the environment before invoking m…

2f1667e

…ake which may call 'dotnet build'.

rolfbjarne changed the title ~~[DRAFT] [dotnet] Add support for universal apps.~~ [dotnet] Add support for universal apps. Jun 23, 2021

rolfbjarne marked this pull request as ready for review June 23, 2021 17:10

rolfbjarne requested review from emaf and spouliot as code owners June 23, 2021 17:10

spouliot approved these changes Jun 23, 2021

View reviewed changes

spouliot mentioned this pull request Jun 23, 2021

[dotnet] Allow choosing the ICU data file to be used for globalization #11320

Closed

rolfbjarne added 8 commits June 24, 2021 11:40

[msbuild] Use the same prefix in ErrorHelper as we do in other MSBuil…

9f82694

…d code.

[MachO] Simplify code a bit according to review.

df369da

[dotnet] Don't create debug settings / configurations for inner builds.

c5dad31

Fixes this error: > error : Unable to merge the file 'Contents/Resources/MonoTouchDebugConfiguration.txt', it's different between the input app bundles.

[dotnet] Use the correct relative paths for Info.plist for all platfo…

1fa1cf3

…rms. Fixes this error: > Unable to merge the file 'Contents/Info.plist', it's different between the input app bundles.

[tests] Fix assertions regarding errors after recent changes.

82fc13e

[tests] Fix makefile for ComplexAssembly to do nothing on a rebuild.

9a406d2

* Don't use a fake target, it'll always be executed.

[tests] Remove BOM in new file.

2f65937

[dotnet] Compute 'ComputedPlatform' correctly for outer multi-rid bui…

3497373

…lds as well.

emaf reviewed Jun 24, 2021

View reviewed changes

[tests] Copy more supporting files for building monotouch-test with .…

31b48eb

…NET.

[dotnet] Use relative paths in a few places.

6d6db24

To not cause problems on Windows.

[tests] Don't run parallel make in tests/common/TestProjects/ComplexA…

8aff15a

…ssembly, it may cause make to deadlock.

emaf approved these changes Jun 28, 2021

View reviewed changes

dalexsoto approved these changes Jun 28, 2021

View reviewed changes

rolfbjarne merged commit ebf6c13 into xamarin:main Jun 29, 2021

rolfbjarne deleted the dotnet-multi-rid branch June 29, 2021 09:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dotnet] Add support for universal apps. #11983

[dotnet] Add support for universal apps. #11983

rolfbjarne commented Jun 18, 2021 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

spouliot left a comment

spouliot Jun 23, 2021

rolfbjarne Jun 24, 2021

spouliot Jun 23, 2021

spouliot Jun 23, 2021

spouliot Jun 23, 2021

rolfbjarne Jun 24, 2021

spouliot Jun 23, 2021

rolfbjarne Jun 24, 2021

filipnavara Jul 7, 2021

akoeplinger Jul 14, 2021

filipnavara Jul 14, 2021

EgorBo Jul 14, 2021

filipnavara Jul 14, 2021 •

edited

Loading

vs-mobiletools-engineering-service2 commented Jun 24, 2021

Failed tests

emaf Jun 24, 2021

rolfbjarne Jun 25, 2021

emaf Jun 24, 2021 •

edited

Loading

rolfbjarne Jun 25, 2021

vs-mobiletools-engineering-service2 commented Jun 24, 2021

rolfbjarne commented Jun 24, 2021

azure-pipelines bot commented Jun 24, 2021

vs-mobiletools-engineering-service2 commented Jun 25, 2021

Failed tests

vs-mobiletools-engineering-service2 commented Jun 25, 2021

[dotnet] Add support for universal apps. #11983

[dotnet] Add support for universal apps. #11983

Conversation

rolfbjarne commented Jun 18, 2021 • edited Loading

This comment has been minimized.

This comment has been minimized.

spouliot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipnavara Jul 14, 2021 • edited Loading

Choose a reason for hiding this comment

vs-mobiletools-engineering-service2 commented Jun 24, 2021

❌ [PR Build] Tests failed on Build ❌

API diff

API & Generator diff

GitHub pages

Test results

Failed tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emaf Jun 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vs-mobiletools-engineering-service2 commented Jun 24, 2021

🔥 Tests failed catastrophically on Build (no summary found). 🔥

rolfbjarne commented Jun 24, 2021

azure-pipelines bot commented Jun 24, 2021

vs-mobiletools-engineering-service2 commented Jun 25, 2021

❌ [PR Build] Tests failed on Build ❌

API diff

API & Generator diff

GitHub pages

Test results

Failed tests

vs-mobiletools-engineering-service2 commented Jun 25, 2021

✅ [PR Build] Tests passed on Build. ✅

API diff

API & Generator diff

GitHub pages

🎉 All 221 tests passed 🎉

rolfbjarne commented Jun 18, 2021 •

edited

Loading

filipnavara Jul 14, 2021 •

edited

Loading

emaf Jun 24, 2021 •

edited

Loading