Skip to content

Conversation

@jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Feb 5, 2020

In preparation for .NET 5, we think the $(TargetFrameworkMoniker)
might be:

.NETCoreApp,Version=5.0,Profile=Xamarin.Android

It might also be Profile=Android, we don't know for sure yet.

The TFM currently is:

MonoAndroid,Version=v10.0

I think we can (and should) remove any code during the build that
looks at $(TargetFrameworkIdentifier).

The places we are currently doing this are for performance:

  • The _ResolveLibraryProjectImports or _GenerateJavaStubs targets
    do not need to run against .NET standard assemblies.

We can rely on a reference to Mono.Android.dll or Java.Interop.dll
instead. We are already using System.Reflection.Metadata to see if the
assembly actually has this reference.

So an example of the condition:

Condition="'%(ResolvedUserAssemblies.TargetFrameworkIdentifier)' == 'MonoAndroid' Or '%(ResolvedUserAssemblies.HasMonoAndroidReference)' == 'True'"

Can just be:

Condition="'%(ResolvedUserAssemblies.HasMonoAndroidReference)' == 'True'"

One test failure was that Xamarin.Forms.Platform.dll used to be
treated as a "Xamarin.Android" assembly.

  • It has a MonoAndroid TFI
  • It has no reference to Mono.Android.dll or Java.Interop.dll
  • It has no Java.Lang.Object subclasses
  • It has no __AndroidLibraryProjects__.zip or *.jar
    EmbeddedResource files

In the case of this assembly, I think everything will continue to
work. It is OK for it to not be treated as a "Xamarin.Android"
assembly.

I also took the opportunity for a small perf improvement:

  • <FilterAssemblies/> only needs to look for obsolete attributes in
    assemblies that reference Mono.Android.dll.

Results

Testing a build with no changes for the SmartHotel360 app:

Before:
 60 ms  FilterAssemblies                           2 calls
After:
 49 ms  FilterAssemblies                           2 calls

I would think there is a small ~11ms performance improvement to all
builds of this app.

@jonathanpeppers jonathanpeppers force-pushed the net5-targetframeworkidentifier branch from 2f06e64 to 3a4e712 Compare February 5, 2020 22:53
@jonpryor
Copy link
Contributor

jonpryor commented Feb 6, 2020

We can rely on a reference to Mono.Android.dll instead

I would like this to also rely on a reference to Java.Interop.dll.

In preparation for .NET 5, we think the `$(TargetFrameworkMoniker)`
*might* be:

    .NETCoreApp,Version=5.0,Profile=Xamarin.Android

It might also be `Profile=Android`, we don't know for sure yet.

The TFM currently is:

    MonoAndroid,Version=v10.0

I think we can (and should) remove any code during the build that
looks at `$(TargetFrameworkIdentifier)`.

The places we are currently doing this are for performance:

* The `_ResolveLibraryProjectImports` or `_GenerateJavaStubs` targets
  do not need to run against .NET standard assemblies.

We can rely on a reference to `Mono.Android.dll` or `Java.Interop.dll`
instead. We are already using System.Reflection.Metadata to see if the
assembly actually has this reference.

So an example of the condition:

    Condition="'%(ResolvedUserAssemblies.TargetFrameworkIdentifier)' == 'MonoAndroid' Or '%(ResolvedUserAssemblies.HasMonoAndroidReference)' == 'True'"

Can just be:

    Condition="'%(ResolvedUserAssemblies.HasMonoAndroidReference)' == 'True'"

One test failure was that `Xamarin.Forms.Platform.dll` *used* to be
treated as a "Xamarin.Android" assembly.

* It has a `MonoAndroid` TFI
* It has *no* reference to `Mono.Android.dll` or `Java.Interop.dll`
* It has no `Java.Lang.Object` subclasses
* It has no `__AndroidLibraryProjects__.zip` or `*.jar`
  `EmbeddedResource` files

In the case of this assembly, I think everything will continue to
work. It is OK for it to not be treated as a "Xamarin.Android"
assembly.

I also took the opportunity for a small perf improvement:

* `<FilterAssemblies/>` only needs to look for obsolete attributes in
  assemblies that reference `Mono.Android.dll`.

~~ Results ~~

Testing a build with no changes for the SmartHotel360 app:

    Before:
     60 ms  FilterAssemblies                           2 calls
    After:
     49 ms  FilterAssemblies                           2 calls

I would think there is a small ~11ms performance improvement to all
builds of this app.
@jonathanpeppers jonathanpeppers force-pushed the net5-targetframeworkidentifier branch from 3a4e712 to efdd682 Compare February 6, 2020 14:33
@jonathanpeppers jonathanpeppers marked this pull request as ready for review February 6, 2020 14:33
@jonathanpeppers
Copy link
Member Author

