Skip to content

Conversation

@jonpryor
Copy link
Contributor

@jonpryor jonpryor commented Jun 11, 2016

Fix the solution and project files so that msbuild may be used to
build the solution instead of requiring xbuild.

There were a few issues that msbuild didn't like:

  1. MSBuild doesn't like the "extra" configuration mappings in
    Xamarin.Android.sln.
  2. MSBuild doesn't like the presence of .dll within @(Reference)
    entries. <Reference Include="System.dll" /> is Bad™, so
    Don't Do That™.™.
  3. Turning $(AndroidSupportedAbis) into an item group is...broken.

(1) and (2) are straightforward fixes. (3) requires some explanation.

src/monodroid needs to only build libmonodroid.so for the
non-"host" ABIs within $(AndroidSupportedAbis). It needs this
restriction because non-selected ABIs may not be configured in
$(AndroidNdkDirectory), and thus can't be built.

This could be done by following
build-tools/mono-runtimes/mono-runtimes.projitems and doing lots of
Conditions on $(AndroidSupportedAbisForConditionalChecks):

<_MonoRuntime Include="armeabi-v7a" Condition="$(AndroidSupportedAbisForConditionalChecks.Contains(':armeabi-v7a:'))" />
...

However, that's kinda ugly when all we need is the ABI name, so
monodroid.projitems was "cute":

<PropertyGroup>
    <_SupportedAbis>$(AndroidSupportedAbis.Replace(':', ';'))</_SupportedAbis>
</PropertyGroup>
<ItemGroup>
    <_MonoRuntime Include="$(_SupportedAbis)" Exclude="@(HostOSName)" />
</ItemGroup>
<!-- @(_MonoRuntime) is `armeabi-v7a` by default -->

This works...on xbuild, but not msbuild. Doh!

(msbuild is "smart" and doesn't treat the ; as an item separator,
so if $(AndroidSupportedAbis) is host-Darwin;armeabi-v7a then
MSBuild treats the ; as part of the filename -- NOT a filename
separator -- and @(_MonoRuntime) contains one item with
an %(Identity) of host-Darwin;armeabi-v7a. On the one hand, this
is kinda awesome and answers the question "how can you have a filename
that contains ;?", but on the other hand it broke my project!)

The only fix I could think of was to use .Split(':'):

<_MonoRuntime Include="$(AndroidSupportedAbis.Split(':'))" Exclude="@(HostOSName)" />

That provides desired behavior with msbuild, but xbuild doesn't
support it and appears to either ignore it, or treat it literally,
in that @(_MonoRuntime) would contain a single item with the
literal value $(AndroidSupportedAbis.Split(':')) (argh!).

Fortunately, there's a "cute" workaround: using .Split() within an
item's Include attribute doesn't work, but using .Split() within a
property group declaration does work:

<PropertyGroup>
  <_SupportedAbis>$(AndroidSupportedAbis.Split(':'))</_SupportedAbis>
</PropertyGroup>
<ItemGroup>
  <_MonoRuntime Include="$(_SupportedAbis)" Exclude="@(HostOSName)" />
</ItemGroup>
<!-- @(_MonoRuntime) is `armeabi-v7a` by default -->

This implies that a property value isn't limited to string values, but
(as here) can be string arrays, which is interesting.


All that aside, while exploring the proper fix for (3) (it took a
remarkably long time to run across it), I decided to reconsider the
property and item arrangement here.

The prior approach was to have a single $(AndroidSupportedAbis)
MSBuild property which controlled both Android target ABIs and host
ABIs. This worked...but wasn't entirely scalable (separate moving
parts need to be kept in sync). Additionally, we need to add AOT
cross-compiler support, which logically would be controlled by the
same/similar mechanism, so a value of "build everything" would start
to look insane:

msbuild /p:AndroidSupportedAbis=armeabi:armeabi-v7a:arm64-v8a:x86:x86_64:host-Darwin:host-Win64:cross-Darwin-arm:cross-Darwin-arm64:cross-Darwin-x86:cross-Darwin-x86_64:cross-Win64-arm:cross-Win64-arm64:cross-Win64-x86:cross-Win64-x86_64

And that's assuming I'm not missing anything, or that we don't add
MIPS support in the future, or...

Blech.

Furthermore, Xamarin.Android already uses
$(AndroidSupportedAbis) in its build system, which means a
top-level override of $(AndroidSupportedAbis) would also impact all
projects which build .apk files, e.g.
src/Mono.Android/Test/Mono.Android-Tests.csproj, which might not be
desirable.

In short, I think we're overloading "Android supported ABIs," and it
should be split up into smaller, easier to rationalize, chunks.

Thus, leave $(AndroidSupportedAbis) to Xamarin.Android's tasks, and
replace it with two new properties:

  • $(AndroidSupportedHostJitAbis): The "host" ABIs to build.
  • $(AndroidSupportedTargetJitAbis): The "target" ABIs to build.

AOT support, when added, would use a new
$(AndroidSupportedHostAotAbis) property, thus keeping the set of
acceptable values small and more easily rationalizable.

Finally, "split up" these new Abis properties into corresponding Abi
item groups, to allow consistent and reusable "mapping" of ABI names
to filesystem locations, etc. The new @(AndroidSupportedHostAotAbi)
and @(AndroidSupportedTargetJitAbi) item groups are derived from
their corresponding values. (Note singular from plural in naming.)

<CFiles Include="jni\**\*.c" />
</ItemGroup>
<Target Name="_GetRuntimes">
<ValueToItems
Copy link
Member

Choose a reason for hiding this comment

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

Should it be SplitScalarIntoItems or ScalarToItems? Though, maybe Value also kinda says the same thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like Value. :-)

@jonpryor jonpryor force-pushed the jonp-msbuild branch 3 times, most recently from c8cc3a1 to 6ef690e Compare June 13, 2016 16:21
[Required]
public string Separator { get; set; }

public ITaskItem[] Exclude { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

You can use a string[] type here, since you don't need the metadata from the items.

@radical
Copy link
Member

radical commented Jun 13, 2016

Other than the two comments, LGTM.

Fix the solution and project files so that `msbuild` may be used to
build the solution instead of requiring `xbuild`.

There were a few issues that `msbuild` didn't like:

1. MSBuild doesn't like the "extra" configuration mappings in
    Xamarin.Android.sln.

2. MSBuild doesn't like the presence of `.dll` within `@(Reference)`
    entries. `<Reference Include="System.dll" />` is Bad™, so
    Don't Do That™.™.

3. Turning `$(AndroidSupportedAbis)` into an item group is...broken.

(1) and (2) are straightforward fixes. (3) requires some explanation.

`src/monodroid` needs to *only* build `libmonodroid.so` for the
non-"host" ABIs within `$(AndroidSupportedAbis)`. It needs this
restriction because non-selected ABIs may not be configured in
`$(AndroidNdkDirectory)`, and thus can't be built.

This *could* be done by following
`build-tools/mono-runtimes/mono-runtimes.projitems` and doing lots of
`Condition`s on `$(AndroidSupportedAbisForConditionalChecks)`:

	<_MonoRuntime Include="armeabi-v7a" Condition="$(AndroidSupportedAbisForConditionalChecks.Contains(':armeabi-v7a:'))" />
	...

However, that's kinda ugly when *all* we need is the ABI name, so
`monodroid.projitems` was "cute":

	<PropertyGroup>
	  <_SupportedAbis>$(AndroidSupportedAbis.Replace(':', ';'))</_SupportedAbis>
	</PropertyGroup>
	<ItemGroup>
	  <_MonoRuntime Include="$(_SupportedAbis)" Exclude="@(HostOSName)" />
	</ItemGroup>
	<!-- @(_MonoRuntime) is `armeabi-v7a` by default -->

This works...on xbuild, but *not* `msbuild`. Doh!

(`msbuild` is "smart" and doesn't treat the `;` as an item separator,
so if `$(AndroidSupportedAbis)` is `host-Darwin;armeabi-v7a` then
MSBuild treats the `;` as part of the filename -- NOT a filename
separator -- and `@(_MonoRuntime)` contains *one* item with
an `%(Identity)` of `host-Darwin;armeabi-v7a`. On the one hand, this
is kinda awesome and answers the question "how can you have a filename
that contains `;`?", but on the other hand it broke my project!)

The only fix I could think of was to use `.Split(':')`:

	<_MonoRuntime Include="$(AndroidSupportedAbis.Split(':'))" Exclude="@(HostOSName)" />

That provides desired behavior with `msbuild`, but `xbuild` doesn't
support it and appears to either *ignore* it, or treat it literally,
in that `@(_MonoRuntime)` would contain a *single* item with the
literal value `$(AndroidSupportedAbis.Split(':'))` (argh!).

Fortunately, there's a "cute" workaround: using `.Split()` within an
item's `Include` attribute doesn't work, but using `.Split()` within a
property group declaration *does* work:

	<PropertyGroup>
	  <_SupportedAbis>$(AndroidSupportedAbis.Split(':'))</_SupportedAbis>
	</PropertyGroup>
	<ItemGroup>
	  <_MonoRuntime Include="$(_SupportedAbis)" Exclude="@(HostOSName)" />
	</ItemGroup>
	<!-- @(_MonoRuntime) is `armeabi-v7a` by default -->

This implies that a property value isn't limited to string values, but
(as here) can be string *arrays*, which is interesting.

~~~

All that aside, while exploring the proper fix for (3) (it took a
remarkably long time to run across it), I decided to reconsider the
property and item arrangement here.

The prior approach was to have a single `$(AndroidSupportedAbis)`
MSBuild property which controlled *both* Android target ABIs and host
ABIs. This worked...but wasn't entirely scalable (separate moving
parts need to be kept in sync). Additionally, we need to add AOT
cross-compiler support, which logically would be controlled by the
same/similar mechanism, so a value of "build everything" would start
to look insane:

	msbuild /p:AndroidSupportedAbis=armeabi:armeabi-v7a:arm64-v8a:x86:x86_64:host-Darwin:host-Win64:cross-Darwin-arm:cross-Darwin-arm64:cross-Darwin-x86:cross-Darwin-x86_64:cross-Win64-arm:cross-Win64-arm64:cross-Win64-x86:cross-Win64-x86_64

And that's assuming I'm not missing anything, or that we don't add
MIPS support in the future, or...

Blech.

Furthermore, Xamarin.Android *already* uses
[`$(AndroidSupportedAbis)` in its build system][0], which means a
top-level override of `$(AndroidSupportedAbis)` would also impact all
projects which build `.apk` files, e.g.
`src/Mono.Android/Test/Mono.Android-Tests.csproj`, which might not be
desirable.

In short, I think we're overloading "Android supported ABIs," and it
should be split up into smaller, easier to rationalize, chunks.

Thus, leave `$(AndroidSupportedAbis)` to Xamarin.Android's tasks, and
replace it with *two* new properties:

* `$(AndroidSupportedHostJitAbis)`: The "host" ABIs to build.
* `$(AndroidSupportedTargetJitAbis)`: The "target" ABIs to build.

AOT support, when added, would use a new
`$(AndroidSupportedHostAotAbis)` property, thus keeping the set of
acceptable values small and more easily rationalizable.

Finally, "split up" these new Abis properties into corresponding Abi
item groups, to allow consistent and reusable "mapping" of ABI names
to filesystem locations, etc. The new `@(AndroidSupportedHostAotAbi)`
and `@(AndroidSupportedTargetJitAbi)` item groups are derived from
their corresponding values. (Note singular from plural in naming.)

[0]: https://developer.xamarin.com/guides/android/under_the_hood/build_process/#AndroidSupportedAbis
@grendello grendello merged commit 6bb1016 into dotnet:master Jun 15, 2016
radical pushed a commit that referenced this pull request May 8, 2018
Before this fix, it failed to build due to bogus reference resolution in
nuget somehow:

```
Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs(99,16): error CS0012: The type `Mono.Cecil.Cil.SequencePoint' is defined in an assembly that is not referenced. Consider adding a reference to assembly `Xamarin.Android.Cecil, Version=0.9.6.0, Culture=neutral, PublicKeyToken=0738eb9f132ed756'
				/sources/monodroid/monodroid/out/lib/mandroid//Java.Interop.Tools.Diagnostics.dll (Location of the symbol related to previous error)
			Task "Csc" execution -- FAILED
			Done building target "CoreCompile" in project "/sources/monodroid/Java.Interop/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil.csproj".-- FAILED
		Done building project "/sources/monodroid/Java.Interop/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil.csproj".-- FAILED
	Task "MSBuild" execution -- FAILED
	Done building target "ResolveProjectReferences" in project "/sources/monodroid/Java.Interop/tools/generator/generator.csproj".-- FAILED
Done building project "/sources/monodroid/Java.Interop/tools/generator/generator.csproj".-- FAILED
```

I have added explicit $(MSBuildThisFileDirectory) to fix the issue.
@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.

4 participants