Skip to content

Conversation

@lambdageek
Copy link
Member

@lambdageek lambdageek commented Aug 7, 2017

We should take #631 first.

@lambdageek lambdageek added the full-mono-integration-build For PRs; run a full build (~6-10h for mono bumps), not the faster PR subset (~2h for mono bumps) label Aug 7, 2017
@lambdageek
Copy link
Member Author

Hi @borgdylan, could you give me a pointer to a jenkins log where you noticed that failure? Is it happening with this PR or in xamarin-android master or someplace else? That particular line (5888) seems to be a pretty different part of mono - it would be great to know the particular testcase that triggers the assertion. If you can, please file a bug at https://bugzilla.xamarin.com with the details. Thanks!

@borgdylan
Copy link

It happened locally when using XA-master. I can either file a bug, or keep the conversation going on gitter.

@lambdageek
Copy link
Member Author

Oh I see there's already a discussion about it on gitter. perfect. XA-master is using a mono that's about 4 months older than this PR and as I said the llvm backend is pretty different from normal AOT. if you have a stack trace from the assertion, put it on gitter please.

@borgdylan
Copy link

How would I get the stack trace for the assertion? My build seems to get its mono from a prebuilt CI archive.

jonpryor added a commit to jonpryor/java.interop that referenced this pull request Aug 21, 2017
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=57828

The #runtime team is attempting to update xamarin-android to use the
[mono/2017-06][0] and [mono/2017-08][1] (and...) branches, and when
doing so they are seeing a unit test fail in
`Xamarin.Android.Build.Tests.BuildTest.GeneratorValidateMultiMethodEventName()`:
no `BG850*` warnings are expected but they are produced.

After much analysis, the cause of ght `BG850*` warnings is that
the mono/2017-06 branch introduces a non-generic "overload" of
`System.Collections.Generic.KeyValuePair`:

	namespace System.Collections.Generic {
		// New with mono/2017-06
		partial class KeyValuePair {
			public static KeyValuePair<TKey, TValue> Create<TKey, TValue>(TKey key, TValue value);
		}

		// Existing since forever
		partial struct KeyValuePair<TKey, TValue> {
		}
	}

Specifically, the above "`KeyValuePair` overload" type runs into a
FIXME within `CodeGenerator.cs`:

	// FIXME: at some stage we want to import generic types.
	// For now generator fails to load generic types that have conflicting type e.g.
	// AdapterView`1 and AdapterView cannot co-exist.
	// It is mostly because generator primarily targets jar (no real generics land).

The problem is that the check that is performed is too strict: given a
type `AdapterView<T>`, we only check to see if `AdapterView` exists as
well. If it does exist, then we *ignore* the `AdapterView<T>` type,
and only use the `AdapterView` type for code generation.

This check causes us to skip type registration for
`KeyValuePair<TKey, TValue>`, which in turn causes us to invalidate
e.g. `IDictionary<TKey, TValue>` -- as it implements
`ICollection<KeyValuePair<TKey, TValue>>` -- and everything promply
falls apart from there:

	warning BG8C00: For type System.Collections.Generic.IDictionary`2, base interface System.Collections.Generic.ICollection`1<System.Collections.Generic.KeyValuePair`2<TKey,TValue>> is invalid.
	warning BG8C00: For type System.Collections.Generic.IDictionary`2, base interface System.Collections.Generic.IEnumerable`1<System.Collections.Generic.KeyValuePair`2<TKey,TValue>> is invalid.
	warning BG8502: Invalidating System.Collections.Generic.IDictionary`2 and all nested types because some of its interfaces were invalid.
	warning BG8801: Invalid parameter type System.Collections.Generic.IDictionary`2<System.String,System.Collections.Generic.IList`1<System.String>> in method MapEquivalents in managed type Java.Util.Locale.LanguageRange.
	warning BG8801: Invalid parameter type System.Collections.Generic.IDictionary`2<System.String,System.Collections.Generic.IList`1<System.String>> in method Parse in managed type Java.Util.Locale.LanguageRange.
	warning BG8801: Invalid parameter type System.Collections.Generic.IDictionary`2<System.String,System.Object> in method NewFileSystem in managed type Java.Nio.FileNio.Spi.FileSystemProvider.
	warning BG8801: Invalid parameter type System.Collections.Generic.IDictionary`2<System.String,System.Object> in method NewFileSystem in managed type Java.Nio.FileNio.Spi.FileSystemProvider.
	warning BG8801: Invalid parameter type System.Collections.Generic.IDictionary`2<System.String,System.String> in method .ctor in managed type Java.Security.Provider.Service.
	warning BG8801: Invalid parameter type System.Collections.Generic.IDictionary`2<System.Object,System.Object> in method PutAll in managed type Java.Security.Provider.

I still don't fully understand the scenario that the check was
attempting to solve, so in lieu of actually fixing the FIXME, make the
check *stricter* so that we only ignore "generically overloaded" types
if they bind the *same* Java type. Since `KeyValuePair<TKey, TValue>`
binds *no* Java type, it will no longer be skipped, thus removing the
BG8502 and related warnings.

Additionally, a bit of code cleanup for consistency: some parts of the
code use `HAVE_CECIL`, and others use `USE_CECIL`. Standardize on
`HAVE_CECIL` for consistency and sanity.

[0]: dotnet/android#631
[1]: dotnet/android#723
jonpryor added a commit to jonpryor/java.interop that referenced this pull request Aug 21, 2017
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=57828

The #runtime team is attempting to update xamarin-android to use the
[mono/2017-06][0] and [mono/2017-08][1] (and...) branches, and when
doing so they are seeing a unit test fail in
`Xamarin.Android.Build.Tests.BuildTest.GeneratorValidateMultiMethodEventName()`:
no `BG850*` warnings are expected but they are produced.

After much analysis, the cause of ght `BG850*` warnings is that
the mono/2017-06 branch introduces a non-generic "overload" of
`System.Collections.Generic.KeyValuePair`:

	namespace System.Collections.Generic {
		// New with mono/2017-06
		partial class KeyValuePair {
			public static KeyValuePair<TKey, TValue> Create<TKey, TValue>(TKey key, TValue value);
		}

		// Existing since forever
		partial struct KeyValuePair<TKey, TValue> {
		}
	}

Specifically, the above "`KeyValuePair` overload" type runs into a
FIXME within `CodeGenerator.cs`:

	// FIXME: at some stage we want to import generic types.
	// For now generator fails to load generic types that have conflicting type e.g.
	// AdapterView`1 and AdapterView cannot co-exist.
	// It is mostly because generator primarily targets jar (no real generics land).