I made this look for both Java.Interop.dll and Mono.Android.dll now.

@jonpryor
Copy link
Contributor

jonpryor commented Feb 6, 2020

I have concerns, as it looks like this won't deal with "transitive dependencies".

So consider this silly example:

$ cat A.cs
using System;
using System.Text.RegularExpressions;

namespace Example {
    public class A {
        Regex r;
    }
}

$ csc /t:Library /nologo A.cs
A.cs(6,15): warning CS0169: The field 'A.r' is never used

$ cat B.cs
using System;

using Example;

namespace Example {
    public class B : A {        
    }
}

$ csc /t:library /nologo B.cs /r:A.dll

A.dll uses Regex, which lives in System.dll:

$ monodis --assemblyref A.dll
AssemblyRef Table
1: Version=4.0.0.0
	Name=mscorlib
	Flags=0x00000000
	Public Key:
0x00000000: B7 7A 5C 56 19 34 E0 89 
2: Version=4.0.0.0
	Name=System
	Flags=0x00000000
	Public Key:
0x00000000: B7 7A 5C 56 19 34 E0 89 

B.dll references A.dll, and type B inherits Example.A, but B.dll does not reference System:

monodis --assemblyref B.dll
AssemblyRef Table
1: Version=4.0.0.0
	Name=mscorlib
	Flags=0x00000000
	Public Key:
0x00000000: B7 7A 5C 56 19 34 E0 89 
2: Version=0.0.0.0
	Name=A
	Flags=0x00000000
	Zero sized public key

Now, why do I care?

Rename System to Mono.Android.dll, and have A be a type which inherits Java.Lang.Object, and perhaps you see the issue. If we have:

// Assembly: A.dll
namespace Example {
    public class A : Java.Lang.Object {
    }
}

// Assembly: B.dll
namespace Example {
    public class B : A {
    }
}

// ...and for local testing purposes, Mono.Android.dll:
namespace Java.Lang {
    public class Object {        
    }
}

The semantics are that we must pass B.dll to <GenerateJavaStubs/> so that we generate Java Callable Wrappers for it. Unfortunately, B.dll doesn't reference Mono.Android.dll.

Which is doubly odd, because it does need to be referenced:

$ csc /nologo /t:library A.cs /r:Mono.Android.dll
$ csc /nologo /t:library B.cs /r:A.dll /r:Mono.Android.dll
$ monodis --assemblyref B.dll
AssemblyRef Table
1: Version=4.0.0.0
	Name=mscorlib
	Flags=0x00000000
	Public Key:
0x00000000: B7 7A 5C 56 19 34 E0 89 
2: Version=0.0.0.0
	Name=A
	Flags=0x00000000
	Zero sized public key

Above we see that B.cs is compiled into B.dll, and the command line includes a /r:Mono.Android.dll, but the resulting assembly does not reference Mono.Android.dll.

...oops?


Maybe this is already handled by the PR; I don't know. What I'm fairly certain about is that @(_ResolvedUserMonoAndroidAssemblies) must include every assembly that directly or indirectly references Mono.Android.dll, such as the above B.dll.

Use of TargetFrameworkIdentifier allowed this, as it was auto-inserted into every assembly by MSBuild voodoo. (Not so much "pure command-line builds", as used above...) I do not know if this PR makes the same guarantees.

@jonathanpeppers
Copy link
Member Author

$ monodis --assemblyref B.dll

I need to make a test as mentioned above that verifies the java stub exists after a build. Right now this PR doesn't look like it would.

@jonathanpeppers jonathanpeppers added the do-not-merge PR should not be merged. label Feb 6, 2020
@jonpryor
Copy link
Contributor

jonpryor commented Feb 7, 2020

Let's just drop this right here...

Assembly Dependencies

@jonathanpeppers
Copy link
Member Author

It definitely breaks in my test that has Foo -> Bar -> Baz:

obj\Debug\android\src\crc640a50d1657a1878d1\Foo.java(5,31): error JAVAC0000:  error: package crc647801da6e17af61a6 does not exist
	extends crc647801da6e17af61a6.Bar
 [C:\src\xamarin-android\bin\TestDebug\temp\MissingMonoAndroidReference\FooApp\FooApp.csproj]

I almost have it recursively finding the reference for Bar, still working on it.

@jonathanpeppers
Copy link
Member Author

jonathanpeppers commented Feb 13, 2020

I am going to close this for now; I can come back to this if needed.

I was unable to pass the TFM from leaf nodes up to the parent node -- just due to how the ordering/recursive code was working in <ResolveAssemblies/>.

What I think we should do instead is make the new .NET 5 TFMs work, when we know what they will be for sure.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

do-not-merge PR should not be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants