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] Better support for netstandard libraries. #1356

Merged
merged 8 commits into from
Mar 21, 2018

Conversation

dellis1972
Copy link
Contributor

@dellis1972 dellis1972 commented Mar 2, 2018

Fixes #1154, #1162

Netstandard packages sometimes ship with both reference and
implementation assemblies. The Nuget build task ResolveNuGetPackageAssets
only resolves the ref version of the assemblies. There does
not seem to be away way to FORCE Nuget to resolve the lib one.
How .net Core manages to do this is still a mistery. That said
the Nuget ResolveNuGetPackageAssets does give us a hint as to
how to use the project.assets.json file to figure out what
lib version of the package we should be including.

This commit reworks ResolveAssemblies to attempt to map the
ref to a lib if we find a Referenece Assembly. Historically
we just issue a warning (which will probably be ignored), but
now we will use the project.assets.json file to find the
implementation version of the ref assembly.

We need to be using Nuget.ProjectModel since it an API for
querying the project.assets.json. We make use of the Nuget build properties
$(ProjectLockFile) for the location of the project.assets.json
, $(NuGetPackageRoot) for the root folder of the Nuget packages
and $(NuGetTargetMoniker) for resolving which TargetFrameworks
we are looking for. All of these properties should be set by Nuget.
If they are not we should fallback to the default behaviour and just issue the warning.

{
	"version": 3,
	"targets": {
			"MonoAndroid,Version=v8.1": {
			"System.IO.Packaging/4.4.0": {
				"type": "package",
				"compile": {
					"ref/netstandard1.3/System.IO.Packaging.dll": {}
				},
				"runtime": {
					"lib/netstandard1.3/System.IO.Packaging.dll": {}
				}
			},
		}
	}
}

The code above is a cut down sample of the project.assets.json. So our
code will first resolve the targets. We use $(NuGetTargetMoniker) to
do this. For an android project this should have a value of
MonoAndroid,Version=v8.1. Note we do NOT need to worry about the version
here. When Nuget restores the packages it creates the file so it will
use the correct version.
Next we try to find the System.IO.Packaging. We need to look at the
lockFile.Libraries to get information about the install path in the
Nuget folder, and then target.Libraries to pick out the runtime
item.
Once we have resolved the path we need to then combine that with the
$(NuGetPackageRoot) to get the full path to the new library. If at any
point during all of this code we don't get what we expect (i.e a null) we
should abort and just issue the warning.

The only real concern with this is if the format of the project.assets.json
file changes. It is not ment to be edited by a human so there is the
possibiltity that the Nuget team will decide to either change the schema or
even migrate to a new file format. Hopefully we can just update the Nuget
nuggets if that happens.

@dellis1972 dellis1972 added the do-not-merge PR should not be merged. label Mar 2, 2018
@dellis1972 dellis1972 requested a review from mrward March 2, 2018 17:54
@mrward
Copy link

mrward commented Mar 2, 2018

System.Runtime uses . for both ref and lib assemblies. So I guess that would need to be handled in some way.

@mrward
Copy link

mrward commented Mar 2, 2018

Although System.IO.Compression uses . in its ref and lib directory and you are using that as a test. So maybe it is already handled :)

@dellis1972 dellis1972 changed the title [WIP [Xamarin.Android.Build.Tasks] Better support for netstandard libraries. Mar 5, 2018
@dellis1972
Copy link
Contributor Author

Requires dotnet/java-interop#270

jonpryor pushed a commit to dotnet/java-interop that referenced this pull request Mar 5, 2018
…forced loading. (#270)

Context: dotnet/android#1154
Context: dotnet/android#1356

One of the problems we currently face is that our build system
resolves `ref` netstandard libraries. With the current cache
system, onces an assembly has been loaded into the Cecil cache
it only ever uses that assembly. We might need to replace the
current cached version with a new one if we detect a `ref` and
need to reload the `lib`.
@@ -48,6 +48,53 @@
<Reference Include="FSharp.Compiler.CodeDom">
<HintPath>..\..\packages\FSharp.Compiler.CodeDom.1.0.0.1\lib\net40\FSharp.Compiler.CodeDom.dll</HintPath>
</Reference>
<Reference Include="Newtonsoft.Json">
<HintPath>..\..\packages\Newtonsoft.Json.11.0.1\lib\net45\Newtonsoft.Json.dll</HintPath>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonpryor do we have to add all this junk to the Mac installer?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dellis1972: if we need it at "runtime" -- app build time, packaging time, etc. -- then yes, we need to add this to the macOS installer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, will need to remember that for when we bump monodroid.

@dellis1972 dellis1972 removed the do-not-merge PR should not be merged. label Mar 9, 2018
@dellis1972
Copy link
Contributor Author

build

@@ -65,6 +77,11 @@ bool Execute (DirectoryAssemblyResolver resolver)

var topAssemblyReferences = new List<AssemblyDefinition> ();

LockFile lockFile = null;
if (!string.IsNullOrEmpty (ProjectAssetFile) && File.Exists (ProjectAssetFile)) {
lockFile = LockFileUtilities.GetLockFile (ProjectAssetFile, NullLogger.Instance);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want NullLogger.Instance here? That implies we won't get any log messages from NuGet, or so I assume.

@@ -320,6 +325,7 @@ protected override void OnResume()
IsRelease = true,
UseLatestPlatformSdk = true,
References = {
new BuildItem.Reference ("Java.Interop.Export"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How'd we manage to not need this before? If we didn't need it before, why's it needed now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is related to what @pjcollins saw in his tests. Not sure why I needed to add Java.Interop.Export, bit the test wouldn't build without it.. might have been a change upstream?

@jonpryor
Copy link
Member

Please update src/Xamarin.Android.Build.Tasks.tpnitems to provide the license information for the added NuGet packages.

@@ -120,6 +142,35 @@ bool Execute (DirectoryAssemblyResolver resolver)
readonly List<string> do_not_package_atts = new List<string> ();
int indent = 2;

AssemblyDefinition ResolveRuntimeAssemblyForReferenceAssembly (LockFile lockFile, DirectoryAssemblyResolver resolver, AssemblyNameDefinition assemblyNameDefinition)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming this method was cribbed from .NET Core? If that's the case, we should likewise mention that in src/Xamarin.Android.Build.Tasks.tpnitems.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope, it was written by me. I referenced the Nuget source code to see how the API Nuget.ProjectModel was used but none of this code was copied from .Net Core or Nuget.

@pjcollins
Copy link
Member

@jonpryor @dellis1972 I managed to work around #1401 and test two projects which were affected by this startup issue (e.g. monodroid-assembly: Could not load assembly 'System.IO.Packaging' during startup registration.). I used the repro from #1162 and one that I created locally, and this fix addresses the issue in both cases. I did have a few notes from the reproduction attempts:

  1. Non fatal messaging about a failure to load Java.Interop.Export. This messaging occurs even with a simple project which does not include any netstandard references. I am not sure if this was somehow caused by my "hacky" installation or if it presents any real cause for concern.
03-12 16:22:31.940 10235 10235 I mono-stdout: Java.Interop.Export assembly was not loaded: System.IO.FileNotFoundException: Could not load file or assembly 'Java.Interop.Export' or one of its dependencies
03-12 16:22:31.944 10235 10235 I mono-stdout: File name: 'Java.Interop.Export'
03-12 16:22:31.944 10235 10235 I mono-stdout:   at System.AppDomain.Load (System.Reflection.AssemblyName assemblyRef, System.Security.Policy.Evidence assemblySecurity) [0x00076] in <a9ec17abe8f541cd85e7bf70a57b9bcd>:0
03-12 16:22:31.944 10235 10235 I mono-stdout:   at System.AppDomain.Load (System.Reflection.AssemblyName assemblyRef) [0x00000] in <a9ec17abe8f541cd85e7bf70a57b9bcd>:0
03-12 16:22:31.944 10235 10235 I mono-stdout:   at (wrapper remoting-invoke-with-check) System.AppDomain.Load(System.Reflection.AssemblyName)
03-12 16:22:31.944 10235 10235 I mono-stdout:   at System.Reflection.Assembly.Load (System.Reflection.AssemblyName assemblyRef) [0x00005] in <a9ec17abe8f541cd85e7bf70a57b9bcd>:0
03-12 16:22:31.944 10235 10235 I mono-stdout:   at Java.Interop.JniRuntime.SetMarshalMemberBuilder (Java.Interop.JniRuntime+CreationOptions options) [0x0002f] in <f329a7de5a6e4b1f8784e0ff6b58c3f3>:0
  1. I captured build output from both 15-7 and this OSS build for comparison, in case anything in those outputs (especially wrt ResolveAssemblies) seems interesting to you:
    15.7 (fail) - d15-7_build_1162.txt
    PR build (pass) - oss-3a60f76_build_1162.txt

  2. Finally, @marek-safar had mentioned on ResolveAssemblies task wrongly uses reference assemblies #1162 that these assemblies should "never be used as an input to linker or any post compilation processing". I am assuming that was only referring to the "compile" version, and that this is no longer a cause for concern as any post compilation processing (such as linking or AOT) will be done against the newly discovered "runtime" assembly?

@jonpryor
Copy link
Member

Non fatal messaging about a failure to load Java.Interop.Export.

That's ignorable, if unfortunate; we're going to see that once on every process startup. :-(

Does Debug.WriteLine() get written in Release apps? I wouldn't expect it to; we should verify this.

Additionally, commercial installs use Release-Configuration assemblies now. Do you see that message with d15-6 or d15-7? If not, then this is because you're using the Debug-configuration Java.Interop.dll, and I'll breath much easier.

Finally, @marek-safar had mentioned on #1162 that these assemblies should "never be used as an input to linker or any post compilation processing"

@radekdoulik? From the oss-3a60f76_build_1162.txt build log, I see:

2>  LinkAssemblies Task (TaskId:241)
2>    UseSharedRuntime: False (TaskId:241)
2>    MainAssembly: obj\Release\linksrc\DroidIssue.Android.dll (TaskId:241)
2>    OutputDirectory: obj\Release\android\assets\ (TaskId:241)
2>    OptionalDestinationDirectory:  (TaskId:241)
2>    I18nAssemblies:  (TaskId:241)
2>    LinkMode: SdkOnly (TaskId:241)
2>    LinkSkip:  (TaskId:241)
2>    LinkDescriptions: (TaskId:241)
2>    ResolvedAssemblies: (TaskId:241)
2>      ...
2>      obj\Release\linksrc\System.Runtime.CompilerServices.Unsafe.dll (TaskId:241)

So it is provided to <LinkAssemblies/>, but:

2>  Output action:     Save assembly: System.Runtime.CompilerServices.Unsafe, Version=4.0.3.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a (TaskId:241)

...it's only Saved, not Linked. Presumably this is correct?

@marek-safar
Copy link
Contributor

I think is very hacky as the assumption that ref to lib paths have to match is not always true.

I think the better solution would be to use or reuse the already existing logic which does that in msbuild/nuget when you build with netstandard packages and it copies correct files to bin folder (note: I don't know how hard/easy would that be)

@dellis1972
Copy link
Contributor Author

@marek-safar I have been trying to find the code which does the replacement without too much luck so far. So you have any insight please feel free to share it :)

This PR does NOT assume ref to lib paths are the same. It uses the Nuget.ProjectModel to resolve the "runtime" using the exact API thats Nuget uses to resolve the "compile" one [1].

So we lookup via the API "System.IO.Packaging" for example, and then the API tells us where to find the "runtime" assembly (if one exists). I don't see that as hacky, I assume that .net core is doing something similar.

A hacky solution IMHO would be

assembly_path.Replace ("ref", "lib");

which would definitely have problems.

[1] https://github.com/xamarin/xamarin-android/pull/1356/files#diff-0288d889422bfb2c25f3263df583775bR160

@pjcollins
Copy link
Member

@jonpryor

Does Debug.WriteLine() get written in Release apps? I wouldn't expect it to; we should verify this.

If I add a Debug.Writeline() message to a new template project and deploy in Release mode, the message is not present in logcat. Interestingly enough however, I don't see Debug.WriteLine() output in logcat when deploying in Debug mode either. I only see those messages in the Debug output pane of the IDE when deploying in Debug mode. In the case reported above, the Java.Interop.Export message was printed after deploying in Release mode. I do not see the same Java.Interop.Export message when using 15.6 however.

@dellis1972
Copy link
Contributor Author

dellis1972 commented Mar 13, 2018 via email

@jonpryor
Copy link
Member

To elaborate on @dellis1972' last comment and @pjcollins' observed behavior:

Interestingly enough however, I don't see Debug.WriteLine() output in logcat when deploying in Debug mode either

What's happening is this:

  1. [Conditional("SYMBOL")] members are only included in the resulting assembly if SYMBOL is #defined by the compiler.

  2. Debug.WriteLine() is [Conditional("DEBUG")].

  3. Java.Interop.JniRuntime, in Java.Interop.dll, has the Debug.WriteLine() invocation. consequently, Java.Interop.dll will only contain the Debug.WriteLine() call in Debug builds of Java.Interop.dll. (App builds are not relevant.)

  4. The commercial Xamarin.Android product builds Java.Interop.dll/etc. in Release configuration.

  5. The installation environment which @pjcollins constructed used Java.Interop.dll/etc. built in the Debug configuration.

In terms of .vsix build output, I would thus expect the bin/BuildDebug/Xamarin.Android.Sdk-OSS-*.vsix file to print the message, while bin/BuildRelease/Xamarin.Android.Sdk-OSS-*.vsix should not.


public System.Threading.Tasks.Task LogAsync (LogLevel level, string data)
{
return System.Threading.Tasks.Task.Run (() => Log (level, data));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually safe? See also what AsyncTask has to go through for async logging...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok probably not...not sure we want ResolveAssemblies to derive from AsyncTask though.

<HintPath>..\..\packages\Newtonsoft.Json.11.0.1\lib\net45\Newtonsoft.Json.dll</HintPath>
</Reference>
<Reference Include="NuGet.Frameworks">
<HintPath>..\..\packages\NuGet.Frameworks.4.6.0\lib\net46\NuGet.Frameworks.dll</HintPath>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These packages are what need to be added to src/Xamarin.Android.Build.Tasks.tpnitems. If they need to be redistributed, they need to be in a .tpnitems file, with corresponding license information.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. Don't think we need to add System.Runtime or System.Runtime.InteropServices since I don't think we need to ship those as they should be resolved as part of the .net runtime.. in theory.

@kzu
Copy link
Contributor

kzu commented Mar 15, 2018

We need some feedback on this from the NuGet team.

Please @rrelyea and @rohit21agrawal could you advise on how to best resolve this?

@kzu
Copy link
Contributor

kzu commented Mar 15, 2018

@rrelyea @rohit21agrawal this is related to what we were talking on-site: if the ResolveNuGetPackageAssets returned items with additional lib paths in addition to the ref, I think we could easily consume that downstream from our targets.

@dellis1972 maybe we could even send them a PR that adds ^? Possibly in https://github.com/NuGet/NuGet.BuildTasks/blob/dev/src/Microsoft.NuGet.Build.Tasks/ResolveNuGetPackageAssets.cs?

jonpryor pushed a commit that referenced this pull request Mar 21, 2018
Fixes dotnet#1154, dotnet#1162

Netstandard packages sometimes ship with both reference and
implementation assemblies. The Nuget build task `ResolveNuGetPackageAssets`
only resolves the `ref` version of the assemblies. There does
not seem to be away way to FORCE Nuget to resolve the lib one.
How .net Core manages to do this is still a mistery. That said
the Nuget `ResolveNuGetPackageAssets` does give us a hint as to
how to use the `project.assets.json` file to figure out what
`lib` version of the package we should be including.

This commit reworks `ResolveAssemblies` to attempt to map the
`ref` to a `lib` if we find a Referenece Assembly. Historically
we just issue a warning (which will probably be ignored), but
now we will use the `project.assets.json` file to find the
implementation version of the `ref` assembly.

We need to be using `Nuget.ProjectModel` since it an API for
querying the `project.assets.json`. We make use of the Nuget build properties
`$(ProjectLockFile)` for the location of the `project.assets.json`
, `$(NuGetPackageRoot)` for the root folder of the Nuget packages
and `$(NuGetTargetMoniker)` for resolving which `TargetFrameworks`
we are looking for. All of these properties should be set by Nuget.
If they are not we should fallback to the default behaviour and just issue the warning.

	{
  		"version": 3,
  		"targets": {
    			"MonoAndroid,Version=v8.1": {
				"System.IO.Packaging/4.4.0": {
					"type": "package",
					"compile": {
						"ref/netstandard1.3/System.IO.Packaging.dll": {}
					},
					"runtime": {
						"lib/netstandard1.3/System.IO.Packaging.dll": {}
					}
				},
			}
		}
	}

The code above is a cut down sample of the `project.assets.json`. So our
code will first resolve the `targets`. We use `$(NuGetTargetMoniker)` to
do this. For an android project this should have a value of
`MonoAndroid,Version=v8.1`. Note we do NOT need to worry about the version
here. When Nuget restores the packages it creates the file so it will
use the correct version.
Next we try to find the `System.IO.Packaging`. We need to look at the
`lockFile.Libraries` to get information about the install path in the
Nuget folder, and then `target.Libraries` to pick out the  `runtime`
item.
Once we have resolved the path we need to then combine that with the
`$(NuGetPackageRoot)` to get the full path to the new library. If at any
point during all of this code we don't get what we expect (i.e a null) we
should abort and just issue the warning.

The only real concern with this is if the format of the `project.assets.json`
file changes. It is not ment to be edited by a human so there is the
possibiltity that the Nuget team will decide to either change the schema or
even migrate to a new file format. Hopefully we can just update the `Nuget`
nuggets if that happens.
@JulianPasque
Copy link

Thanks alot for this PR 👍

jonpryor pushed a commit that referenced this pull request Mar 21, 2018
…s. (#1356)

Fixes: #1154
Fixes: #1162

NetStandard packages sometimes ship with both reference and
implementation assemblies. The NuGet build task
`<ResolveNuGetPackageAssets/>` only resolves the `ref` version of the
assemblies. There does not seem to be away way to *force* NuGet to
resolve the lib one.

This commit reworks the `<ResolveAssemblies/>` task to attempt to map
the `ref` to a `lib` if we find a Referenece Assembly. Historically
we just issue a warning (which will probably be ignored), but
now we will use the `project.assets.json` file to find the
implementation version of the `ref` assembly, by using the
`NuGet.ProjectModel` package to find the library which is associated
with a reference assembly.

We thus "ignore" reference assemblies, using the "referenced"/"real"
assemblies instead, to ensure that our existing build process
continues to generate usable packages.

*Note*: An alternative approach would be to "embrace" reference
assemblies, allowing them to be used in the build. This doesn't
currently work with our build system, as we assume assemblies will
contain native libraries, Android resources, and other artifacts
which must be extracted at build time, and reference assemblies will
not contain these artifacts.

We would like to explore the "embrace" strategy, but this will
require far more effort to support.
LogWarning ($"Could not resolve target for '{TargetMoniker}'");
return null;
}
var libraryPath = lockFile.Libraries.FirstOrDefault (x => x.Name == assemblyNameDefinition.Name);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Library's name property is the package id - is it guaranteed that AssemblyNameDefninition.Name will have a matching package id?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rohit21agrawal that is a good point. I might have to rework this a bit then :(
cc @jonpryor should we revert?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about emitting AssemblyMetadataAttribute with the PackageId from msbuild, using something like https://mobile.twitter.com/kzu/status/977257467917819905

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reworked the code in #1459 :)

jonpryor pushed a commit to dotnet/java-interop that referenced this pull request Mar 22, 2018
…forced loading. (#270)

Context: dotnet/android#1154
Context: dotnet/android#1356

One of the problems we currently face is that our build system
resolves `ref` netstandard libraries. With the current cache
system, onces an assembly has been loaded into the Cecil cache
it only ever uses that assembly. We might need to replace the
current cached version with a new one if we detect a `ref` and
need to reload the `lib`.
jonpryor added a commit that referenced this pull request Mar 22, 2018
Context: #1154
Context: #1356

Commit c7b9a50 [broke the build][0], because we neglected to
cherry-pick a dependency from Java.Interop (doh!):

[0]: https://jenkins.mono-project.com/view/Xamarin.Android/job/xamarin-android-d15-7/13/

	Tasks/ResolveAssemblies.cs(183,32): error CS1739: The best overload for 'Load' does not have a parameter named 'forceLoad'

Bump to Java.Interop/d15-7/4e1965d5, which adds the
`DirectoryAssemblyResolver.Load(string fileName, bool forceLoad)`
overload, which should fix the above error.
@filoe
Copy link

filoe commented Mar 24, 2018

Is this already published on nuget?

@JulianPasque
Copy link

Xamarin.android is not installed using nuget. I think lt will be published through a Visual Studio update. However there is still an unmerged PR

@@ -3,4 +3,17 @@
<package id="FSharp.Compiler.CodeDom" version="1.0.0.1" targetFramework="net45" />
<package id="FSharp.Core" version="3.1.2.5" targetFramework="net451" />
<package id="Irony" version="0.9.1" targetFramework="net45" />
<package id="Newtonsoft.Json" version="11.0.1" targetFramework="net451" />
<package id="NuGet.Common" version="4.6.0" targetFramework="net462" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to switch to PackageReference, this would have been two entries or so, with no changes in the csproj ;)

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.

System.IO.Packaging not resolved correctly
9 participants