Skip to content

[Xamarin.Android.Build.Tasks] Use @(AndroidDefineConstants) - #635

Merged
dellis1972 merged 1 commit into
dotnet:masterfrom
jonpryor:jonp-itemgroup-defines
Jun 7, 2017
Merged

[Xamarin.Android.Build.Tasks] Use @(AndroidDefineConstants)#635
dellis1972 merged 1 commit into
dotnet:masterfrom
jonpryor:jonp-itemgroup-defines

Conversation

@jonpryor

@jonpryor jonpryor commented Jun 7, 2017

Copy link
Copy Markdown
Contributor

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 <GetAndroidDefineConstants/> task generated a ;-separated
    string value.

  3. xbuild differs from msbuild in ;-splitting behavior.

The <GetAndroidDefineConstants/> 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 <GetAndroidDefineConstants/> 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 <Fsc/> tasks' Fsc.DefineConstants property. This
in turn allows the <Fsc/> 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 <Fsc/> 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.

@jonpryor

jonpryor commented Jun 7, 2017

Copy link
Copy Markdown
Contributor Author

Aside: while this adds a test, the test is effectively meaningless, because the test framework uses xbuild, not msbuild, and building under xbuild works as expected.

@dellis1972: Is there some way to force building with msbuild? Or should we just deal with that when we migrate the entire build system to actually use msbuild for unit test execution on the bots?

Additionally, it looks like the $USE_MSBUILD environment variable was intended to be used as a way to specify that msbuild should be used, but I don't see how that actually works; see Builder. GetXbuildPath(), which appears to be dead code.

@jonpryor
jonpryor force-pushed the jonp-itemgroup-defines branch from 4d0d3b8 to 13a08e5 Compare June 7, 2017 12:39
@dellis1972

Copy link
Copy Markdown
Contributor

@jonpryor there is an environment variable we can set to force the use of msbuild

USE_MSBUILD=1

public override bool Execute ()
{
var sb = new StringBuilder ();
var constants = new List<ITaskItem>();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

formatting :P

",
});
using (var b = CreateApkBuilder ("temp/" + nameof (FSharpAppHasAndroidDefine))) {
Assert.IsTrue (b.Build (proj), "Build should have succeeded.");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we remove this ? The output will be emitted automatically if it fails.

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 `<GetAndroidDefineConstants/>` task generated a `;`-separated
    string value.

 3. `xbuild` differs from `msbuild` in `;`-splitting behavior.

The `<GetAndroidDefineConstants/>` 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 `<GetAndroidDefineConstants/>` 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 `<Fsc/>` tasks' `Fsc.DefineConstants` property. This
in turn allows the `<Fsc/>` 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 `<Fsc/>` 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.
@jonpryor
jonpryor force-pushed the jonp-itemgroup-defines branch from 13a08e5 to 59cebb5 Compare June 7, 2017 13:30
@dellis1972
dellis1972 merged commit 669d515 into dotnet:master Jun 7, 2017
jonpryor added a commit that referenced this pull request Jun 7, 2017
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 `<GetAndroidDefineConstants/>` task generated a `;`-separated
    string value.

 3. `xbuild` differs from `msbuild` in `;`-splitting behavior.

The `<GetAndroidDefineConstants/>` 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 `<GetAndroidDefineConstants/>` 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 `<Fsc/>` tasks' `Fsc.DefineConstants` property. This
in turn allows the `<Fsc/>` 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 `<Fsc/>` 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.
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request May 1, 2020
Changes: dotnet/java-interop@ce8dc40...377c4c7

Fixes: dotnet/java-interop#631

  * dotnet/java-interop@377c4c7: Bump to xamarin/xamarin-android-tools/master@f5fcb9fd (dotnet#637)
  * dotnet/java-interop@857b9a9: [Java.Interop.Export] Begin supporting methods with 14+ params (dotnet#635)
  * dotnet/java-interop@9876e31: [Java.Interop.BootstrapTasks] Convert to SDK style project (dotnet#609)
  * dotnet/java-interop@cbb50af: [generator-Tests] Use Roslyn for .NET Core Support (dotnet#638)
  * dotnet/java-interop@56955d9: [generator] Use custom delegates instead of Func/Action. (dotnet#632)
  * dotnet/java-interop@0a77166: [Java.Interop.sln] Commit updated automatically generated guids. (dotnet#634)
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request May 1, 2020
Changes: dotnet/java-interop@ce8dc40...377c4c7

Fixes: dotnet/java-interop#631

  * dotnet/java-interop@377c4c7: Bump to xamarin/xamarin-android-tools/master@f5fcb9fd (dotnet#637)
  * dotnet/java-interop@857b9a9: [Java.Interop.Export] Begin supporting methods with 14+ params (dotnet#635)
  * dotnet/java-interop@9876e31: [Java.Interop.BootstrapTasks] Convert to SDK style project (dotnet#609)
  * dotnet/java-interop@cbb50af: [generator-Tests] Use Roslyn for .NET Core Support (dotnet#638)
  * dotnet/java-interop@56955d9: [generator] Use custom delegates instead of Func/Action. (dotnet#632)
  * dotnet/java-interop@0a77166: [Java.Interop.sln] Commit updated automatically generated guids. (dotnet#634)
jonpryor added a commit that referenced this pull request May 2, 2020
Changes: dotnet/java-interop@ce8dc40...377c4c7

Fixes: dotnet/java-interop#631

  * dotnet/java-interop@377c4c7: Bump to xamarin/xamarin-android-tools/master@f5fcb9fd (#637)
  * dotnet/java-interop@857b9a9: [Java.Interop.Export] Begin supporting methods with 14+ params (#635)
  * dotnet/java-interop@9876e31: [Java.Interop.BootstrapTasks] Convert to SDK style project (#609)
  * dotnet/java-interop@cbb50af: [generator-Tests] Use Roslyn for .NET Core Support (#638)
  * dotnet/java-interop@56955d9: [generator] Use custom delegates instead of Func/Action. (#632)
  * dotnet/java-interop@0a77166: [Java.Interop.sln] Commit updated automatically generated guids. (#634)
jonpryor added a commit that referenced this pull request May 6, 2020
Changes: dotnet/java-interop@6608c59...e599781

Fixes: dotnet/java-interop#631

  * dotnet/java-interop@e599781: Bump to xamarin/xamarin-android-tools/master@f5fcb9fd (#637)
  * dotnet/java-interop@b7aec95: [Java.Interop.Export] Begin supporting methods with 14+ params (#635)
  * dotnet/java-interop@eb6202b: [Java.Interop.BootstrapTasks] Convert to SDK style project (#609)
  * dotnet/java-interop@e078618: [generator-Tests] Use Roslyn for .NET Core Support (#638)
  * dotnet/java-interop@8e5310b: [generator] Use custom delegates instead of Func/Action. (#632)
  * dotnet/java-interop@f20f853: [Java.Interop.sln] Commit updated automatically generated guids. (#634)
@github-actions github-actions Bot locked and limited conversation to collaborators Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants