Skip to content

Conversation

@luhenry
Copy link
Contributor

@luhenry luhenry commented Apr 3, 2018

No description provided.

luhenry and others added 30 commits December 11, 2017 10:45
Context: #1975

In newer versions of Mono, `xabuild` is failing with:

    /Library/Frameworks/Mono.framework/Versions/Current/lib/mono/msbuild/15.0/bin/Microsoft.Common.CurrentVersion.targets(2126,5): error MSB3248:
        Parameter "AssemblyFiles" has invalid value "xamarin-android/bin/Debug/lib/xamarin.android/xbuild-frameworks/MonoAndroid/v1.0/mscorlib.dll".
        Could not load file or assembly 'System.Reflection.Metadata, Version=1.3.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' or one of its dependencies.

Or another example:

    Mono: The following assembly referenced from monodroid/external/xamarin-android/bin/Debug/bin/Microsoft.Build.Tasks.Core.dll could not be loaded:
        Assembly:   System.Reflection.Metadata    (assemblyref_index=7)
        Version:    1.3.0.0
        Public Key: b03f5f7f11d50a3a

Looking at copies of `System.Reflection.Metadata` in my Mono
installation:

    $ find /Library/Frameworks/Mono.framework/ | grep System.Reflection.Metadata.dll$
    /Library/Frameworks/Mono.framework//Versions/5.12.0/lib/mono/msbuild/15.0/bin/System.Reflection.Metadata.dll
    /Library/Frameworks/Mono.framework//Versions/5.12.0/lib/mono/msbuild/15.0/bin/Roslyn/System.Reflection.Metadata.dll
    /Library/Frameworks/Mono.framework//Versions/5.12.0/lib/mono/fsharp/System.Reflection.Metadata.dll
    /Library/Frameworks/Mono.framework//Versions/5.12.0/lib/mono/4.5/System.Reflection.Metadata.dll
    /Library/Frameworks/Mono.framework//Versions/5.12.0/lib/mono/4.5/dim/System.Reflection.Metadata.dll

It appears that the Mono version of MSBuild has its own copy of
`System.Reflection.Metadata.dll`. Referencing this MSBuild-specific
assembly from `xabuild.csproj` on non-Windows platforms appears to
solve the problem.
@luhenry luhenry requested a review from jonathanpeppers as a code owner July 19, 2018 17:13
Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

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

I think this is a simple change, and if it’s green we should go for it.

I’ll figure out something better if #1975 pans out.

@jonpryor
Copy link
Contributor

On the plus side, it built this time. (Yay!)

On the minus side, we have 26 unit test failures.

Progress!

@marek-safar
Copy link
Contributor

@jonp there are 0 test failures now

@marek-safar
Copy link
Contributor

build

2 similar comments
@marek-safar
Copy link
Contributor

build

@marek-safar
Copy link
Contributor

build

marek-safar and others added 5 commits July 27, 2018 00:42
Context: mono/mono#9868

`MonoAotOffsetsDumper` execution fails because
`$(AndroidSupportedTargetAotAbis)` doesn't contain the value `armeabi`.

`$(AndroidSupportedTargetAotAbis)` meanwhile comes from the
`$(ALL_JIT_ABIS)` make variable, which *does* contain `armeabi`:

	export ALL_JIT_ABIS  = \
		armeabi \
		armeabi-v7a \
		arm64-v8a \
		x86 \
		x86_64

Which raises a question: why isn't `armeabi` in
`$(AndroidSupportedTargetAotAbis)`?

Shot in the dark time!  Export the `$(ALL_JIT_ABIS)` variable so that
it's value will be dumped and visible in the MSBuild `.binlog` files
that are produced as part of the build and/or diag log output.

What's *really* odd about this is that the
xamarin/xamarin-android:mono-2018-04 branch hasn't otherwise touched
`build-tools/scripts`, and on the xamarin/xamarin-android:master
Jenkins build `armeabi` *is* in `$(AndroidSupportedTargetAotAbis)`,
e.g. https://jenkins.mono-project.com/view/Xamarin.Android/job/xamarin-android/1121/
Commit list for mono/mono:

* mono/mono@f3a2216b65a backport #9800 to 2018-04
* mono/mono@07ac0897350 Bump msbuild to track mono-2018-04 (#9834)
* mono/mono@d31dbe843a5 Apply F# portable pdb debug fix for pinvokes & bump (#9798)

Diff: mono/mono@a4ac628...f3a2216

<AndroidGenerateJniMarshalMethods Condition=" '$(AndroidGenerateJniMarshalMethods)' == '' ">False</AndroidGenerateJniMarshalMethods>

<MakeBundleKeepTemporaryFiles Condition=" '$(MakeBundleKeepTemporaryFiles)' == '' ">False</MakeBundleKeepTemporaryFiles>
Copy link
Contributor

Choose a reason for hiding this comment

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

This should either be $(_MakeBundleKeepTemporaryFiles) or $(AndroidMakeBundleKeepTemporaryFiles).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So < _MakeBundleKeepTemporaryFiles Condition=" '$(_MakeBundleKeepTemporaryFiles)' == '' ">False</_MakeBundleKeepTemporaryFiles > or <MakeBundleKeepTemporaryFiles Condition=" '$(_MakeBundleKeepTemporaryFiles)' == '' ">False</MakeBundleKeepTemporaryFiles>?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean that MSBuild properties that we use should either have a _ prefix (for "private" use) or should have an Android prefix (for "public" use), e.g.

<AndroidMakeBundleKeepTemporaryFiles Condition=" '$(AndroidMakeBundleKeepTemporaryFiles)' == '' ">False</AndroidMakeBundleKeepTemporaryFiles>

luhenry added 2 commits August 7, 2018 16:23
… the variable by Android since it seems like it should be public
@jonpryor jonpryor merged commit 90edf81 into master Aug 8, 2018
jonpryor pushed a commit that referenced this pull request Aug 13, 2018
Fixes: #1130
Fixes: #1561 (comment)
Fixes: #1845
Fixes: #1951

Context: https://bugzilla.xamarin.com/show_bug.cgi?id=10087
Context: https://bugzilla.xamarin.com/show_bug.cgi?id=11771
Context: https://bugzilla.xamarin.com/show_bug.cgi?id=12850
Context: https://bugzilla.xamarin.com/show_bug.cgi?id=18941
Context: https://bugzilla.xamarin.com/show_bug.cgi?id=19436
Context: https://bugzilla.xamarin.com/show_bug.cgi?id=25444
Context: https://bugzilla.xamarin.com/show_bug.cgi?id=33208
Context: https://bugzilla.xamarin.com/show_bug.cgi?id=58413
Context: https://bugzilla.xamarin.com/show_bug.cgi?id=59184
Context: https://bugzilla.xamarin.com/show_bug.cgi?id=59400
Context: https://bugzilla.xamarin.com/show_bug.cgi?id=59779
Context: https://bugzilla.xamarin.com/show_bug.cgi?id=60065
Context: https://bugzilla.xamarin.com/show_bug.cgi?id=60843
Context: mono/mono#6174
Context: mono/mono#6178
Context: mono/mono#6180
Context: mono/mono#6181
Context: mono/mono#6186
Context: mono/mono#6187
Context: mono/mono#6211
Context: mono/mono#6266
Context: mono/mono#6579
Context: mono/mono#6666
Context: mono/mono#6752
Context: mono/mono#6801
Context: mono/mono#6812
Context: mono/mono#6848
Context: mono/mono#6940
Context: mono/mono#6948
Context: mono/mono#6998
Context: mono/mono#6999
Context: mono/mono#7016
Context: mono/mono#7085
Context: mono/mono#7086
Context: mono/mono#7095
Context: mono/mono#7134
Context: mono/mono#7137
Context: mono/mono#7145
Context: mono/mono#7184
Context: mono/mono#7240
Context: mono/mono#7262
Context: mono/mono#7289
Context: mono/mono#7338
Context: mono/mono#7356
Context: mono/mono#7364
Context: mono/mono#7378
Context: mono/mono#7389
Context: mono/mono#7449
Context: mono/mono#7460
Context: mono/mono#7535
Context: mono/mono#7536
Context: mono/mono#7537
Context: mono/mono#7565
Context: mono/mono#7588
Context: mono/mono#7596
Context: mono/mono#7610
Context: mono/mono#7613
Context: mono/mono#7620
Context: mono/mono#7624
Context: mono/mono#7637
Context: mono/mono#7655
Context: mono/mono#7657
Context: mono/mono#7661
Context: mono/mono#7685
Context: mono/mono#7696
Context: mono/mono#7729
Context: mono/mono#7786
Context: mono/mono#7792
Context: mono/mono#7805
Context: mono/mono#7822
Context: mono/mono#7828
Context: mono/mono#7860
Context: mono/mono#7864
Context: mono/mono#7903
Context: mono/mono#7920
Context: mono/mono#8089
Context: mono/mono#8143
Context: mono/mono#8267
Context: mono/mono#8311
Context: mono/mono#8340
Context: mono/mono#8409
Context: mono/mono#8417
Context: mono/mono#8430
Context: mono/mono#8698
Context: mono/mono#8701
Context: mono/mono#8712
Context: mono/mono#8721
Context: mono/mono#8726
Context: mono/mono#8866
Context: mono/mono#9023
Context: mono/mono#9031
Context: mono/mono#9033
Context: mono/mono#9044
Context: mono/mono#9179
Context: mono/mono#9318
Context: mono/mono#9318
Context: https://github.com/xamarin/maccore/issues/628
Context: https://github.com/xamarin/maccore/issues/629
Context: https://github.com/xamarin/maccore/issues/673
jonpryor added a commit to xamarin/xamarin-android-api-compatibility that referenced this pull request Aug 14, 2018
Context: dotnet/android#1503
Context: https://jenkins.mono-project.com/view/Xamarin.Android/job/xamarin-android/1126/
Context: mono/mono@ca9cbdc

The [bump to mono:2018-04][0] introduced two "incompatible" changes
which impact our API Compatibility checks:

 1. `mono-api-html.exe` behavior has changed, and
 2. `System.String.Concat(object, object, object, object)` was
    removed.

`mono-api-html.exe` from mono:2018-02 and before had a bug wherein it
didn't properly handle nested types.  The immediate affect of this was
that the `Android.App.Notification.Icon` property change which was
handled/ignored in `inter-api-extra-v5.1-v6.0.txt` was seemingly
ignored, resulting in an API compatibility failure:

	<h1>### API BREAK BETWEEN v5.1 and v6.0</h1>
	<h3>Type Changed: Android.App.Notification.Action.Action</h3>
	<p>Modified properties:</p>
	<pre>
	<div data-is-breaking>  public <span class='removed removed-inline removed-breaking-inline'>int</span> <span class='added '>Android.Graphics.Drawables.Icon</span> Icon { get; <span class='removed removed-inline removed-breaking-inline'>set;</span>
 }
	</div></pre>

Note that the `mono-api-html` output here is still somewhat buggy:
*there is no* `Android.App.Notification.Action.Action` type -- nested
type names appear to be duplicated now, instead of skipped entirely --
so it's possible that future `mono-api-html` changes may break us.

Other errors were being reported as well [^0], all of which are
*reasonable and correct* but weren't needed with mono:2018-02's
`mono-api-html`, which is...worrying, but ¯\_(ツ)_/¯.

The `System.String.Concat(object, object, object, object)` removal is
a bit of a misnomer: *there is no* `String.Concat()` method which
takes four `object` parameters.  There *was* a `String.Concat()`
method which took *three* `object` parameters and an `__arglist`
parameter, and the `__arglist`-including overload was removed because
[it was deemed safe to remove][1]:

> @mark-safar: I let the change in because it believe it's safe to
> remove (I don't think our AOT compiler support that fully either).

[^0]: Brief summary of most "new" changes which `mono-api-html` from
      mono:2018-02 didn't need but are reported with mono:2018-04:

	<h1>### API BREAK BETWEEN v4.3 and v4.4</h1>
	<h3>Type Changed: Android.Media.RemoteControlClient.MetadataEditor</h3>
	<p>Modified base type:</p>
	<pre>
	<div data-is-breaking>  <span class='removed removed-inline removed-breaking-inline'>Java.Lang.Object</span> <span class='added '>Android.Media.MediaMetadataEditor</span>
	</div></pre>

	<h3>Type Changed: Android.Views.Surface.OutOfResourcesException</h3>
	<p>Modified base type:</p>
	<pre>
	<div data-is-breaking>  <span class='removed removed-inline removed-breaking-inline'>Java.Lang.Exception</span> <span class='added '>Java.Lang.RuntimeException</span>
	</div></pre>

	<h3>Type Changed: Java.Lang.ClassNotFoundException</h3>
	<p>Modified base type:</p>
	<pre>
	<div data-is-breaking>  <span class='removed removed-inline removed-breaking-inline'>Java.Lang.Exception</span> <span class='added '>Java.Lang.ReflectiveOperationException</span>
	</div></pre>

	<h3>Type Changed: Java.Lang.IllegalAccessException</h3>
	<p>Modified base type:</p>
	<pre>
	<div data-is-breaking>  <span class='removed removed-inline removed-breaking-inline'>Java.Lang.Exception</span> <span class='added '>Java.Lang.ReflectiveOperationException</span>
	</div></pre>

	<h3>Type Changed: Java.Lang.InstantiationException</h3>
	<p>Modified base type:</p>
	<pre>
	<div data-is-breaking>  <span class='removed removed-inline removed-breaking-inline'>Java.Lang.Exception</span> <span class='added '>Java.Lang.ReflectiveOperationException</span>
	</div></pre>

	<h3>Type Changed: Java.Lang.NoSuchFieldException</h3>
	<p>Modified base type:</p>
	<pre>
	<div data-is-breaking>  <span class='removed removed-inline removed-breaking-inline'>Java.Lang.Exception</span> <span class='added '>Java.Lang.ReflectiveOperationException</span>
	</div></pre>

	<h3>Type Changed: Java.Lang.NoSuchMethodException</h3>
	<p>Modified base type:</p>
	<pre>
	<div data-is-breaking>  <span class='removed removed-inline removed-breaking-inline'>Java.Lang.Exception</span> <span class='added '>Java.Lang.ReflectiveOperationException</span>
	</div></pre>

	<h3>Type Changed: Java.Lang.Reflect.InvocationTargetException</h3>
	<p>Modified base type:</p>
	<pre>
	<div data-is-breaking>  <span class='removed removed-inline removed-breaking-inline'>Java.Lang.Exception</span> <span class='added '>Java.Lang.ReflectiveOperationException</span>
	</div></pre>

	<h1>### API BREAK BETWEEN v4.4.87 and v5.0</h1>
	<h3>Type Changed: Android.OS.Bundle</h3>
	<p>Modified base type:</p>
	<pre>
	<div data-is-breaking>  <span class='removed removed-inline removed-breaking-inline'>Java.Lang.Object</span> <span class='added '>Android.OS.BaseBundle</span>
	</div></pre>

	<h1>### API BREAK BETWEEN v5.1 and v6.0</h1>
	<h3>Type Changed: Android.App.Notification.Action.Action</h3>
	<p>Modified properties:</p>
	<pre>
	<div data-is-breaking>  public <span class='removed removed-inline removed-breaking-inline'>int</span> <span class='added '>Android.Graphics.Drawables.Icon</span> Icon { get; <span class='removed removed-inline removed-breaking-inline'>set;</span> }
	</div></pre>

[0]: dotnet/android#1503
[1]: dotnet/android#2051 (comment)
jonpryor added a commit to xamarin/xamarin-android-api-compatibility that referenced this pull request Aug 15, 2018
Context: dotnet/android#1503
Context: https://jenkins.mono-project.com/view/Xamarin.Android/job/xamarin-android/1126/
Context: mono/mono@ca9cbdc

The [bump to mono:2018-04][0] introduced two "incompatible" changes
which impact our API Compatibility checks:

 1. `mono-api-html.exe` behavior has changed, and
 2. `System.String.Concat(object, object, object, object)` was
    removed.

`mono-api-html.exe` from mono:2018-02 and before had a bug wherein it
didn't properly handle nested types.  The immediate affect of this was
that the `Android.App.Notification.Icon` property change which was
handled/ignored in `inter-api-extra-v5.1-v6.0.txt` was seemingly
ignored, resulting in an API compatibility failure:

	<h1>### API BREAK BETWEEN v5.1 and v6.0</h1>
	<h3>Type Changed: Android.App.Notification.Action.Action</h3>
	<p>Modified properties:</p>
	<pre>
	<div data-is-breaking>  public <span class='removed removed-inline removed-breaking-inline'>int</span> <span class='added '>Android.Graphics.Drawables.Icon</span> Icon { get; <span class='removed removed-inline removed-breaking-inline'>set;</span>
 }
	</div></pre>

Note that the `mono-api-html` output here is still somewhat buggy:
*there is no* `Android.App.Notification.Action.Action` type -- nested
type names appear to be duplicated now, instead of skipped entirely --
so it's possible that future `mono-api-html` changes may break us.

Other errors were being reported as well [^0], all of which are
*reasonable and correct* but weren't needed with mono:2018-02's
`mono-api-html`, which is...worrying, but ¯\_(ツ)_/¯.

The `System.String.Concat(object, object, object, object)` removal is
a bit of a misnomer: *there is no* `String.Concat()` method which
takes four `object` parameters.  There *was* a `String.Concat()`
method which took *three* `object` parameters and an `__arglist`
parameter, and the `__arglist`-including overload was removed because
[it was deemed safe to remove][1]:

> @mark-safar: I let the change in because it believe it's safe to
> remove (I don't think our AOT compiler support that fully either).

[^0]: Brief summary of most "new" changes which `mono-api-html` from
      mono:2018-02 didn't need but are reported with mono:2018-04:

	<h1>### API BREAK BETWEEN v4.3 and v4.4</h1>
	<h3>Type Changed: Android.Media.RemoteControlClient.MetadataEditor</h3>
	<p>Modified base type:</p>
	<pre>
	<div data-is-breaking>  <span class='removed removed-inline removed-breaking-inline'>Java.Lang.Object</span> <span class='added '>Android.Media.MediaMetadataEditor</span>
	</div></pre>

	<h3>Type Changed: Android.Views.Surface.OutOfResourcesException</h3>
	<p>Modified base type:</p>
	<pre>
	<div data-is-breaking>  <span class='removed removed-inline removed-breaking-inline'>Java.Lang.Exception</span> <span class='added '>Java.Lang.RuntimeException</span>
	</div></pre>

	<h3>Type Changed: Java.Lang.ClassNotFoundException</h3>
	<p>Modified base type:</p>
	<pre>
	<div data-is-breaking>  <span class='removed removed-inline removed-breaking-inline'>Java.Lang.Exception</span> <span class='added '>Java.Lang.ReflectiveOperationException</span>
	</div></pre>

	<h3>Type Changed: Java.Lang.IllegalAccessException</h3>
	<p>Modified base type:</p>
	<pre>
	<div data-is-breaking>  <span class='removed removed-inline removed-breaking-inline'>Java.Lang.Exception</span> <span class='added '>Java.Lang.ReflectiveOperationException</span>
	</div></pre>

	<h3>Type Changed: Java.Lang.InstantiationException</h3>
	<p>Modified base type:</p>
	<pre>
	<div data-is-breaking>  <span class='removed removed-inline removed-breaking-inline'>Java.Lang.Exception</span> <span class='added '>Java.Lang.ReflectiveOperationException</span>
	</div></pre>

	<h3>Type Changed: Java.Lang.NoSuchFieldException</h3>
	<p>Modified base type:</p>
	<pre>
	<div data-is-breaking>  <span class='removed removed-inline removed-breaking-inline'>Java.Lang.Exception</span> <span class='added '>Java.Lang.ReflectiveOperationException</span>
	</div></pre>

	<h3>Type Changed: Java.Lang.NoSuchMethodException</h3>
	<p>Modified base type:</p>
	<pre>
	<div data-is-breaking>  <span class='removed removed-inline removed-breaking-inline'>Java.Lang.Exception</span> <span class='added '>Java.Lang.ReflectiveOperationException</span>
	</div></pre>

	<h3>Type Changed: Java.Lang.Reflect.InvocationTargetException</h3>
	<p>Modified base type:</p>
	<pre>
	<div data-is-breaking>  <span class='removed removed-inline removed-breaking-inline'>Java.Lang.Exception</span> <span class='added '>Java.Lang.ReflectiveOperationException</span>
	</div></pre>

	<h1>### API BREAK BETWEEN v4.4.87 and v5.0</h1>
	<h3>Type Changed: Android.OS.Bundle</h3>
	<p>Modified base type:</p>
	<pre>
	<div data-is-breaking>  <span class='removed removed-inline removed-breaking-inline'>Java.Lang.Object</span> <span class='added '>Android.OS.BaseBundle</span>
	</div></pre>

	<h1>### API BREAK BETWEEN v5.1 and v6.0</h1>
	<h3>Type Changed: Android.App.Notification.Action.Action</h3>
	<p>Modified properties:</p>
	<pre>
	<div data-is-breaking>  public <span class='removed removed-inline removed-breaking-inline'>int</span> <span class='added '>Android.Graphics.Drawables.Icon</span> Icon { get; <span class='removed removed-inline removed-breaking-inline'>set;</span> }
	</div></pre>

[0]: dotnet/android#1503
[1]: dotnet/android#2051 (comment)
@jonpryor jonpryor mentioned this pull request Oct 5, 2018
@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

full-mono-integration-build For PRs; run a full build (~6-10h for mono bumps), not the faster PR subset (~2h for mono bumps)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants