Skip to content

Bump to mono/mono-4.8.0-branch/9e164326 #223

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 15, 2016

Conversation

jonpryor
Copy link
Contributor

Mono 4.8 requires SGEN Bridge Version 5.

Supersedes: #221

@jonpryor
Copy link
Contributor Author

Mono 4.8 uses Cecil/master, which is ABI-incompatible with Cecil 0.9.6 (what we're currently using):

https://gist.github.com/jonpryor/8136c2ba811d9c9383d8f4d4bf411be3

../../external/mono/mcs/tools/tuner/Mono.Tuner/CecilRocks.cs(481,43): error CS1061: Type `Mono.Cecil.TypeReference' does not contain a definition for `InterfaceType' and no extension method `InterfaceType' of type `Mono.Cecil.TypeReference' could be found. Are you missing an assembly reference?

We're going to have to re-work how we build Cecil. We can't use the nuget package.

@lewurm
Copy link
Contributor

lewurm commented Sep 14, 2016

as a reference, xamarin-macios required also changes around Cecil when they bumped mono: dotnet/macios#733

@kumpera
Copy link
Contributor

kumpera commented Sep 14, 2016

@marek-safar we're blocked on Cecil from bumping mono

@jonpryor
Copy link
Contributor Author

As an added complication, the xamarin-android repo references projects/assemblies from the Java.Interop repo. The Java.Interop repo also uses Mono.Cecil, but has no mono reference. Consequently, "just" updating xamarin-android to use Mono.Cecil projects from external/mono will not work.

Thus, take 2:

  1. Bump Java.Interop to use the Mono.Cecil 0.10.0-beta1-pre2 NuGet Package: Bump to Mono.Cecil 0.10.0-beta1-v2 java-interop#77
  2. Update xamarin-android to likewise use the Mono.Cecil 0.10.0-beta1-pre2 NuGet package and include the appropriate Java.Interop commit, once merged.

In other news... ABI BREAKS SUCK.

Film at 11.

@marek-safar
Copy link
Contributor

You can use different versions of cecil (e.g not to change Java.Interop) but for code you are taking from mono 4.8 you need mono 4.8 cecil dependency. This should be quite simple, as all work was done but you need to fix your project(s) to have correct references.

@jonpryor jonpryor force-pushed the jonp-mono-4.8 branch 3 times, most recently from edf9b08 to 049b38b Compare September 15, 2016 13:30
Mono 4.8 has new SGEN bridge performance characteristics and
semantics, requiring that we bump the SGEN Bridge Version to 5.

Additionally, Mono 4.8 is using Cecil/master, which is
API INCOMPATIBLE with Cecil 0.9.6, and Mono sources are included in
`Xamarin.Android.Build.Tasks.dll`.

We could attempt to address the Cecil incompatibility by building
Mono's `external/cecil` projects, but that doesn't work because
Java.Interop assemblies *also* use Cecil, and
`Xamarin.Android.Build.Tasks.dll` uses `Java.Interop.Tools.Cecil.dll`,
which uses Cecil via NuGet package, so everything gets very intermixed
and confusing if Java.Interop tries to use e.g. Cecil 0.9.6 via nuget
while xamarin-android attempts to use Cecil via Mono's source.

Skip that problem, and bump to Java.Interop/a1d3ecc8, which in turn
uses the Cecil 0.10.0-beta1-v2 *Preview* NuGet package, which is
API compoatible with Cecil/master and mono/master.

Additionally, update xamarin-android's Cecil 0.9.6 use to likewise use
Cecil 0.10.0-beta1-v2.

This allows xamarin-android to build while using Mono 4.8 sources.

Finally, there's lots of "unrelated noise" in this patch, largely
because every time I add a project to the solution via Xamarin Studio,
Xamarin Studio decides to modify EVERY PROJECT IN THE REPO. Preserve
some of these changes -- and "fix" others -- so that I can reduce the
"visual noise" of future Xamarin Studio changes.
@grendello grendello merged commit 1d65825 into dotnet:master Sep 15, 2016
radical pushed a commit that referenced this pull request May 8, 2018
Fixes: dotnet/java-interop#168

Java allows public classes to inherit from "package-private" classes, in
which case the `public` & `protected` members of the "package-private"
class are visible within the public class:

    // Java
    /* package */ abstract class SpannableStringInternal {
      public void getChars(int start, int end, char[] dest, int off);
      // ...
    }
    public class SpannableString extends SpannableStringInternal {
      // ...
    }

The problem is that `generator` didn't properly support this construct,
and *skips* binding of "package-private" types, resulting in generated
C# code such as:

    // C#
    public partial class SpannableString : SpannableStringInternal {
      // CS0246: The type or namespace name `SpannableStringInternal` could not be found
    }

This could be worked around via Metadata by making the "package-private"
class `public`, but this means that if (when?) the "package-private"
class is *removed*, we have an ABI break.

Support this construct by updating `generator` to "copy" the members
from the "package-private" class into the declaring class:

    // C#
    public partial class SpannableString : Java.Lang.Object {
      public void GetChars(int start, int end, char[] dest, int off);
      // ...
    }

This allows the generated code to compile without metadata fixup.

Specifics in implementing this:

  - Add a `GenBase.FixupAccessModifiers()` method, later this may
    need to be further extended for methods
  - Call `FixupAccessModifiers()` in the "validation" step
  - Added a setter for `BaseType` so it can be modified
  - In `FixupAccessModifiers()`, lookup the base class of the current
    type and check if it is non-`public`.
  - Skip the package-private types, and replace them with that
    class's base type
  - Look for each method on the base type, and copy it to the public
    type if it does not exist
  - Added tests for this scenario
@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.

6 participants