[mmp] Add stripping of 32-bit dylibs to work with new App Store restrictions #3387

chamons · 2018-02-02T19:41:47Z

[macOS]Mac App failed to publish with error:"Unsupported Architectures. Your executable contained the following disallowed architectures: '[i386 Payload/MacCoolApp.app/Contents/MonoBundle/libMonoPosixHelper.dylib)]" #3367
Required some test work that might be more obvious by themselves. We can squash when landing.

- Turns out that /p:configuration:debug is not sufficient to tell mmp to do the right thing - That, in most projects, sets the DebugSymbols property, which really is what is checked. - However, two of our projects did not have that, so we always did release mmp work. - Removed configuration property for tests and addded real "Release" configuration option

- dotnet#3367 - App Store will now fail builds if you add in a 32-bit dylib - If you are a 32-bit app you don't need the 64-bit part of your fat dylib anyway - Add --native-strip to allow customization of behavior, as not everyone uses app store

monojenkins · 2018-02-02T22:19:39Z

Build failure

spouliot

❤️ the tests

spouliot · 2018-02-04T19:22:47Z

tools/mmp/driver.cs

+				string archToRemove = arch == "i386" ? "x86_64" : "i386";
+				int ret = XcodeRun ("lipo", $"{dest} -remove {archToRemove} -output {dest}");
+				if (ret != 0)
+					throw new MonoMacException (5310, true, "lipo failed with an error code '{0}'. Check build log for details.", ret);


is this a new error? if so it should be documented

Yes, I forgot.

spouliot · 2018-02-04T19:22:54Z

tools/mmp/driver.cs

+				if (ret != 0)
+					throw new MonoMacException (5310, true, "lipo failed with an error code '{0}'. Check build log for details.", ret);
+				if (name != "MonoPosixHelper")
+					ErrorHelper.Warning (1501, $"{name} was stripped of architecture {archToRemove} to comply with App Store restrictions. This could break exisiting codesigning signatures. Consider stripping the library with lipo or disabling with --native-strip=skip");


spouliot · 2018-02-04T19:23:13Z

tools/mmp/driver.cs

-
+			
 			bool isStaticLib = real_src.EndsWith (".a", StringComparison.Ordinal);
+			bool isDynamicLib = real_src.EndsWith (".dylib", StringComparison.Ordinal);


what happens for frameworks ?

Nothing, but something very likely should. Let me get someone in to test this case in submission to Apple.

rolfbjarne · 2018-02-05T06:42:34Z

tests/mmptest/src/CodeStrippingTests.cs

+
+				 test.Release = true;
+				 buildOutput = TI.TestUnifiedExecutable (test).BuildOutput;
+				 Assert.AreEqual (releaseStrips, didStrip (buildOutput), "Release lipo usage did not match expectations");


Maybe also test that the binary only has the desired architectures? We already have code to get the architectures of a binary file: https://github.com/xamarin/xamarin-macios/blob/bec116c273ab119c648522a28c3f650881ed4d9e/tests/mtouch/MTouch.cs#L3996

rolfbjarne · 2018-02-05T06:43:08Z

tests/common/mac/ProjectTestHelpers.cs

-				if (!string.IsNullOrEmpty (configuration))
-					buildArgs.Append ($" /property:Configuration={configuration} ");
-			} else
+			}


Style: else goes on the same line as the closing brace.

rolfbjarne · 2018-02-05T06:43:32Z

tools/mmp/driver.cs

 				CopyFileAndRemoveReadOnly (real_src, dest);
 			}
-
+			


Whitespace noise.

monojenkins · 2018-02-16T23:15:16Z

Build failure

chamons · 2018-02-16T23:18:30Z

I believe I've handled the current code review issues and added framework stripping.

I did not build after merging master, as a mono bump took too long before end of day. I'll check back tomorrow. Please consider re-reviewing as a lot has changed.

monojenkins · 2018-02-16T23:20:10Z

Build failure

monojenkins · 2018-02-17T02:14:08Z

Build failure

spouliot

Needs to discuss optimizations/docs
But otherwise it seems good :)

spouliot · 2018-02-17T16:10:12Z

tools/mmp/driver.cs

+					default:
+						ErrorHelper.Error (26, $"Could not parse the command line argument '-native-strip:{v}'");
+						break;
+					}


I wonder if that should be added in the new optimizations ? c.c. @rolfbjarne
It behaves just like them, i.e. a sane default (computed based on the project settings) and overrides for enabling and disabling it.
In any case it needs to be documented outside the errors code it introduce.

We already have two interpretations of strip: mtouch takes the arguments nostrip (to not strip IL from managed assemblies) and nosymbolstrip (to not strip native symbols from the executable). Both of these makes sense (their command-line tools are named mono-cil-strip and strip, respectively), but for this case, the work "strip" doesn't even show up in the man page for lipo.

My suggestion: --trim-architectures.

I think this could make sense to include as an optimization (it even makes sense for XI as well, since we do the same thing there, the only difference would be the default: enabled for device builds (faster deploy), and disabled for sim builds (faster build). It's never been optional for XI though).

rolfbjarne · 2018-02-19T06:46:17Z

tests/common/mac/ProjectTestHelpers.cs

 {
+	static class StringExtensions
+	{
+		internal static string [] SplitLines (this string s) => s.Split (new [] { Environment.NewLine }, StringSplitOptions.None);


Maybe tools/common/StringUtils.cs is a better place for this?

rolfbjarne · 2018-02-19T10:13:14Z

tools/mmp/driver.cs

+			if (ret != 0)
+				throw new MonoMacException (5311, true, "lipo failed with an error code '{0}'. Check build log for details.", ret);
+			if (name != "MonoPosixHelper")
+				ErrorHelper.Warning (2108, $"{name} was stripped of architecture {archToRemove} to comply with App Store restrictions. This could break exisiting codesigning signatures. Consider stripping the library with lipo or disabling with --native-strip=skip");


Won't this warning show up for all libraries, independent of their original architecture(s)? You don't seem to have any logic to detect which architecture(s) the libraries had before running lipo.

That said, mtouch also does the same thing, and has code to do this without running lipo: https://github.com/xamarin/xamarin-macios/blob/96085adb66129c42e9ae8e28f44d06112f9e1896/tools/common/MachO.cs#L194-L200

the only problem is that I don't think that code can handle slices of non-standard architectures like PPC (see bug #42464).

The "obvious" thing to do here is to first write a test that says "if I'm 64-bit and i have a 64-bit library there should be no warning".

And yes, it looks like that test would fail right now. :(

rolfbjarne · 2018-02-19T10:21:00Z

tools/mmp/driver.cs

+					default:
+						ErrorHelper.Error (26, $"Could not parse the command line argument '-native-strip:{v}'");
+						break;
+					}


We already have two interpretations of strip: mtouch takes the arguments nostrip (to not strip IL from managed assemblies) and nosymbolstrip (to not strip native symbols from the executable). Both of these makes sense (their command-line tools are named mono-cil-strip and strip, respectively), but for this case, the work "strip" doesn't even show up in the man page for lipo.

My suggestion: --trim-architectures.

I think this could make sense to include as an optimization (it even makes sense for XI as well, since we do the same thing there, the only difference would be the default: enabled for device builds (faster deploy), and disabled for sim builds (faster build). It's never been optional for XI though).

chamons · 2018-02-20T22:04:58Z

I've moved to --optimize=-trim-architectures instead of a new option and fixed the location of common string utils.

I'm going to work on fixing the likely every present warning w\ native libraries.

@rolfbjarne please review my optimization changes, first time I've added one.

chamons · 2018-02-22T17:28:02Z

build

monojenkins · 2018-02-22T20:29:14Z

Build failure

monojenkins · 2018-02-23T18:21:13Z

Build failure

chamons · 2018-02-23T19:30:44Z

I can reproduce them when run from the command line but not from the IDE, which is strange. In any case, looking.

… should have

…ly from mdb/pdb files copied - I am uncertain why this started failing, but a number of test changes might have triggered it. - In any case, the code was flat wrong before.

…libMonoPosixHelper.dylib will strip in Release

monojenkins · 2018-02-23T20:50:23Z

Build failure

chamons · 2018-02-23T21:05:50Z

This PR has started to turn into a monster, just like every other time I touch mmp...

A few more tests are failing locally after the last set of pushes. Looking now.

… to release/debug changes and output more failure info

chamons · 2018-02-23T21:23:44Z

Looks clean enough locally now. 🤞

monojenkins · 2018-02-23T21:25:21Z

Build failure

monojenkins · 2018-02-24T00:24:06Z

Build failure

chamons · 2018-02-26T15:11:07Z

Looks like some tests are comparing strings that changed when I added a message. Fixing.

monojenkins · 2018-02-26T21:42:04Z

Build failure

…tructure

monojenkins · 2018-02-28T23:37:45Z

Build failure

chamons · 2018-03-01T15:23:06Z

Some known test failures:

https://github.com/xamarin/maccore/issues/640
https://github.com/xamarin/maccore/issues/656
https://github.com/xamarin/maccore/issues/629

Looking into protocol failures, feel unrelated.

chamons · 2018-03-01T15:27:17Z

https://github.com/xamarin/maccore/issues/655 for the protocol failures.

…ictions (dotnet#3387) - dotnet#3367 - App Store will now fail builds if you add in a 32-bit dylib - If you are a 32-bit app you don't need the 64-bit part of your fat dylib anyway - Add --optimize=-trim-architectures to allow customization of behavior, as not everyone uses app store In addition, while writing tests for this is was noticed that mmp tests did not "really" run Release configuration correctly in most cases. Fixing this turned out to be a bit of a pain, but necessary to correctly test this (and other things). - Turns out that /p:configuration:debug is not sufficient to tell mmp to do the right thing - That, in most projects, sets the DebugSymbols property, which really is what is checked. - However, two of our projects did not have that, so we always did release mmp work. - Removed configuration property for tests and added real "Release" configuration option

…ictions (#3387) (#3633) - #3367 - App Store will now fail builds if you add in a 32-bit dylib - If you are a 32-bit app you don't need the 64-bit part of your fat dylib anyway - Add --optimize=-trim-architectures to allow customization of behavior, as not everyone uses app store In addition, while writing tests for this is was noticed that mmp tests did not "really" run Release configuration correctly in most cases. Fixing this turned out to be a bit of a pain, but necessary to correctly test this (and other things). - Turns out that /p:configuration:debug is not sufficient to tell mmp to do the right thing - That, in most projects, sets the DebugSymbols property, which really is what is checked. - However, two of our projects did not have that, so we always did release mmp work. - Removed configuration property for tests and added real "Release" configuration option

chamons added 4 commits February 2, 2018 14:34

Fix UnifiedExample.csproj release configuration and stale debug bits

e82c490

Remove stray sln

c0e50b2

spouliot reviewed Feb 4, 2018

View reviewed changes

rolfbjarne requested changes Feb 5, 2018

View reviewed changes

VincentDondain approved these changes Feb 5, 2018

View reviewed changes

dalexsoto approved these changes Feb 6, 2018

View reviewed changes

chamons added 8 commits February 16, 2018 09:32

Code review changes and improve test

3b62be8

Add test for framework striping to come

1901adc

Implement lipo of user frameworks

e5602d7

Update and document error codes

b1a93fb

Update tests for new warning code and test framework warns

83447e1

Merge branch 'master' into xm_32_lipo

caa3fc7

Fix merge issue

8cc0e46

Update to new warning due to merge

13f68e1

Fix merge issue with docs

32badd6

spouliot reviewed Feb 17, 2018

View reviewed changes

rolfbjarne requested changes Feb 19, 2018

View reviewed changes

chamons added 4 commits February 20, 2018 14:17

Move StringExtensions to more common location

0c533e3

Move option to optimization

bf503f6

Fix inline-isdirectbinding macOS text

556d722

Update error message for --optimize=-trim-architectures

fd579b4

Add tests showing broken behavior

f5ce194

Add removed TestCase attribute. 🤦

b03fd0f

chamons added 2 commits February 23, 2018 13:36

Fix classic tests by only defining Release\Debug for unified

198055b

Improve Unified_HelloWorld_ShouldHaveNoWarnings to test Release as it…

202e4df

… should have

chamons self-assigned this Feb 23, 2018

chamons added the do-not-merge Do not merge this pull request label Feb 23, 2018

chamons added 2 commits February 23, 2018 14:19

Fix Dontlink_Allow_ReadonlyAssembly by teaching mmp to remove read on…

5bc6278

…ly from mdb/pdb files copied - I am uncertain why this started failing, but a number of test changes might have triggered it. - In any case, the code was flat wrong before.

Teach ThirdPartyLibrary_WithCorrectBitness_ShouldNotStripOrWarn that …

3e956c7

…libMonoPosixHelper.dylib will strip in Release

Tweak UnifiedDebugBuilds_ShouldLinkToPartialStatic_UnlessDisabled due…

b8d839d

… to release/debug changes and output more failure info

chamons removed the do-not-merge Do not merge this pull request label Feb 23, 2018

Update mmp tests

d95aca8

Fix system mono test failures due to better release support in infras…

07ad537

…tructure

chamons merged commit ca43601 into dotnet:master Mar 1, 2018

chamons deleted the xm_32_lipo branch March 1, 2018 15:36

chamons mentioned this pull request Mar 1, 2018

[mmp] Add stripping of 32-bit dylibs to work with new App Store restr… #3633

Merged



		bool isStaticLib = real_src.EndsWith (".a", StringComparison.Ordinal);
		bool isDynamicLib = real_src.EndsWith (".dylib", StringComparison.Ordinal);

[mmp] Add stripping of 32-bit dylibs to work with new App Store restrictions #3387

[mmp] Add stripping of 32-bit dylibs to work with new App Store restrictions #3387

Uh oh!

Conversation

chamons commented Feb 2, 2018

Uh oh!

monojenkins commented Feb 2, 2018

Uh oh!

spouliot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monojenkins commented Feb 16, 2018

Uh oh!

chamons commented Feb 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

monojenkins commented Feb 16, 2018

Uh oh!

monojenkins commented Feb 17, 2018

Uh oh!

spouliot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chamons commented Feb 20, 2018

Uh oh!

chamons commented Feb 22, 2018

Uh oh!

monojenkins commented Feb 22, 2018

Uh oh!

monojenkins commented Feb 23, 2018

Uh oh!

chamons commented Feb 23, 2018

Uh oh!

monojenkins commented Feb 23, 2018

Uh oh!

chamons commented Feb 23, 2018

Uh oh!

chamons commented Feb 23, 2018

Uh oh!

monojenkins commented Feb 23, 2018

Uh oh!

monojenkins commented Feb 24, 2018

Uh oh!

chamons commented Feb 26, 2018

Uh oh!

monojenkins commented Feb 26, 2018

Uh oh!

monojenkins commented Feb 28, 2018

Uh oh!

chamons commented Mar 1, 2018

Uh oh!

chamons commented Mar 1, 2018

chamons commented Feb 16, 2018 •

edited

Loading