[mtouch/mmp] Share numerous command-line arguments. #8599

rolfbjarne · 2020-05-14T08:49:40Z

Share --targetver, and deprecate mmp's --minos.
Share --reference, and deprecate -r|--ref and -a|--assembly.
Share --debug.
Share --sdk.
Share --warn-on-type-ref.
Share --registrar and --runregistrar.
Share --xml.
Share --i18n.
Share --nolink, --linksdkonly, --linkplatform and --linkskip.
Share --cache.
Share --force.
Share --tls-provider and --http-message-handler.

This PR is probably easiest to reviewed commit-by-commit.

…skip.

…bly.

monojenkins · 2020-05-14T10:41:06Z

Build failure
✅ Provisioning succeeded
✅ Build succeeded
✅ API Diff (from stable)
✅ API Diff (from PR only) (no change)
ℹ️ Generator Diff (please review changes)
🔥 Test run failed 🔥

Test results

1 tests failed, 88 tests passed.

Failed tests

MSBuild tests/iOS: Failed (Execution failed with exit code 1)

spouliot · 2020-05-14T12:37:56Z

tools/common/Driver.cs

+#endif
+#if MMP
+			options.Add ("a|assembly=", "Add an assembly to be processed [DEPRECATED, use --reference instead]", v => app.References.Add (v), true);
+#endif


^ should not those remain the the tool-specific files to avoid #if?

I guess it is fine to keep together and maybe be able to remove/hide in the future

@spouliot Yes, the idea is to keep the related code close, but I'm fine either way.

I've moved this code back into mtouch.cs/driver.cs

spouliot · 2020-05-14T12:39:04Z

tools/common/Driver.cs

+				} catch (Exception ex) {
+					throw ErrorHelper.CreateError (26, ex, Errors.MX0026, $"minos:{v}", ex.Message);
+				}
+			}, true);


same, why not keep it inside mmp itself ?

stephen-hawley

This is minor: most of the newly added options need punctuation to be consistent with the existing ones.

stephen-hawley · 2020-05-14T15:16:57Z

tools/common/Driver.cs

 			options.Add ("version", "Output version information and exit.", v => a = Action.Version);
 			options.Add ("v|verbose", "Specify how verbose the output should be. This can be passed multiple times to increase the verbosity.", v => Verbosity++);
 			options.Add ("q|quiet", "Specify how quiet the output should be. This can be passed multiple times to increase the silence.", v => Verbosity--);
+			options.Add ("debug:", "Build a debug app. If AOT-compiling, will also generate native debug code for the specified assembly (set to 'all' to generate debug code for all assemblies, the default is to generate debug code for user assemblies only)", v => {


nit: the last sentence needs a . at the end.

@stephen-hawley fixed

stephen-hawley · 2020-05-14T15:18:07Z

tools/common/Driver.cs

 			options.Add ("sdkroot=", "Specify the location of Apple SDKs, default to 'xcode-select' value.", v => sdk_root = v);
+			options.Add ("sdk=", "Specifies the SDK version to compile against (version, for example \"10.9\")", v => {


needs a . at the end.

@stephen-hawley fixed

stephen-hawley · 2020-05-14T15:18:38Z

tools/common/Driver.cs

 			options.Add ("target-framework=", "Specify target framework to use. Currently supported: '" + string.Join ("', '", TargetFramework.ValidFrameworks.Select ((v) => v.ToString ())) + "'.", v => SetTargetFramework (v));
 #if MMP
 			options.Add ("abi=", "Comma-separated list of ABIs to target. x86_64", v => app.ParseAbi (v));
 #else
 			options.Add ("abi=", "Comma-separated list of ABIs to target. Currently supported: armv7, armv7+llvm, armv7+llvm+thumb2, armv7s, armv7s+llvm, armv7s+llvm+thumb2, arm64, arm64+llvm, arm64_32, arm64_32+llvm, i386, x86_64", v => app.ParseAbi (v));
 #endif
 			options.Add ("no-xcode-version-check", "Ignores the Xcode version check.", v => { min_xcode_version = null; }, true /* This is a non-documented option. Please discuss any customers running into the xcode version check on the maciosdev@ list before giving this option out to customers. */);
+			options.Add ("nolink", "Do not link the assemblies", v => app.LinkMode = LinkMode.None);


@stephen-hawley fixed

stephen-hawley · 2020-05-14T15:20:41Z

tools/common/Driver.cs

+					app.Registrar = RegistrarMode.PartialStatic;
+					break;
+				case "il":
+					app.Registrar = RegistrarMode.Dynamic;


Is il really an exact alias for dynamic? Does it need to be deprecated?

building on that comment, we can move the case next to the dynamic one.

I just removed the il option.

mandel-macaque · 2020-05-14T16:03:16Z

tools/common/Driver.cs

+			options.Add ("registrar:", "Specify the registrar to use (dynamic, static or default (dynamic in the simulator, static on device))", v => {
+				var split = v.Split ('=');
+				var name = split [0];
+				var value = split.Length > 1 ? split [1] : string.Empty;


Do we trust people not doing 'stupid' things like Static etc..?

They'll just get an error saying to use static instead.

mandel-macaque · 2020-05-14T16:04:02Z

tools/common/Driver.cs

+					app.Registrar = RegistrarMode.PartialStatic;
+					break;
+				case "il":
+					app.Registrar = RegistrarMode.Dynamic;


building on that comment, we can move the case next to the dynamic one.

monojenkins · 2020-05-14T17:17:07Z

Build success
✅ Provisioning succeeded
✅ Build succeeded
✅ API Diff (from stable)
✅ API Diff (from PR only) (no change)
ℹ️ Generator Diff (please review changes)
✅ Test run succeeded

The IL generator was what MonoMac had before the dynamic/static registrar code got shared between MonoTouch and MonoMac. The IL registrar been gone for years, and as far as I know nobody ever used --registrar:il, even though it was provided as a compatibility option in the beginning (we still had the IL registrar around for a while after adding the static+dynamic registrars, until it was completely replaced by the dynamic registrar). So just remove this option, if anyone ever used it they can replace it with --registrar:dynamic.

monojenkins · 2020-05-18T12:37:16Z

Build success
✅ Provisioning succeeded
✅ Build succeeded
✅ API Diff (from stable)
✅ API Diff (from PR only) (no change)
ℹ️ Generator Diff (please review changes)
✅ Test run succeeded

rolfbjarne added 13 commits May 14, 2020 10:46

[mtouch/mmp] Share Application.IsDualBuild, Is32Build and Is64Build.

e6df397

[mtouch/mmp] Share --tls-provider and --http-message-handler.

ff4cbed

[mtouch/mmp] Share --force.

40dbdee

[mtouch/mmp] Share --cache.

b973e72

[mtouch/mmp] Share --nolink, --linksdkonly, --linkplatform and --link…

8abefdb

…skip.

[mtouch/mmp] Share --i18n.

8e3eece

[mtouch/mmp] Share --xml.

8f177f2

[mtouch/mmp] Share --registrar and --runregistrar.

fa99984

[mtouch/mmp] Share --warn-on-type-ref.

8c03eca

[mtouch/mmp] Share --sdk.

4224dbc

[mtouch/mmp] Share --debug.

9c01da2

[mtouch/mmp] Share --reference, and deprecate -r|--ref and -a|--assem…

5920154

…bly.

[mtouch/mmp] Share --targetver, and deprecate mmp's --minos.

ebc16c3

rolfbjarne requested review from chamons, emaf, jstedfast and spouliot as code owners May 14, 2020 08:49

rolfbjarne added the not-notes-worthy Ignore for release notes label May 14, 2020

[msbuild] Adjust tests after switching to use --reference instead of -r.

c58634a

spouliot reviewed May 14, 2020

View reviewed changes

spouliot approved these changes May 14, 2020

View reviewed changes

dalexsoto approved these changes May 14, 2020

View reviewed changes

stephen-hawley requested changes May 14, 2020

View reviewed changes

mandel-macaque reviewed May 14, 2020

View reviewed changes

mandel-macaque approved these changes May 14, 2020

View reviewed changes

rolfbjarne added 2 commits May 18, 2020 09:07

Update according to review.

c98e582

rolfbjarne added 2 commits May 18, 2020 09:24

Merge remote-tracking branch 'origin/master' into mtouch-mmp-share

18d0472

[mtouch/mmp] Keep bundler-specific code in its corresponding file.

f025f61

mandel-macaque approved these changes May 18, 2020

View reviewed changes

stephen-hawley approved these changes May 19, 2020

View reviewed changes

rolfbjarne merged commit 392200d into xamarin:master May 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mtouch/mmp] Share numerous command-line arguments. #8599

[mtouch/mmp] Share numerous command-line arguments. #8599

rolfbjarne commented May 14, 2020 •

edited

Loading

monojenkins commented May 14, 2020

Failed tests

spouliot May 14, 2020

dalexsoto May 14, 2020

rolfbjarne May 14, 2020

rolfbjarne May 18, 2020

spouliot May 14, 2020

stephen-hawley left a comment

stephen-hawley May 14, 2020

rolfbjarne May 18, 2020

stephen-hawley May 14, 2020

rolfbjarne May 18, 2020

stephen-hawley May 14, 2020

rolfbjarne May 18, 2020

stephen-hawley May 14, 2020

mandel-macaque May 14, 2020

rolfbjarne May 18, 2020

stephen-hawley May 19, 2020

mandel-macaque May 14, 2020

rolfbjarne May 18, 2020

mandel-macaque May 14, 2020

monojenkins commented May 14, 2020

monojenkins commented May 18, 2020

		options.Add ("sdkroot=", "Specify the location of Apple SDKs, default to 'xcode-select' value.", v => sdk_root = v);
		options.Add ("sdk=", "Specifies the SDK version to compile against (version, for example \"10.9\")", v => {

[mtouch/mmp] Share numerous command-line arguments. #8599

[mtouch/mmp] Share numerous command-line arguments. #8599

Conversation

rolfbjarne commented May 14, 2020 • edited Loading

monojenkins commented May 14, 2020

Test results

Failed tests

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

stephen-hawley 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

Choose a reason for hiding this comment

monojenkins commented May 14, 2020

monojenkins commented May 18, 2020

rolfbjarne commented May 14, 2020 •

edited

Loading