[msbuild] Skip reference assemblies passed to mtouch #3791

VincentDondain · 2018-03-20T21:48:48Z

Fixes issue Could not AOT the assembly System.Runtime.CompilerServices.Unsafe.dll (MT3001) #3199: Could not AOT the assembly System.Runtime.CompilerServices.Unsafe.dll (MT3001)
(Could not AOT the assembly System.Runtime.CompilerServices.Unsafe.dll (MT3001) #3199)
Test case: https://www.dropbox.com/s/kxt3isgzn74nq35/Issue3199.zip?dl=0

Problem

Given a Nuget Package added via the "package reference" mechanism and the said package having netstandard lib and ref folders;
MTouch was getting reference assemblies and was trying to AOT them. This resulted in an error as those assemblies are only facades.

Solution

Skipping the assemblies that have /ref/ in their path seems like the simplest and yet most functional solution to the problem.

As it turn out, there is some logic already in place that copies the lib assemblies to the destination folder. It seems equivalent to marking them as "Local Copy".
What this does is that it makes those assemblies available to msbuild via @(ReferenceCopyLocalPaths). This gives us the opportunity to safely remove the ref assemblies from @(ReferencePath).

Note: mtouch is getting the assemblies to reference via a combination of @(ReferencePath) and @(ReferenceCopyLocalPaths).

- Fixes issue xamarin#3199: Could not AOT the assembly System.Runtime.CompilerServices.Unsafe.dll (MT3001) (xamarin#3199) - Test case: https://www.dropbox.com/s/kxt3isgzn74nq35/Issue3199.zip?dl=0 Problem ======= Given a Nuget Package added via the "package reference" mechanism and the said package having netstandard `lib` **and** `ref` folders; MTouch was getting reference assemblies and was trying to AOT them. This resulted in an error as those assemblies are only facades. Solution ======== Skipping the assemblies that have `/ref/` in their path seems like the simplest and yet most functional solution to the problem. As it turn out, there is some logic already in place that copies the `lib` assemblies to the destination folder. It seems equivalent to marking them as "Local Copy". What this does is that it makes those assemblies available to msbuild via `@(ReferenceCopyLocalPaths)`. This gives us the opportunity to safely remove the `ref` assemblies from `@(ReferencePath)`. *Note: `mtouch` is getting the assemblies to reference via a combination of `@(ReferencePath)` and `@(ReferenceCopyLocalPaths)`.*

dalexsoto

Makes sense, I hope it is not very likely that we find /re/ in customer's assemblies 👍

rolfbjarne · 2018-03-20T22:26:30Z

What does Xamarin.Android do in this case? It seems they should have the same problem (so we should also implement the same solution).

monojenkins · 2018-03-21T01:15:04Z

Build success

spouliot

As mentioned in xamarin/xamarin-android#1356 (comment) ref is a convention and something else can be used.

So it's not a correct fix even for nuget and it can break existing code.

It's a better workaround that copying files locally (the previous one) and we can suggest it to customers (local change) but it's not the right way to solve this issue.

spouliot · 2018-03-21T13:59:03Z

msbuild/Xamarin.iOS.Tasks.Core/Xamarin.iOS.Common.targets

@@ -780,7 +780,9 @@ Copyright (C) 2013-2016 Xamarin. All rights reserved.
 		Outputs="$(_NativeExecutable);$(DeviceSpecificOutputPath)mtouch.stamp">

 		<ItemGroup>
-			<MTouchReferencePath Include="@(ReferenceCopyLocalPaths)" Condition="'%(Extension)' == '.dll'" />
+			<!-- Skip the reference assemblies. There is currently no scenario where it is valid to pass them to `mtouch`. They are impossible to AOT. Fixes: https://github.com/xamarin/xamarin-macios/issues/3199 -->
+			<MTouchReferencePath Include="@(ReferencePath)" Condition="!$([System.String]::Copy('%(Directory)').Contains ('/ref/'))" />


That's bound to break (past, current or future) code that's unrelated to nuget
and it's gonna be difficult to locate why it happened

jstedfast · 2018-03-21T15:28:18Z

I agree with @spouliot and @rolfbjarne makes a good suggestion. We should see what Android does and try to solve it the same way.

I'm also concerned that /ref/ might appear in customer's paths, so filtering on that seems wrong.

VincentDondain · 2018-03-21T17:14:34Z

Filtering on /ref/ was a bit optimistic I realize (:

Now I'm not sure copying what Xamarin.Android did is our best option (here's the PR for reference).

One important discovery I made is that the full version of the assembly is automatically copied by msbuild and ends up in @(ReferenceCopyLocalPaths). See gist with _CopyFilesMarkedCopyLocal target. So mtouch gets the reference assembly from @(ReferencePath) (that I hoped to filter out) and the full version from @(ReferenceCopyLocalPaths).
The XA patch is complex and if there is a change in the nuget json format (oh, there will...) it won't work (even if it falls back to the current behavior).

My next steps are:

Investigate why we get the /lib/ version of the assembly from Copy (msbuild).
Talk to interested parties (#msbuild and friends) to know if people have better ideas on how this should be handled.

spouliot · 2018-03-21T20:31:49Z

The \lib\ version seems to be an input to the Copy task.

Target "_CopyFilesMarkedCopyLocal" in file "/Library/Frameworks/Mono.framework/Versions/5.10.0/lib/mono/msbuild/15.0/bin/Microsoft.Common.CurrentVersion.targets":
  Using "Copy" task from assembly "/Library/Frameworks/Mono.framework/External/xbuild/Xamarin/iOS/Xamarin.iOS.Tasks.dll".
  Task "Copy"
    Copying file from "/Users/vidondai/.nuget/packages/system.runtime.compilerservices.unsafe/4.4.0/lib/netstandard2.0/System.Runtime.CompilerServices.Unsafe.dll" to "bin/iPhone/Debug/System.Runtime.CompilerServices.Unsafe.dll".
  Done executing task "Copy".

We need to figure out how that (\lib\) path/file became something known inside msbuild since some code was able to infer the \lib\ version existence (and requirement) somehow...

The task name _CopyFilesMarkedCopyLocal is weird before the logs hints it is not a CopyLocal

        This reference is not "CopyLocal" because at least one source item had "Private" set to "false" and no source items had "Private" set to "true".

see https://gist.github.com/VincentDondain/4bffc499126134f4649199ed8515220a#file-gistfile1-txt-L2552

so it's unclear why it was copied ?!?

Note that the Private = false was for ref not lib so that might be the reason, however the logs are silent about that transition :(

VincentDondain · 2018-03-22T15:31:54Z

Argh and we have an other problem that's not even covered by this PR's "fancy workaround".

See test case: https://www.dropbox.com/s/btb6rzb636pnutf/Issue3199Test2.zip?dl=0

In this new test case the iOS project references a netstandard class library, which references a Nuget Package (System.Runtime.CompilerServices.Unsafe) but the app does NOT reference that package directly.

In this specific case mtouch will fail with MTOUCH : error MT2101: Can't resolve the reference '!!1& System.Runtime.CompilerServices.Unsafe::As(!!0&)', referenced from the method 'System.Void LibWithPackage.Class1::Test()' in 'System.Runtime.CompilerServices.Unsafe, Version=4.0.3.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. as it doesn't have a reference to System.Runtime.CompilerServices.Unsafe.dll.

Therefore the mechanism where the \lib\ version of the package's dll is copied to @(ReferenceCopyLocalPaths) only happens if the main app references the package directly.

Full build log: https://gist.github.com/VincentDondain/3a067c97e7f034a2a5c542dc6d3d7072

VincentDondain · 2018-03-22T19:10:17Z

Here is a gist where I output the content of different msbuild variables: https://gist.github.com/VincentDondain/38073e31f5bba95e4520bcffa35f03a6. That helps understand the latest commit.

VincentDondain · 2018-03-22T19:11:50Z

Now this doesn't solve part B discussed in my comment here: #3791 (comment) (aka "iOS project references a netstandard class library, which references a Nuget Package (System.Runtime.CompilerServices.Unsafe) but the app does NOT reference that package directly") but it fixes netstandard packages with /lib/ and /ref/ added to the main iOS app project.

spouliot

That makes a lot more sense :)

monojenkins · 2018-03-22T19:20:26Z

Build failure

!!! Couldn't read commit file !!!

monojenkins · 2018-03-22T21:41:26Z

Build success

!!! Couldn't read commit file !!!

rolfbjarne

Would it be possible to add a test for this?

VincentDondain · 2018-03-23T14:19:35Z

@rolfbjarne yes, trying right now to build the test case I attached from our tests but I'm failing on restore the nugget package for now.

VincentDondain · 2018-03-23T18:14:57Z

Ok so it looks like adding a proper msbuild test for this issue is currently not possible (or very very hard / hacky).

See https://bugzilla.xamarin.com/show_bug.cgi?id=53164.

Our msbuild tests are in fact still using xbuild and @radical is trying to move them to msbuild. Because the test case for this issue is using NugetPackages added via "PackageReference" it needs msbuild. xbuild cannot "restore" nor is using the right ResolveAssemblyReferences target (that feeds csc with the right assemblies).

I think we should merge this issue before getting the test (filled issue #3810 to remember we should add it).

spouliot · 2018-03-23T20:17:17Z

@VincentDondain can't you just add the nuget/code ito monotouch-test (not the msbuild tests) until then ? that would fail when we build it for device

VincentDondain · 2018-03-23T20:52:24Z

@spouliot good idea, I just added that with my latest commit. Without this PR's fix we'd get a build error when building monotouch-test.

It's not as if the test itself is testing anything though but it's still a good way to know if we regress.

spouliot · 2018-03-23T21:01:50Z

👍

monojenkins · 2018-03-23T22:55:58Z

Build failure

!!! Couldn't read commit file !!!

The generated makefile that allow us to run tests such as `monotouch-test` is now using msbuild. In addition instead of doing a `/t:Build` it;s making use of msbuild `/restore` target which will automatically restore all NuGet packages and build the project.

monojenkins · 2018-03-26T16:47:23Z

Build failure

!!! Couldn't read commit file !!!

- Removed `new string [] { "Import", "Project" },` which is causing issues when using msbuild (hopefully it's not breaking things since we're mostly using msbuild now).

This reverts commit 3e76335.

This reverts commit 044b34b.

This reverts commit 72a633d.

VincentDondain · 2018-03-27T21:47:25Z

See last comment of this PR #3817 (comment).

I tried to run the tests with msbuild but it is still currently blocked on https://bugzilla.xamarin.com/show_bug.cgi?id=53164.

Without msbuild and /restore we can't restore the NuGet package added via PackageReference therefore we can't have the monotouch-test I tried to add (nor can we have an msbuild test).

In addition to the aforementioned bug we're also tracking the addition of the proper msbuild test for this here: #3810

spouliot

Ok, we'll have to wait for automated testing :(
Thanks for trying against all odds :)

monojenkins · 2018-03-28T00:11:00Z

Build success

!!! Couldn't read commit file !!!

- Fixes issue xamarin#3199: Could not AOT the assembly System.Runtime.CompilerServices.Unsafe.dll (MT3001) (xamarin#3199) - Test case: https://www.dropbox.com/s/kxt3isgzn74nq35/Issue3199.zip?dl=0 ### Problem Given a Nuget Package added via the "package reference" mechanism and the said package having netstandard 'lib' **and** 'ref' folders; MTouch was getting reference assemblies and was trying to AOT them. This resulted in an error as those assemblies are only facades. ### Solution 'ResolveNuGetPackageAssets' computes the NuGet packages using their json asset file, adds the full assemblies to 'ReferenceCopyLocalPaths' and outputs the resolved references via '_ReferencesFromNuGetPackages'. This is exactly what "Microsoft.NuGet.Build.Tasks"'s 'ResolveNuGetPackageAssets' target does.

- Fixes issue #3199: Could not AOT the assembly System.Runtime.CompilerServices.Unsafe.dll (MT3001) (#3199) - Test case: https://www.dropbox.com/s/kxt3isgzn74nq35/Issue3199.zip?dl=0 ### Problem Given a Nuget Package added via the "package reference" mechanism and the said package having netstandard 'lib' **and** 'ref' folders; MTouch was getting reference assemblies and was trying to AOT them. This resulted in an error as those assemblies are only facades. ### Solution 'ResolveNuGetPackageAssets' computes the NuGet packages using their json asset file, adds the full assemblies to 'ReferenceCopyLocalPaths' and outputs the resolved references via '_ReferencesFromNuGetPackages'. This is exactly what "Microsoft.NuGet.Build.Tasks"'s 'ResolveNuGetPackageAssets' target does.

- Fixes xamarin#3810: [msbuild] Add msbuild test for xamarin#3791: Skip reference assemblies passed to mtouch (xamarin#3810) - MSBuild tests now build the iOS and Mac test cases for issue 3199 that's using a nuget package as "PackageReference" which was causing some issues. With the fix disabled we get: ``` Errors and Failures: 1) Test Failure : Xamarin.iOS.Tasks.Issue3199.BuildTest #RunTarget-ErrorCount Could not AOT the assembly '/Users/vidondai/Documents/xi-master/xamarin-macios/msbuild/tests/Issue3199/obj/iPhone/Debug/mtouch-cache/3-Build/System.Runtime.CompilerServices.Unsafe.dll' Expected: 0 But was: 1 ``` and ``` "/Users/vidondai/Documents/xi-master/xamarin-macios/tests/common/mac/TestProjects/Issue3199/Issue3199.csproj" (default target) (1) -> (_CompileToNative target) -> MMP : error MM3001: Could not AOT the assembly '/Users/vidondai/Documents/xi-master/xamarin-macios/tests/common/mac/TestProjects/Issue3199/bin/Debug/Issue3199.app/Contents/MonoBundle/System.Runtime.CompilerServices.Unsafe.dll' [/Users/vidondai/Documents/xi-master/xamarin-macios/tests/common/mac/TestProjects/Issue3199/Issue3199.csproj] 63 Warning(s) 1 Error(s) ```

VincentDondain requested review from emaf, jstedfast, kzu and rolfbjarne as code owners March 20, 2018 21:48

dalexsoto approved these changes Mar 20, 2018

View reviewed changes

spouliot added the do-not-merge Do not merge this pull request label Mar 21, 2018

spouliot suggested changes Mar 21, 2018

View reviewed changes

Bye bye hardcoded path comparison

f095296

Even simpler

fb7b79d

spouliot approved these changes Mar 22, 2018

View reviewed changes

rolfbjarne reviewed Mar 22, 2018

View reviewed changes

spouliot removed the do-not-merge Do not merge this pull request label Mar 23, 2018

VincentDondain mentioned this pull request Mar 23, 2018

[msbuild] Add msbuild test for #3791: Skip reference assemblies passed to mtouch #3810

Closed

[msbuild] Add monotouch-test test

3e76335

VincentDondain mentioned this pull request Mar 26, 2018

[mtouch] Support netstandard library nuget package with reference assembly #3804

Closed

pjcollins mentioned this pull request Mar 26, 2018

[Xamarin.Android.Build.Tasks] Rework how we resolve netstandard Nugets xamarin/xamarin-android#1459

Merged

VincentDondain added 4 commits March 26, 2018 16:08

[xharness] Use msbuild in some places too

72a633d

- Removed `new string [] { "Import", "Project" },` which is causing issues when using msbuild (hopefully it's not breaking things since we're mostly using msbuild now).

Revert "[msbuild] Add monotouch-test test"

6e96017

This reverts commit 3e76335.

Revert "[makefile] Enable msbuild for generated makefiles"

d17b541

This reverts commit 044b34b.

Revert "[xharness] Use msbuild in some places too"

598956e

This reverts commit 72a633d.

spouliot approved these changes Mar 27, 2018

View reviewed changes

VincentDondain merged commit e297e99 into xamarin:master Mar 28, 2018

VincentDondain deleted the issue-3199 branch March 28, 2018 02:39

marcoseraphin mentioned this pull request Apr 2, 2018

.Net Standard 2.0 iOS compiling error - Could not AOT the assembly System.Runtime.CompilerServices.Unsafe.dll (MT3001) realm/realm-dotnet#1711

Closed

davidfowl mentioned this pull request Apr 6, 2018

Xamarin support tracking issue aspnet/SignalR#1886

Closed

VincentDondain mentioned this pull request Oct 29, 2018

[msbuild/tests] Add tests for issue 3199 #5053

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[msbuild] Skip reference assemblies passed to mtouch #3791

[msbuild] Skip reference assemblies passed to mtouch #3791

VincentDondain commented Mar 20, 2018

dalexsoto left a comment

rolfbjarne commented Mar 20, 2018

monojenkins commented Mar 21, 2018

spouliot left a comment •

edited

spouliot Mar 21, 2018

jstedfast commented Mar 21, 2018

VincentDondain commented Mar 21, 2018

spouliot commented Mar 21, 2018

VincentDondain commented Mar 22, 2018

VincentDondain commented Mar 22, 2018

VincentDondain commented Mar 22, 2018 •

edited

spouliot left a comment

monojenkins commented Mar 22, 2018

monojenkins commented Mar 22, 2018

rolfbjarne left a comment

VincentDondain commented Mar 23, 2018

VincentDondain commented Mar 23, 2018

spouliot commented Mar 23, 2018

VincentDondain commented Mar 23, 2018

spouliot commented Mar 23, 2018

monojenkins commented Mar 23, 2018

monojenkins commented Mar 26, 2018

VincentDondain commented Mar 27, 2018

spouliot left a comment

monojenkins commented Mar 28, 2018

[msbuild] Skip reference assemblies passed to mtouch #3791

[msbuild] Skip reference assemblies passed to mtouch #3791

Conversation

VincentDondain commented Mar 20, 2018

Problem

Solution

dalexsoto left a comment

Choose a reason for hiding this comment

rolfbjarne commented Mar 20, 2018

monojenkins commented Mar 21, 2018

spouliot left a comment • edited

Choose a reason for hiding this comment

spouliot Mar 21, 2018

Choose a reason for hiding this comment

jstedfast commented Mar 21, 2018

VincentDondain commented Mar 21, 2018

spouliot commented Mar 21, 2018

VincentDondain commented Mar 22, 2018

VincentDondain commented Mar 22, 2018

VincentDondain commented Mar 22, 2018 • edited

spouliot left a comment

Choose a reason for hiding this comment

monojenkins commented Mar 22, 2018

monojenkins commented Mar 22, 2018

rolfbjarne left a comment

Choose a reason for hiding this comment

VincentDondain commented Mar 23, 2018

VincentDondain commented Mar 23, 2018

spouliot commented Mar 23, 2018

VincentDondain commented Mar 23, 2018

spouliot commented Mar 23, 2018

monojenkins commented Mar 23, 2018

monojenkins commented Mar 26, 2018

VincentDondain commented Mar 27, 2018

spouliot left a comment

Choose a reason for hiding this comment

monojenkins commented Mar 28, 2018

spouliot left a comment •

edited

VincentDondain commented Mar 22, 2018 •

edited