[Xamarin.Android.Build.Tasks] Deploy changes to transitive reference assembly dependencies #4887
Conversation
brendanzagaeski
requested review from
dellis1972,
jonathanpeppers and
jonpryor
as code owners
There was a problem hiding this comment.
There is one test failure, that seems like this change caused it?
Xamarin.Android.Build.Tests.BuildTest.BuildWithResolveAssembliesFailure(False)
Should recieve 'error XA2002: Can not resolve reference:' regarding Microsoft.Azure.Amqp!
Expected: True
But was: False
Maybe some assertion needs updated:
"/Users/runner/runners/2.171.1/work/1/s/bin/TestRelease/temp/BuildWithResolveAssembliesFailure(False)/MyApp/MyApp.csproj" (Build;SignAndroidPackage target) (1) ->
(_ResolveAssemblies target) ->
/Library/Frameworks/Mono.framework/External/xbuild/Xamarin/Android/Xamarin.Android.Legacy.targets(234,5): error XA2002: Can not resolve reference: `Microsoft.Azure.Amqp`, referenced by `Microsoft.Azure.EventHubs`. Please add a NuGet package or assembly reference for `Microsoft.Azure.Amqp`, or remove the reference to `Microsoft.Azure.EventHubs`. [/Users/runner/runners/2.171.1/work/1/s/bin/TestRelease/temp/BuildWithResolveAssembliesFailure(False)/MyApp/MyApp.csproj]
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/IncrementalBuildTest.cs
Outdated
Show resolved
Hide resolved
jonathanpeppers
force-pushed
the
transitive-refasm
branch
from
d39e374 to
0b0f887
Compare
|
I changed: |
|
@brendanzagaeski: Looks like this (somehow) broke satellite assembly use? https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=3894877&view=ms.vss-test-web.build-test-results-tab&runId=14457216&resultId=100003&paneView=debug |
|
Shoot, yup. Looking into that satellite assembly behavior now. |
brendanzagaeski
force-pushed
the
transitive-refasm
branch
from
0b0f887 to
7532b2a
Compare
|
Changes in the latest force-push: To fix the broken + <FilteredAssemblies
+ Include="@(ReferenceDependencyPaths)"
+ Condition="'%(ReferenceDependencyPaths.ResolvedFrom)' != 'ImplicitlyExpandDesignTimeFacades' And Exists('%(ReferenceDependencyPaths.Identity)') "
+ />Update the commit message accordingly: -To fix this behavior, include more of the items from
-`@(ReferenceCopyLocalPaths)` in the `@(FilteredAssemblies)` input to
-`<ResolveAssemblies/>` by removing the
-`'%(ReferenceCopyLocalPaths.RelativeDir)' == ''` condition.
+To fix this behavior, add the `@(ReferenceDependencyPaths)` items that
+are found by the `<ResolveAssemblyReference/>` task to the
+`@(FilteredAssemblies)` input.Explanation: Having satellite assemblies included in the input to Longer explanation: After the original fix, two satellite assemblies were being passed as additional inputs to
As a consequence, And then the linking step created: After that, the (This is part of the problem happens with non-satellite assemblies too, as a user reported a little while back: #3745.) Finally, when the APK included the incorrect |
| } | ||
| }; | ||
| lib2.SetProperty ("ProduceReferenceAssembly", "True"); | ||
| lib1.OtherBuildItems.Add (new BuildItem.ProjectReference ($"..\\{lib2.ProjectName}\\{lib2.ProjectName}.csproj", lib2.ProjectName, lib2.ProjectGuid)); |
There was a problem hiding this comment.
Why does this use lib1.OtherBuildItems and not lib1.References?
There was a problem hiding this comment.
Oh yeah, that's a little tricky thing I learned when setting up this test. What happens is that XmlUtils.ToXml() adds only OtherBuildItems in projects that have $(EnableDefaultItems)=true:
And DotNetStandard has $(EnableDefaultItems)=true:
|
brendanzagaeski commented on Jul 16 #4887 (comment)
I got a chance to dig into this more and found that the difference for the failing APKs was related to |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| <FilteredAssemblies | ||
| Include="@(ReferenceDependencyPaths)" | ||
| Condition="'%(ReferenceDependencyPaths.ResolvedFrom)' != 'ImplicitlyExpandDesignTimeFacades' And Exists('%(ReferenceDependencyPaths.Identity)') " | ||
| /> |
There was a problem hiding this comment.
I think this needs to use @(_ReferenceDependencyPaths):
This way we will include the %(AndroidSkip...) metadata, it matches the use of @(_ReferencePath) below.
There was a problem hiding this comment.
@brendanzagaeski I think with this change ^ we can probably merge this. Its looking good.
There was a problem hiding this comment.
Ooo, nice catch. I've added that change now.
…assembly dependencies Fixes: https://developercommunity.visualstudio.com/content/problem/1086457/index.html Context: 8f2ae24 Context: https://dev.azure.com/devdiv/DevDiv/_workitems/edit/938468 If a Xamarin.Android app referenced a .NET Standard library that had `$(ProduceReferenceAssembly)`=`true`, and that library in turn referenced a second .NET Standard library, then changes to the second library would not be deployed during incremental deployments in Visual Studio unless the app project also directly referenced the second library. For additional context, note that as discussed in internal work item 938468, if a .NET Framework console application references a .NET Standard library that has `$(ProduceReferenceAssembly)`=`true`, then changes to the library are similarly missing in the running console app after incremental builds in Visual Studio. Although the problem in Xamarin.Android apps was similar to the .NET Framework behavior, an important difference was that Xamarin.Android apps did reflect changes correctly for libraries referenced directly by the app. Only transitive references caused trouble in Xamarin.Android. The reason direct references worked was that Visual Studio always ran Xamarin.Android's `Install` target on each incremental deployment, and the `<ResolveAssemblies/>` task in that target correctly located the up-to-date assemblies for direct references. For example, for the included build test, the `<ResolveAssemblies/>` task resolved the `Library1` reference to: C:\Source\xamarin-android\bin\TestDebug\temp\TransitiveDependencyProduceReferenceAssembly\Library1\bin\Debug\netstandard2.0\Library1.dll In contrast, the `<ResolveAssemblies/>` task resolved the `Library2` reference to: C:\Source\xamarin-android\bin\TestDebug\temp\TransitiveDependencyProduceReferenceAssembly\App\bin\Debug\Library2.dll This was problematic because that copy of the library was left over from the previous deployment. In a sense, this was a circular reference: The input for the current deployment was the output from the previous deployment. To fix this behavior, add the `@(ReferenceDependencyPaths)` items that are found by the `<ResolveAssemblyReference/>` task to the `@(FilteredAssemblies)` input. For the included build test, this means the following additional item is now included in `@(FilteredAssemblies)`: C:\Source\xamarin-android\bin\TestDebug\temp\TransitiveDependencyProduceReferenceAssembly\Library1\bin\Debug\netstandard2.0\Library2.dll This path is then treated as a top-level reference in `<ResolveAssemblies/>`, so the task no longer needs to try to resolve it from elsewhere. In theory, another way to fix the problem could be to change how `<ResolveAssemblies/>` resolves transitive dependencies, but `<ResolveAssemblies/>` is only used in the legacy build targets, so adding more sophistication now would have limited value. Other changes: * The `BuildWithResolveAssembliesFailure` test using `packages.config` gets a slightly different error message due to the head project knowing about `Microsoft.Azure.EventHubs`. I think the new behavior is OK. * `@(FilteredAssemblies)` can now have duplicates, so we need to use `->Distinct()`. Note that MSBuild preserves ordering: https://github.com/microsoft/msbuild/blob/9c2d922959e80ba72fefb7addec21ede227639c3/src/Build/Evaluation/Expander.cs#L2484-L2513 Co-authored-by: Jonathan Peppers <jonathan.peppers@gmail.com>
brendanzagaeski
force-pushed
the
transitive-refasm
branch
from
a5c40c2 to
4647127
Compare
|
Changes in the latest force-push: Change |
|
Something weird happened when I merged this... GitHub failed, then displayed a retry button. When I clicked retry, it lost what I edited in the commit message? GitHub suggested this to me twice, and I had removed the second one... But then, this was still in the final commit. |
Fixes: https://developercommunity.visualstudio.com/content/problem/1086457/index.html
Context: 8f2ae24
Context: https://dev.azure.com/devdiv/DevDiv/_workitems/edit/938468
If a Xamarin.Android app referenced a .NET Standard library that had
$(ProduceReferenceAssembly)=true, and that library in turnreferenced a second .NET Standard library, then changes to the second
library would not be deployed during incremental deployments in Visual
Studio unless the app project also directly referenced the second
library.
For additional context, note that as discussed in internal work item
938468, if a .NET Framework console application references a .NET
Standard library that has
$(ProduceReferenceAssembly)=true, thenchanges to the library are similarly missing in the running console app
after incremental builds in Visual Studio.
Although the problem in Xamarin.Android apps was similar to the .NET
Framework behavior, an important difference was that Xamarin.Android
apps did reflect changes correctly for libraries referenced directly by
the app. Only transitive references caused trouble in Xamarin.Android.
The reason direct references worked was that Visual Studio always ran
Xamarin.Android's
Installtarget on each incremental deployment, andthe
<ResolveAssemblies/>task in that target correctly located theup-to-date assemblies for direct references.
For example, for the included build test, the
<ResolveAssemblies/>task resolved the
Library1reference to:In contrast, the
<ResolveAssemblies/>task resolved theLibrary2reference to:
This was problematic because that copy of the library was left over from
the previous deployment. In a sense, this was a circular reference: The
input for the current deployment was the output from the previous
deployment.
To fix this behavior, include more of the items from
@(ReferenceCopyLocalPaths)in the@(FilteredAssemblies)input to<ResolveAssemblies/>by removing the'%(ReferenceCopyLocalPaths.RelativeDir)' == ''condition.For the included build test, this means the following additional item is
now included in
@(FilteredAssemblies):This path is then treated as a top-level reference in
<ResolveAssemblies/>, so the task no longer needs to try to resolve itfrom elsewhere.
In theory, another way to fix the problem could be to change how
<ResolveAssemblies/>resolves transitive dependencies, but<ResolveAssemblies/>is only used in the legacy build targets, soadding more sophistication now would have limited value.