The *intent* of the check that is that, given a type `AdapterView<T>`,
we only check to see if `AdapterView` exists as well. If it does exist,
then we *ignore* the `AdapterView<T>` type, and only use the
`AdapterView` type for code generation.

In the case of `KeyValuePair`, the same "check" is done, which cause
us to *skip* type registration for `KeyValuePair<TKey, TValue>`, which
in turn causes us to invalidate e.g. `IDictionary<TKey, TValue>` -- as
it implements `ICollection<KeyValuePair<TKey, TValue>>` -- and
everything promply falls apart from there:

	warning BG8C00: For type System.Collections.Generic.IDictionary`2, base interface System.Collections.Generic.ICollection`1<System.Collections.Generic.KeyValuePair`2<TKey,TValue>> is invalid.
	warning BG8C00: For type System.Collections.Generic.IDictionary`2, base interface System.Collections.Generic.IEnumerable`1<System.Collections.Generic.KeyValuePair`2<TKey,TValue>> is invalid.
	warning BG8502: Invalidating System.Collections.Generic.IDictionary`2 and all nested types because some of its interfaces were invalid.
	warning BG8801: Invalid parameter type System.Collections.Generic.IDictionary`2<System.String,System.Collections.Generic.IList`1<System.String>> in method MapEquivalents in managed type Java.Util.Locale.LanguageRange.
	warning BG8801: Invalid parameter type System.Collections.Generic.IDictionary`2<System.String,System.Collections.Generic.IList`1<System.String>> in method Parse in managed type Java.Util.Locale.LanguageRange.
	warning BG8801: Invalid parameter type System.Collections.Generic.IDictionary`2<System.String,System.Object> in method NewFileSystem in managed type Java.Nio.FileNio.Spi.FileSystemProvider.
	warning BG8801: Invalid parameter type System.Collections.Generic.IDictionary`2<System.String,System.Object> in method NewFileSystem in managed type Java.Nio.FileNio.Spi.FileSystemProvider.
	warning BG8801: Invalid parameter type System.Collections.Generic.IDictionary`2<System.String,System.String> in method .ctor in managed type Java.Security.Provider.Service.
	warning BG8801: Invalid parameter type System.Collections.Generic.IDictionary`2<System.Object,System.Object> in method PutAll in managed type Java.Security.Provider.

I still don't fully understand the scenario that the check was
attempting to solve, so in lieu of actually fixing the FIXME, make the
check *stricter* so that we only ignore "generically overloaded" types
if they bind the *same* Java type. Since `KeyValuePair<TKey, TValue>`
binds *no* Java type, it will no longer be skipped, thus removing the
BG8502 and related warnings.

Additionally, a bit of code cleanup for consistency: some parts of the
code use `HAVE_CECIL`, and others use `USE_CECIL`. Standardize on
`HAVE_CECIL` for consistency and sanity.

[0]: dotnet/android#631
[1]: dotnet/android#723
atsushieno pushed a commit to dotnet/java-interop that referenced this pull request Aug 22, 2017
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=57828

The #runtime team is attempting to update xamarin-android to use the
[mono/2017-06][0] and [mono/2017-08][1] (and...) branches, and when
doing so they are seeing a unit test fail in
`Xamarin.Android.Build.Tests.BuildTest.GeneratorValidateMultiMethodEventName()`:
no `BG850*` warnings are expected but they are produced.

After much analysis, the cause of ght `BG850*` warnings is that
the mono/2017-06 branch introduces a non-generic "overload" of
`System.Collections.Generic.KeyValuePair`:

	namespace System.Collections.Generic {
		// New with mono/2017-06
		partial class KeyValuePair {
			public static KeyValuePair<TKey, TValue> Create<TKey, TValue>(TKey key, TValue value);
		}

		// Existing since forever
		partial struct KeyValuePair<TKey, TValue> {
		}
	}

Specifically, the above "`KeyValuePair` overload" type runs into a
FIXME within `CodeGenerator.cs`:

	// FIXME: at some stage we want to import generic types.
	// For now generator fails to load generic types that have conflicting type e.g.
	// AdapterView`1 and AdapterView cannot co-exist.
	// It is mostly because generator primarily targets jar (no real generics land).

The *intent* of the check that is that, given a type `AdapterView<T>`,
we only check to see if `AdapterView` exists as well. If it does exist,
then we *ignore* the `AdapterView<T>` type, and only use the
`AdapterView` type for code generation.

In the case of `KeyValuePair`, the same "check" is done, which cause
us to *skip* type registration for `KeyValuePair<TKey, TValue>`, which
in turn causes us to invalidate e.g. `IDictionary<TKey, TValue>` -- as
it implements `ICollection<KeyValuePair<TKey, TValue>>` -- and
everything promply falls apart from there:

	warning BG8C00: For type System.Collections.Generic.IDictionary`2, base interface System.Collections.Generic.ICollection`1<System.Collections.Generic.KeyValuePair`2<TKey,TValue>> is invalid.
	warning BG8C00: For type System.Collections.Generic.IDictionary`2, base interface System.Collections.Generic.IEnumerable`1<System.Collections.Generic.KeyValuePair`2<TKey,TValue>> is invalid.
	warning BG8502: Invalidating System.Collections.Generic.IDictionary`2 and all nested types because some of its interfaces were invalid.
	warning BG8801: Invalid parameter type System.Collections.Generic.IDictionary`2<System.String,System.Collections.Generic.IList`1<System.String>> in method MapEquivalents in managed type Java.Util.Locale.LanguageRange.
	warning BG8801: Invalid parameter type System.Collections.Generic.IDictionary`2<System.String,System.Collections.Generic.IList`1<System.String>> in method Parse in managed type Java.Util.Locale.LanguageRange.
	warning BG8801: Invalid parameter type System.Collections.Generic.IDictionary`2<System.String,System.Object> in method NewFileSystem in managed type Java.Nio.FileNio.Spi.FileSystemProvider.
	warning BG8801: Invalid parameter type System.Collections.Generic.IDictionary`2<System.String,System.Object> in method NewFileSystem in managed type Java.Nio.FileNio.Spi.FileSystemProvider.
	warning BG8801: Invalid parameter type System.Collections.Generic.IDictionary`2<System.String,System.String> in method .ctor in managed type Java.Security.Provider.Service.
	warning BG8801: Invalid parameter type System.Collections.Generic.IDictionary`2<System.Object,System.Object> in method PutAll in managed type Java.Security.Provider.

I still don't fully understand the scenario that the check was
attempting to solve, so in lieu of actually fixing the FIXME, make the
check *stricter* so that we only ignore "generically overloaded" types
if they bind the *same* Java type. Since `KeyValuePair<TKey, TValue>`
binds *no* Java type, it will no longer be skipped, thus removing the
BG8502 and related warnings.

Additionally, a bit of code cleanup for consistency: some parts of the
code use `HAVE_CECIL`, and others use `USE_CECIL`. Standardize on
`HAVE_CECIL` for consistency and sanity.

[0]: dotnet/android#631
[1]: dotnet/android#723
Fixes https://bugzilla.xamarin.com/show_bug.cgi?id=57828

These failing tests should be fixed:

    Xamarin.Android.Build.Tests.BuildTest.GeneratorValidateMultiMethodEventName(False,null,"<attr path=\"/api/package/interface[@...","")
    Xamarin.Android.Build.Tests.BuildTest.GeneratorValidateMultiMethodEventName(False,null,"\n\t\t\t\t\t<attr path=\"/api/package...","String s")
@jonpryor
Copy link
Contributor

jonpryor commented Oct 6, 2017

Looks like the Jenkins shutdown aborted the build:

FATAL: command execution failed
java.nio.channels.ClosedChannelException
        at org.jenkinsci.remoting.protocol.NetworkLayer.onRecvClosed(NetworkLayer.java:154)
        at org.jenkinsci.remoting.protocol.impl.NIONetworkLayer.ready(NIONetworkLayer.java:142)
        at org.jenkinsci.remoting.protocol.IOHub$OnReady.run(IOHub.java:721)
        at jenkins.util.ContextResettingExecutorService$1.run(ContextResettingExecutorService.java:28)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
        at java.lang.Thread.run(Thread.java:745)
Caused: java.io.IOException: Backing channel 'JNLP4-connect connection from 167.220.148.19/167.220.148.19:55649' is disconnected.
...

We'll need to re-run this once Jenkins is back up.

@lambdageek
Copy link
Member Author

build

@akoeplinger
Copy link
Member

I talked with @lambdageek and since I'm making good progress on mono-2017-10 it probably makes more sense to spend the effort there :) I cherry-picked the relevant fixes from this branch.

@akoeplinger akoeplinger mentioned this pull request Oct 18, 2017
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Sep 21, 2020
Fixes: dotnet/java-interop#682
Fixes: dotnet/java-interop#717

Context: dotnet/java-interop#719

Changes: dotnet/java-interop@a807961...79d9533

  * dotnet/java-interop@79d95334: [generator] Use GC.KeepAlive for reference type method parameters. (dotnet#722)
  * dotnet/java-interop@1a19ec04: [Xamarin.Android.Tools.Bytecode] Hide Kotlin nested types inside (dotnet#723)
  * dotnet/java-interop@24a9abdb: [Xamarin.Android.Tools.ApiXmlAdjuster] Find app.android.IntentService (dotnet#718)
jonpryor added a commit that referenced this pull request Sep 22, 2020
Fixes: dotnet/java-interop#682
Fixes: dotnet/java-interop#717

Context: dotnet/java-interop#719

Changes: dotnet/java-interop@a807961...79d9533

  * dotnet/java-interop@79d95334: [generator] Use GC.KeepAlive for reference type method parameters. (#722)
  * dotnet/java-interop@1a19ec04: [Xamarin.Android.Tools.Bytecode] Hide Kotlin nested types inside (#723)
  * dotnet/java-interop@24a9abdb: [Xamarin.Android.Tools.ApiXmlAdjuster] Find app.android.IntentService (#718)
jonpryor pushed a commit that referenced this pull request Oct 20, 2020
Fixes: dotnet/java-interop#461
Fixes: dotnet/java-interop#682
Fixes: dotnet/java-interop#717
Fixes: dotnet/java-interop#719
Fixes: dotnet/java-interop#728

Changes: dotnet/java-interop@ac914ce...b991bb8

  * dotnet/java-interop@b991bb86: [generator] Revert change to use auto-properties in EventArgs classes (#736)
  * dotnet/java-interop@ee50d89b: Bump to xamarin/xamarin-android-tools/master@f2af06f2 (#733)
  * dotnet/java-interop@a0b895c1: [build] Suppress NuGet warnings (#730)
  * dotnet/java-interop@8b1b0507: [generator] Fix parsing of complex generic types (#729)
  * dotnet/java-interop@ee7afeed: [generator] Prevent generating duplicate EventArgs classes (#726)
  * dotnet/java-interop@1f21f38c: [generator] Use GC.KeepAlive for reference type method parameters. (#725)
  * dotnet/java-interop@5136ef98: [Xamarin.Android.Tools.Bytecode] Hide Kotlin nested types inside (#723)
  * dotnet/java-interop@53d60513: [jnimarshalmethod-gen] Fix registration on Windows (#721)
  * dotnet/java-interop@5a834d42: [jnimarshalmethod-gen] Avoid creating AppDomains (#720)
  * dotnet/java-interop@a76edb8c: [Xamarin.Android.Tools.ApiXmlAdjuster] Find app.android.IntentService (#718)
  * dotnet/java-interop@6cde0877: [Java.Interop] Emit a reference assembly for Java.Interop.dll (#716)
  * dotnet/java-interop@b858dc59: [generator] Provide line/col numbers for api.xml warnings (#715)
  * dotnet/java-interop@9be92a04: [ci] Don't kick off CI for documentation only changes. (#712)
  * dotnet/java-interop@03c22722: [jnimarshalmethod-gen] Fix type resolution crash (#706)
@github-actions github-actions bot locked and limited conversation to collaborators Feb 5, 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.

9 participants