Skip to content
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

[Xamarin.Android.Build.Tasks] remove TFI == MonoAndroid checks #4225

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.

@jonpryor
Copy link
Member

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 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
Member

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
Member

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