From 59cebb5c0cba7cf75cc31e962658b0c5b8bee1be Mon Sep 17 00:00:00 2001 From: Jonathan Pryor Date: Wed, 7 Jun 2017 08:17:10 -0400 Subject: [PATCH] [Xamarin.Android.Build.Tasks] Use @(AndroidDefineConstants) Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=56976 A "funny" thing happened in the migration from `xbuild` to `msbuild`: F# compilation behavior changed. This makes nearly no sense at all; it's the confluence of: 1. `fsc.exe` deosn't support specifying multiple symbols in a single `fsc.exe --define`. Specifically, *unlike* `csc`/`mcs`, the following command line does *not* define the symbols `A` and `B`; it is instead, effectively, *ignored*: fsc.exe "--define=A;B" 2. The `` task generated a `;`-separated string value. 3. `xbuild` differs from `msbuild` in `;`-splitting behavior. The `` task is used to determine additional conditional compilation symbols based on the API level. For example, if an app sets `$(TargetFrameworkVersion)`=v2.3 (API-10), then `` will create the MSBuild property `$(AndroidDefineConstants)` with the value: __XAMARIN_ANDROID_v1_0__;__MOBILE__;__ANDROID__;__ANDROID_1__;__ANDROID_2__;__ANDROID_3__;__ANDROID_4__;__ANDROID_5__;__ANDROID_6__;__ANDROID_7__;__ANDROID_8__;__ANDROID_9__;__ANDROID_10__ This is provided as a courtesy to developers so that they can easily conditionally include or exclude code based on the `$(TargetFrameworkVersion) value. When using `xbuild`, this works: the `$(AndroidDefineConstants)` property is split on the `;` to create an `ITaskItem[]`, which is provided to the `` tasks' `Fsc.DefineConstants` property. This in turn allows the `` task to generate a sequence of `--define` options, one per symbol: fsc.exe --define=__XAMARIN_ANDROID_v1_0__ --define=__MOBILE__ --define=__ANDROID__ ... Unfortunately, this behavior is an `xbuild` bug, in that `msbuild` *doesn't* behave this way. Instead, `msbuild` doesn't split the `$(AndroidDefineConstants)` property on `;` to create an `ITaskItem[]`; instead, it creates a single element which contains the `$(AndroidDefineConstants)` value as-is. This causes the `` task to generate a command-line like: fsc.exe "--define=__XAMARIN_ANDROID_v1_0__;__MOBILE__;__ANDROID__;..." ... If this were `csc`, that would be *fine*, but it's not `csc`, and it's not fine. The result is that code that requires that e.g. `__ANDROID__` be defined no longer works as expected, becuase `__ANDROID__` *isn't* defined when building with `msbuild`. The fix? If we need an `ITaskItem[]`, we should create one, not create a string and assume that `msbuild` will split on `;`. Change the `GetAndroidDefineConstants.AndroidDefineConstants` property from being a `string` into an `ITaskItem[]`, where each element of the array is a separate symbol which should be defined. This allows the desired values to be sent to the `Fsc.DefineConstants` property, allowing compilation to work as desired. --- .../Tasks/GetAndroidDefineConstants.cs | 19 +++++++++------ .../Xamarin.Android.Build.Tests/BuildTest.cs | 23 +++++++++++++++++++ .../Xamarin.Android.Common.targets | 8 +++---- 3 files changed, 39 insertions(+), 11 deletions(-) diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/GetAndroidDefineConstants.cs b/src/Xamarin.Android.Build.Tasks/Tasks/GetAndroidDefineConstants.cs index df1da09fcca..ffefb17da5a 100644 --- a/src/Xamarin.Android.Build.Tasks/Tasks/GetAndroidDefineConstants.cs +++ b/src/Xamarin.Android.Build.Tasks/Tasks/GetAndroidDefineConstants.cs @@ -1,6 +1,7 @@ // Copyright (C) 2011 Xamarin, Inc. All rights reserved. using System; +using System.Collections.Generic; using System.Text; using System.Text.RegularExpressions; using Microsoft.Build.Framework; @@ -16,21 +17,25 @@ public class GetAndroidDefineConstants : Task public string ProductVersion { get; set; } [Output] - public string AndroidDefineConstants { get; set; } + public ITaskItem[] AndroidDefineConstants { get; set; } public override bool Execute () { - var sb = new StringBuilder (); + var constants = new List (); if (!string.IsNullOrEmpty (ProductVersion)) { - sb.AppendFormat ("__XAMARIN_ANDROID_{0}__;", Regex.Replace (ProductVersion, "[^A-Za-z0-9]", "_")); + var version = Regex.Replace (ProductVersion, "[^A-Za-z0-9]", "_"); + constants.Add (new TaskItem ($"__XAMARIN_ANDROID_{version}__")); } - sb.Append ("__MOBILE__;__ANDROID__"); - for (int i = 1; i <= AndroidApiLevel; ++i) - sb.Append (";__ANDROID_").Append (i).Append ("__"); + constants.Add (new TaskItem ("__MOBILE__")); + constants.Add (new TaskItem ("__ANDROID__")); - AndroidDefineConstants = sb.ToString (); + for (int i = 1; i <= AndroidApiLevel; ++i) { + constants.Add (new TaskItem ($"__ANDROID_{i}__")); + } + + AndroidDefineConstants = constants.ToArray (); return true; } diff --git a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs index 323c3574dca..ea90ff3aa05 100644 --- a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs +++ b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs @@ -50,6 +50,29 @@ public void BuildBasicApplicationReleaseFSharp () } } + [Test] + public void FSharpAppHasAndroidDefine () + { + var proj = new XamarinAndroidApplicationProject () { + Language = XamarinAndroidProjectLanguage.FSharp, + }; + proj.Sources.Add (new BuildItem ("Compile", "IsAndroidDefined.fs") { + TextContent = () => @" +module Xamarin.Android.Tests +// conditional compilation; can we elicit a compile-time error? +let x = +#if __ANDROID__ + 42 +#endif // __ANDROID__ + +printf ""%d"" x +", + }); + using (var b = CreateApkBuilder ("temp/" + nameof (FSharpAppHasAndroidDefine))) { + Assert.IsTrue (b.Build (proj), "Build should have succeeded."); + } + } + [Test] public void BuildApplicationAndClean ([Values (false, true)] bool isRelease) { diff --git a/src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets b/src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets index 1c4d1b1d1ac..1bd6cc28834 100755 --- a/src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets +++ b/src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets @@ -822,12 +822,12 @@ because xbuild doesn't support framework reference assemblies. - + - - - + + $(DefineConstants);@(AndroidDefineConstants) +