Update cecil for parity with mono/2017-02 #128
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi @xmcclure, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! The agreement was validated by .NET Foundation and real humans are currently evaluating your PR. TTYL, DNFBOT; |
jonpryor
force-pushed
the
mono-2017-02
branch
4 times, most recently
from
7659d00 to
37b0f20
Compare
|
Wonderful. The unit test failure appears to be due to using Roslyn as the compiler, in that the test failures doesn't happen when using mcs... This will take a bit longer to track down than the compilation and gendarme errors... |
xmcclure
changed the title
Update cecil for parity with mono/2017-02
Bumping cecil requires updating `assembly-rename.patch` so that it
will cleanly apply, and fixing `DirectoryAssemblyResolver.cs` so that
`AssemblyResolverCoda.GetAssembly()` will compile.
Then there's *all the other changes* in this patch, which are
*unrelated* to the Cecil bump, but instead related to an
"environmental" change on the PR builder:
The xamarin-android repo [now supports auto-provisioning][0]
dependencies, allowing Mono to be upgraded as part of the build
preparation process.
One of the xamarin-android PRs makes use of this, to install
Mono 4.9 (to test building under/with Mono 4.9/Roslyn/etc.).
The xamarin-android PR builder is the same machine as the Java.Interop
PR builder.
Meaning Java.Interop PRs may -- or may not, depending on global
machine state -- be building with Mono 4.9.
As It Turns Out™, building under Roslyn "breaks" things. :-D
Gendarme now flags additional errors due to Roslyn IL differences,
e.g. additional closure members which are apparently always null, and
other IL differences.
Then there's the larger changes
`src/Java.Interop/Tests/Java.Interop/TestType.cs`: these are needed
because using Roslyn *broke our unit tests*:
System.ArgumentException : targetType must implement IJavaPeerable!
at Java.Interop.JniRuntime+JniValueManager.CreatePeer (Java.Interop.JniObjectReference& reference, Java.Interop.JniObjectReferenceOptions transfer, System.Type targetType)
at Java.Interop.ProxyValueMarshaler.CreateGenericValue (Java.Interop.JniObjectReference& reference, Java.Interop.JniObjectReferenceOptions options, System.Type targetType)
at Java.Interop.JniValueMarshaler`1[T].CreateValue (Java.Interop.JniObjectReference& reference, Java.Interop.JniObjectReferenceOptions options, System.Type targetType)
at Java.Interop.JniValueMarshaler.CreateValue (System.IntPtr handle, System.Type targetType)
at (wrapper dynamic-method) System.Object:lambda_method (System.Runtime.CompilerServices.Closure,intptr,intptr)
Unfortunately, the `ArgumentException` doesn't provide much context,
so -- relatedly -- update the error message so that it provides the
type that doesn't implement `IJavaPeerable`:
targetType `Java.InteropTests.TestType+<>c, Java.Interop-Tests, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null` must implement IJavaPeerable!
This added context introduces a new question: WTF is `TestType.<>c`?!
Funny story! Roslyn and `mcs` differ in how they implement lambda
methods: `mcs` adds the lambda method into the enclosing class. Roslyn
adds it to a *nested* `<>c` class:
// C# Source:
class Example {
public static void Main ()
{
Action a = () => {};
}
}
// mcs output equivalent
class Example {
static void <Main>m__0 ()
{
}
public static void Main ()
{
Action a = <Main>m__0;
}
}
// Roslyn output equivalent
class Example {
class <>c {
public static <>c <>9 = new <>c ();
public static Action <>9__0_0;
internal void <Main>b__0_0 ()
{
}
}
public static void Main ()
{
if (<>c.<>9__0_0 == null)
<>c.<>9__0_0 = <>c.<Main>.b__0_0;
Action a = <>c.<>9__0_0;
}
}
Aside: Roslyn, you're drunk. WTF?!
Why does this matter? Because `TestType` uses
`JniEnvironment.Runtime.MarshalMemberBuilder.CreateMarshalToManagedDelegate()`
on a `Delegate` instance, which does lots of `System.Linq.Expressions`
voodoo, the short of which is that it creates a new method that uses
`Delegate.Method.DeclaringType`:
static Delegate GetEqualsThisHandler ()
{
Func<IntPtr, IntPtr, IntPtr, bool> h = (jnienv, n_self, n_value) => {
...
};
return JniEnvironment.Runtime.MarshalMemberBuilder.CreateMarshalToManagedDelegate (h);
}
In `mcs` output, `h.Method.DeclaringType` is `typeof(TestType)`.
In Roslyn output, `h.Method.DeclaringType` is `typeof(TestType.<>c)`.
This breaks all manner of things, which is why the unit tests fail.
The fix? Don't Do That™, specifically, don't use lambda methods here.
Move the lambda method bodies out into normal methods, so that the
`Delegate.Method.DeclaringType` for a delegate targeting those methods
is of the appropriate type.
[0]: dotnet/android@54a7a02
jonpryor
force-pushed
the
mono-2017-02
branch
from
37b0f20 to
567b82d
Compare
xmcclure
changed the title
Closed
jonpryor
pushed a commit
that referenced
this pull request
Oct 4, 2021
Context: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1314263 Context: c936d09 Changes: dotnet/android-tools@d92fc3e...34e98e2 * dotnet/android-tools@34e98e2: [build] Allow Assembly "vendorization" (#136) * dotnet/android-tools@061bcc2: [build] Import "parent directory.override.targets" (#135) * dotnet/android-tools@a5194e9: [Xamarin.Android.Tools.AndroidSdk] Downgrade build-tools to API-30 * dotnet/android-tools@440e6be: [Xamarin.Android.Tools.AndroidSdk] Update SDK component for API-31 (#134) * dotnet/android-tools@9b658b2: Merge pull request #133 from xamarin/ndk-r23 * dotnet/android-tools@ff73f92: [build] Use GitInfo to generate $(Version) (#131) * dotnet/android-tools@4c2e36c: [Xamarin.Android.Tools.AndroidSdk] Eclipse Adoptium support (#132) * dotnet/android-tools@eaec4e3: [Xamarin.Android.Tools.AndroidSdk] More Microsoft Dist JDK Support (#130) * dotnet/android-tools@f9c1b0d: [BaseTasks] improve Task settings in AsyncTaskExtensions (#129) * dotnet/android-tools@efc9b67: Merge pull request #128 from dellis1972/bumpitybump * dotnet/android-tools@40adec0: Bump LibZipSharp and Mono.Unix to latest stable versions, * dotnet/android-tools@87acd6b: [Microsoft.Android.Build.BaseTasks] add StrongName (#127) * dotnet/android-tools@02f7ae7: [NDK] Properly detect 64-bit NDK * dotnet/android-tools@623332d: Bump LibZipSharp to 2.0.0-alpha7 (#126) * dotnet/android-tools@c055fa8: [Microsoft.Android.Build] Bump to MSBuild 16.10.0 (#125) * dotnet/android-tools@49936d6: Merge pull request #124 from xamarin/update-libzipsharp * dotnet/android-tools@ef78dfc: Bump LibZipSharp to 2.0.0-alpha6 * dotnet/android-tools@e3d708c: [BaseTasks] fix `\`-delimited paths on macOS (#122) * dotnet/android-tools@bdcf899: Reference the new Mono.Unix nuget (#123) * dotnet/android-tools@90d7621: [BaseTasks] add ABI detection for RIDs (#121) * dotnet/android-tools@79e3b97: [JdkInfo] handle invalid XML from /usr/libexec/java_home (#120) * dotnet/android-tools@81519fe: Add SECURITY.md (#119) * dotnet/android-tools@683f375: [Microsoft.Android.Build] Use MSBuild NuGets (#118) * dotnet/android-tools@2241489: [Xamarin.Android.Tools.AndroidSdk] Support Microsoft Dist JDK (#117) * dotnet/android-tools@52ef989: [Xamarin.Android.Tools.AndroidSdk] Fix CS0168 warning (#116) Moving the Roslyn Analyzers initialization code from `Directory.Build.props` to `Directory.Build.targets` (c936d09) caused the Roslyn Analyzers to be applied to code imported from `external/xamarin-android-tools`. Previously the xamarin-android-tools code was unaffected because it had its own `Directory.Build.props` file, so it did not traverse the file system upwards any further to find the Java.Interop files. Fixes for Roslyn issues has not been applied to `xamarin-android-tools` code, so it adds ~150 warnings to the Java.Interop build. dotnet/android-tools@ff73f925 added a `Directory.Build.targets`, so updating `external/xamarin-android-tools` to the latest xamarin-android-tools commit will prevent Java.Interop's `Directory.Build.targets` file from being used, which will exclude xamarin-android-tools from the Roslyn Analyzers.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.