In conflict resolution, treat items from FrameworkList.xml as platform assemblies #1618

dsplaisted · 2017-09-30T01:24:09Z

Tests still to come (though they will probably depend on the .NET 4.7.1 targeting pack, so they'll have to be disabled until it is released).

@ericstj @AlexGhiondea @nguerrera @AndyGerlicher @cdmihai for review

…m assemblies

nguerrera · 2017-09-30T02:21:42Z

src/Tasks/Common/src/ConflictResolution/FrameworkListReader.cs

+                Version assemblyVersion;
+                if (string.IsNullOrEmpty(assemblyVersionString) || !Version.TryParse(assemblyVersionString, out assemblyVersion))
+                {
+                    string errorMessage = string.Format(CultureInfo.InvariantCulture, Strings.ErrorParsingFrameworkListInvalidValue,


Messages shown to the user should be formatted with current culture

nguerrera · 2017-09-30T02:23:38Z

src/Tasks/Common/src/ConflictResolution/FrameworkListReader.cs

+            }
+
+            var frameworkList = XDocument.Load(frameworkListPath);
+            foreach (var file in frameworkList.Elements("File"))


Some framework lists chain to others. Critically, I believe Mono's are that way and this will need to work when targeting net471 with Mono msbuild.

I took a look at this and it turns out that the GetReferenceAssemblyPaths target takes care of this. If there are chained framework lists, it will set TargetFrameworkDirectory to a semicolon-separated list of the chained folders. For example:

C:\Program Files %28x86%29\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\ReferenceAssemblies\Microsoft\Framework\MonoAndroid\v6.0;C:\Program Files %28x86%29\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\ReferenceAssemblies\Microsoft\Framework\MonoAndroid\v5.1;C:\Program Files %28x86%29\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\ReferenceAssemblies\Microsoft\Framework\MonoAndroid\v5.0;C:\Program Files %28x86%29\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\ReferenceAssemblies\Microsoft\Framework\MonoAndroid\v4.4.87;C:\Program Files %28x86%29\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\ReferenceAssemblies\Microsoft\Framework\MonoAndroid\v4.4;C:\Program Files %28x86%29\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\ReferenceAssemblies\Microsoft\Framework\MonoAndroid\v4.3;C:\Program Files %28x86%29\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\ReferenceAssemblies\Microsoft\Framework\MonoAndroid\v4.2;C:\Program Files %28x86%29\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\ReferenceAssemblies\Microsoft\Framework\MonoAndroid\v4.1;C:\Program Files %28x86%29\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\ReferenceAssemblies\Microsoft\Framework\MonoAndroid\v4.0.3;C:\Program Files %28x86%29\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\ReferenceAssemblies\Microsoft\Framework\MonoAndroid\v2.3;C:\Program Files %28x86%29\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\ReferenceAssemblies\Microsoft\Framework\MonoAndroid\v1.0;;;C:\Program Files %28x86%29\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\ReferenceAssemblies\Microsoft\Framework\MonoAndroid\v1.0\Facades\

However, the FrameworkList.xml files for Mono don't list any File entries. So I think we will need to scan the directory for assemblies to treat as platform assemblies if we don't find any File items listed in the FrameworkList.xml.

That could get expensive, if you do need to do that make sure you don't crack the assemblies at that point but instead create lazy items that point to the TargetFrameworkDirectory location.

I don't suppose MSBuild has an API built on top of their framework list / metadata cache?

@ericstj Yes, that's the way I would do it. I'm not sure at this point whether we need this to apply to Xamarin/Mono.

The MSBuild API (the RedistList class) isn't public and doesn't have any way to get a list of the framework assemblies, only to check if a given assembly name is part of the framework. We could make an API for this public if we have to, but so far it doesn't look like we do.

@akoeplinger @DustinCampbell

nguerrera · 2017-09-30T02:27:22Z

src/Tasks/Common/src/ConflictResolution/FrameworkListReader.cs

+                yield return new ConflictItem(assemblyName + ".dll",
+                                                packageId: null,
+                                                assemblyVersion: assemblyVersion,
+                                                fileVersion: null);


Extra whitespace

… 4.7.1

dsplaisted · 2017-10-03T14:56:22Z

@Pilchie for approval

Customer scenario

.NET Framework 4.7.1 projects which reference libraries targeting .NET Standard 1.x

Bugs this fixes

#1336

Workarounds, if any

n/a

Risk

Medium

Performance impact

TBD

Root cause analysis

.NET 4.7.1 ships with .NET Standard Facades in box, and as part of the reference assemblies. This requires some additional logic in order for the in-box versions to override the ones coming from NuGet.

How was the bug found?

Internal design

Pilchie · 2017-10-03T15:01:46Z

What is the state of the testing mentioned above?

Additionally, what can be done to mitigate performance concerns?

dsplaisted · 2017-10-03T18:57:52Z

@Pilchie The PR includes tests, which are disabled for now because we don't have the .NET 4.7.1 targeting pack on the CI machines. I've run the tests locally and filed #1625 to enable them later.

As for perf, we could make a process-wide cache of the data we are reading, which is what MSBuild does. We could also update MSBuild to provide an API to get the list of assemblies in the framework along with the versions. MSBuild already reads and caches this file so that solution would have very little overhead.

Pilchie · 2017-10-03T22:17:05Z

I'm going to need some code reviewers to sign off on this before I can approve an escalate.

Pilchie · 2017-10-03T22:17:54Z

src/Tasks/Common/src/ConflictResolution/FrameworkListReader.cs

+            {
+                //  This is not an error, as we get both the root target framework directory as well as the Facades folder passed in as TargetFrameworkDirectories.
+                //  Only the root will have a RedistList\FrameworkList.xml in it
+                yield break;


Consider not using a yield enumerator if this is a hot path where allocations matter.

Pilchie · 2017-10-03T22:18:40Z

src/Tasks/Common/src/ConflictResolution/ResolvePackageFileConflicts.cs

+            //  Also treat assemblies from FrameworkList.xml as platform assemblies
+            if (TargetFrameworkDirectories != null && TargetFrameworkDirectories.Any())
+            {
+                platformItems = platformItems.Concat(TargetFrameworkDirectories.SelectMany(tfd =>


Consider not using SelectMany + Concat if this is a hot path where allocations matter.

nguerrera

Please file a bug to handle the Mono case. (I know you have offline discussions going on that, but it would be good to have a bug tracking what isn't handled in initial commit.)

dsplaisted · 2017-10-03T22:37:43Z

@nguerrera I've filed #1627

nguerrera · 2017-10-09T17:16:19Z

Why did this get closed?

dsplaisted · 2017-10-10T20:31:44Z

Why did this get closed?

Good question... I guess I misclicked (or maybe my daughter did :-) )

dsplaisted · 2017-10-10T23:39:14Z

@MattGertz for approval. Ask mode template is here: #1618 (comment)

…K project references SDK project

MattGertz · 2017-10-11T04:30:52Z

We are in Escrow. Please migrate to VSO bug -- thanks!

livarcocc · 2017-10-11T17:48:29Z

@MattGertz, see https://devdiv.visualstudio.com/DevDiv/_queries?id=503749&_a=edit&triage=true.

MattGertz · 2017-10-11T18:04:01Z

Approved from my side. Escrow technically starts at 5PM tonight, but we likely can't get it in before then due to turnaround on signing, so I've submitted for Escrow in VSO.

dsplaisted · 2017-10-11T22:26:12Z

@dotnet-bot test OSX10.12 Debug

dsplaisted · 2017-10-11T22:26:26Z

@dotnet-bot test OSX10.12 Release

…ion scope

nguerrera · 2017-10-10T23:04:19Z

test/Microsoft.NET.Build.Tests/GivenThatWeWantToTargetNet471.cs

@@ -0,0 +1,124 @@
+using FluentAssertions;


missing copyright header

ericstj · 2017-10-17T00:05:51Z

src/Tasks/Common/src/ConflictResolution/ResolvePackageFileConflicts.cs

+
+                compilePlatformItems = TargetFrameworkDirectories.SelectMany(tfd =>
+                {
+                    return frameworkListReader.GetConflictItems(Path.Combine(tfd.ItemSpec, "RedistList", "FrameworkList.xml"), log);


Return here is confusing, can you eliminate the curly braces?

With dotnet#1618, conflict resolution now depends on TargetFrameworkDirectory property, and the GetReferenceAssemblyPaths target to set it. However, the custom FilterCopyLocal target in our projects was causing GetReferenceAssemblyPaths to run before GetFrameworkPaths, and when GetFrameworkPaths was subsequently run, it reset the TargetFrameworkDirectory property to v4.0.30319. This change adds a dependency to PrepareForBuild so that both targets run in the right order.

…df6-aca1-e570af2cc4d4 [release/6.x] Update dependencies from dotnet/roslyn

dsplaisted added 2 commits September 29, 2017 18:16

Add additional string for FrameworkList parsing

57154b9

In conflict resolution, treat items from FrameworkList.xml as platfor…

95c8f54

…m assemblies

dnfclas added the cla-already-signed label Sep 30, 2017

nguerrera reviewed Sep 30, 2017

View reviewed changes

dsplaisted added 2 commits October 2, 2017 13:12

Use current culture for message formatting

9a74577

Fix issue reading FrameworkList.xml, and add tests for targeting .NET…

d071b0b

… 4.7.1

Pilchie reviewed Oct 3, 2017

View reviewed changes

dsplaisted mentioned this pull request Oct 3, 2017

Verify whether build logic for .NET Standard support needs to be added to Mono/Xamarin #1627

Closed

nguerrera approved these changes Oct 3, 2017

View reviewed changes

livarcocc added this to the 15.5 milestone Oct 5, 2017

dsplaisted closed this Oct 8, 2017

dsplaisted reopened this Oct 10, 2017

dnfclas added the cla-already-signed label Oct 10, 2017

dsplaisted added 2 commits October 10, 2017 15:02

Merge upstream changes

227756b

Cache results of reading framework list, apply code review feedback

0204e27

Fix issue with conflicting types for framework list cache when non-SD…

06a34c6

…K project references SDK project

MattGertz added the Pending Shiproom Approval label Oct 11, 2017

Update test to handle System.Net.Http and System.IO.Compression facades

0a5f0f7

Include items from FrameworkList.xml in compile-time conflict resolut…

3c1307d

…ion scope

livarcocc added Approved to merge and removed Pending Shiproom Approval labels Oct 11, 2017

Update .NET 4.7.1 tests to ensure RAR doesn't find conflicts

09606bc

nguerrera approved these changes Oct 12, 2017

View reviewed changes

Add copyright header

8057f42

dsplaisted merged commit 9e7c61c into dotnet:release/15.5 Oct 12, 2017

ericstj reviewed Oct 17, 2017

View reviewed changes

dsplaisted mentioned this pull request Oct 19, 2017

Filtering CopyLocalLockFileAssemblies #933

Closed

nguerrera mentioned this pull request Nov 14, 2017

Latest SDK cannot pack itself #1730

Closed

nguerrera mentioned this pull request Apr 18, 2018

ResolveAssemblyReference is incorrectly marking references from NuGet packages as referenceassembly #1738

Closed

JL03-Yue pushed a commit that referenced this pull request Mar 19, 2024

Merge pull request #1618 from dotnet/darc-release/6.x-20dd11e9-db29-4…

1077418

…df6-aca1-e570af2cc4d4 [release/6.x] Update dependencies from dotnet/roslyn

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

In conflict resolution, treat items from FrameworkList.xml as platform assemblies #1618

In conflict resolution, treat items from FrameworkList.xml as platform assemblies #1618

dsplaisted commented Sep 30, 2017

nguerrera Sep 30, 2017

nguerrera Sep 30, 2017

dsplaisted Oct 2, 2017

ericstj Oct 2, 2017

dsplaisted Oct 2, 2017

nguerrera Oct 2, 2017

nguerrera Sep 30, 2017

dsplaisted commented Oct 3, 2017

Pilchie commented Oct 3, 2017

dsplaisted commented Oct 3, 2017

Pilchie commented Oct 3, 2017

Pilchie Oct 3, 2017

Pilchie Oct 3, 2017

nguerrera left a comment

dsplaisted commented Oct 3, 2017

nguerrera commented Oct 9, 2017

dsplaisted commented Oct 10, 2017

dsplaisted commented Oct 10, 2017

MattGertz commented Oct 11, 2017

livarcocc commented Oct 11, 2017

MattGertz commented Oct 11, 2017

dsplaisted commented Oct 11, 2017

dsplaisted commented Oct 11, 2017

nguerrera Oct 10, 2017

ericstj Oct 17, 2017

In conflict resolution, treat items from FrameworkList.xml as platform assemblies #1618

In conflict resolution, treat items from FrameworkList.xml as platform assemblies #1618

Conversation

dsplaisted commented Sep 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsplaisted commented Oct 3, 2017

Pilchie commented Oct 3, 2017

dsplaisted commented Oct 3, 2017

Pilchie commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguerrera left a comment

Choose a reason for hiding this comment

dsplaisted commented Oct 3, 2017

nguerrera commented Oct 9, 2017

dsplaisted commented Oct 10, 2017

dsplaisted commented Oct 10, 2017

MattGertz commented Oct 11, 2017

livarcocc commented Oct 11, 2017

MattGertz commented Oct 11, 2017

dsplaisted commented Oct 11, 2017

dsplaisted commented Oct 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment