Add conflict resolution #1052

dsplaisted · 2017-03-29T18:06:38Z

Fixes #974. Currently work in progress, see the issue for what's done and what's left to do.

EDIT: This should now be ready for review.

Copied from dotnet/standard repo, commit a6be13f

…e other conflict resolution tests to cover more packages

…item type logic

…e on disk This avoids conflicts being reported in log files when the same FrameworkAssembly reference is declared in multiple NuGet packages

eerhardt · 2017-04-04T13:52:34Z

src/Tasks/Microsoft.NET.Build.Tasks/ConflictResolution/RemoveDepsFileConflicts.cs

+
+namespace Microsoft.NET.Build.Tasks.ConflictResolution
+{
+    public partial class RemoveDepsFileConflicts : Task


I'm not sure if this was in your plan or not, but instead of generating everything into the .deps.json file, and then removing the conflicts in a separate task, we should just be feeding the Conflicts into the original GenerateDepsFile task. Then we can generate the whole thing in one shot.

It makes the logic simpler, and more efficient.

eerhardt · 2017-04-04T13:53:24Z

src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.Packaging.Tools.targets

@@ -0,0 +1,133 @@
+<!--
+***********************************************************************************************
+Microsoft.Packaging.Tools.targets


Is this the best file name for this targets file?

No, in fact it must be changed.

eerhardt · 2017-04-04T13:53:57Z

src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.Packaging.Tools.targets

+      </PropertyGroup>
+    </When>
+    <Otherwise>
+      <!-- NuGet 2, run before targets that consume references -->


I don't think we need to handle NuGet 2 or 3. We will only ever support the NuGet we are using.

eerhardt · 2017-04-04T13:54:41Z

src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.Packaging.Tools.targets

+    <!-- Allow completely disabling the conflict resolution targets-->
+    <When Condition="'$(DisableHandlePackageFileConflicts)' == 'true'" />
+    <!-- Condition here is a hack until https://github.com/dotnet/sdk/issues/534 is fixed -->
+    <When Condition="'$(TargetFramework)' != '' or '$(TargetFrameworks)' != ''">


We shouldn't need this condition/hack. We obviously know that the project is using this SDK, because this file is being loaded from our SDK.

The only thing we need is a good property to understand when the SDK is providing this so that we can use the version from the standard repo in that case (nuget2/nuget3).

eerhardt · 2017-04-04T13:55:24Z

src/Tasks/Microsoft.Packaging.Tools/docs/trimming.md

@@ -0,0 +1,148 @@
+#Dependency Trimming


It was my understanding that trimming was not a 2.0 feature. Should this be removed?

If we are leaving it in, we should have tests for it.

This should not be brought up. @dsplaisted as we mentioned before the trimming stuff should be left in standard repo. Just make sure you are able to support the same sort of hook it requires, which is the same as conflict resolution: items to exclude from copy to output by build and publish and exclude from deps file.

eerhardt · 2017-04-04T13:56:25Z

src/Tasks/Microsoft.Packaging.Tools/pkg/Microsoft.Packaging.Tools.pkgproj

@@ -0,0 +1,20 @@
+<?xml version="1.0" encoding="utf-8"?>


We shouldn't need this file nor the .builds file.

All these targets and tasks should just come with our SDK just like any other targets and task.

Nor the CSProj. I'd expect this all to be added to your existing project.

eerhardt · 2017-04-04T13:56:54Z

src/Tasks/Microsoft.Packaging.Tools/tasks/Trimming/Microsoft.NET.Build.Tasks/LockFileCache.cs

+
+namespace Microsoft.NET.Build.Tasks
+{
+    internal class LockFileCache


We already have this file in our SDK. This can be removed.

eerhardt · 2017-04-04T13:58:51Z

src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.Packaging.Tools.targets

+                              TargetFramework="$(TargetFrameworkMoniker)"
+                              RuntimeIdentifier="$(RuntimeIdentifier)"
+                              PlatformLibraryName="$(MicrosoftNETPlatformLibrary)"
+                              PrivateAssetsPackageReferences="@(PrivateAssetsPackageReference)">


This needs the change I made here: https://github.com/dotnet/standard/pull/283/files

…n't do it if assets file doesn't exist

eerhardt · 2017-04-04T15:46:28Z

test/Microsoft.NET.Publish.Tests/GivenThatWeWantToPublishAHelloWorldProject.cs

+
+        void Conflicts_are_resolved_when_publishing(bool selfContained, [CallerMemberName] string callingMethod = "")
+        {
+            var targetFramework = "netcoreapp2.0";


It would be good to also test "low fat apps" as well. See #1053.

I've tried adding a test for this, and it's getting a bunch of DLLs in the output that shouldn't be copied there. I think it's because the version of the Microsoft.NETCore.App package that we're using (2.0.0-beta-001834-00) has this logic in it:

<ItemGroup Condition="'$(RuntimeIdentifier)' == ''"> <PackageConflictPlatformManifests Include="$(MSBuildThisFileDirectory)Microsoft.NETCore.App.PlatformManifest.txt" /> </ItemGroup>

So the platform manifest isn't getting picked up for rid-specific shared framework apps. Has this been updated in newer versions of the package? What version would have a fix?

@eerhardt @ericstj @weshaggard

I just made a fix for that. dotnet/core-setup#1936

That fix is in 2.0.0-preview1-001913-00.

That build is working its way into the CLI now. @dotnet-bot should be updating dotnet/cli#6238 with the 1913 version soon.

@dsplaisted - there is a new CLI build that contains this fix:

2.0.0-preview1-005722

ericstj · 2017-04-04T18:43:02Z

src/Tasks/Microsoft.Packaging.Tools/tasks/targets/Microsoft.Packaging.Tools.Trimming.targets

@@ -0,0 +1,80 @@
+<?xml version="1.0" encoding="utf-8" ?>


Do not copy this...

ericstj · 2017-04-04T18:47:14Z

src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.Packaging.Tools.targets

+  </PropertyGroup>
+  <Target Name="RemovePublishDepsFileConflicts" AfterTargets="$(RemovePublishDepsFileConflictsAfter)"
+          Inputs="$(PublishDepsFilePath)" Outputs="$(_RemovePublishDepsFileConflictsSemaphore)">
+    <RemoveDepsFileConflicts DepsFilePath="$(PublishDepsFilePath)" Conflicts="@(_ConflictPackageFiles);@(_PublishConflictPackageFiles)" />


We need to kill this target and task and add the proper support into to deps file generation to provide a list of things to omit.

eerhardt · 2017-04-06T20:07:36Z

test/Microsoft.NET.TestFramework/TestAsset.cs

+        <dependency id=""System.Threading.Timer"" version=""4.3.0"" />
+        <dependency id=""System.Xml.ReaderWriter"" version=""4.3.0"" />
+        <dependency id=""System.Xml.XDocument"" version=""4.3.0"" />
+      </group>";


Why hard-code xml here just to parse it below?

It's a copy/paste from the .nuspec. It seems less error prone (and easier) to just leave it in that form.

eerhardt · 2017-04-06T20:10:41Z

src/Tasks/Microsoft.NET.Build.Tasks/GenerateDepsFile.cs

        public ITaskItem[] ReferenceSatellitePaths { get; set; }

+        [Required]
+        public ITaskItem[] FilesToSkip { get; set; }


We should probably have a test that verifies the .deps.json file is trimmed correctly. I don't see one. It could probably go into Microsfot.NET.Build.Tests

eerhardt · 2017-04-06T20:34:59Z

src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.ConflictResolution.targets

+                              TargetFramework="$(TargetFrameworkMoniker)"
+                              RuntimeIdentifier=""
+                              PlatformLibraryName="$(MicrosoftNETPlatformLibrary)"
+                              PrivateAssetsPackageReferences="@(PrivateAssetsPackageReference)">


This should pass in $(IsSelfContained)

eerhardt · 2017-04-06T20:41:21Z

src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.ConflictResolution.targets

+    <HandlePackageFileConflictsDependsOn Condition="'$(SelfContained)' != 'true'">_GetLockFileAssemblies</HandlePackageFileConflictsDependsOn>
+  </PropertyGroup>
+
+  <Target Name="HandlePackageFileConflicts" AfterTargets="ResolvePackageDependenciesForBuild" DependsOnTargets="$(HandlePackageFileConflictsDependsOn)">


Do we want these "Conflicts" Targets to be part of our "public API"?

If not, we should prefix the name with "_".

In reply to: 110262316 [](ancestors = 110262316)

To me it seems to be on the same level as other targets (eg for generating the deps files) that we define.

"Public API" or not I think it's OK to change these if we need to- ie I don't think we need to guarantee compatibility across versions of the SDK for targets that plug into our SDK.

I think these "handle conflicts" targets are implementation details, and shouldn't be part of our public API.

Generating the deps file isn't an implementation detail. The deps file is a user artifact.

I think we need to have an "all up" extension story on what we think we can and can't change across versions of the SDK. The current approach has been to prefix "private" things with _. If that is changing here, we should document it somehow to let people know what our extension story is.

I do think folks will depend on doing stuff after these, so we have to imagine an extension point for "where do I sequence a target to get a filtered list of reference/referencecopylocalpaths/deps-file-items.

I agree with @eerhardt that these should not be public.

They should also not occupying the "good spot" for sequencing. What I mean here is that folks should easily be able to run after them without having ambiguous ordering with these targets themselves.

The names/sequencing they have now are due to me implementing them outside the SDK. Inside the SDK I think they should be rolled into other target sequences.

Here's a straw man:

As a dev extending the SDK I can sequence a target AfterTargets="ResolveReferences" and I am garunteed to get a complete and filtered list of Reference and ReferencePath items.

As a dev extending the SDK I can sequence a target AfterTargets="ResolvePackageDependenciesForBuild" and I am garunteed to get a complete and filtered list of items used to construct the build-time deps file.

As a dev extending the SDK I can sequence a target AfterTargets="RunResolvePublishAssemblies" and I am garunteed to get a complete and filtered list of ResolvedAssembliesToPublish items (not the case today).

As a dev extending the SDK I can sequence a target AfterTargets="ComputeFilesToPublish" and I am garunteed to get a complete and filtered list of ResolvedFileToPublish items, as well as a complete and filtered set of items that will be used to construct the deps file.

Perhaps 1 & 2 and 3 & 4 can be collapsed so long as the items output don't have a relationship (eg one copied to the other).

Moreover, we should have a property that folks can append Targets to so that folks that want to hook this sequence can insert themselves into a deterministic sequence and follow a pattern with common inputs/outputs (a specific set of items) that they can operate on and garuntee those items are the only thing they need to care about updating.

dsplaisted · 2017-04-07T19:51:06Z

@eerhardt @ericstj I've addressed the feedback, and added some tests for the deps.json

…t resolution when RID is specified

dsplaisted · 2017-04-10T07:50:33Z

I've added tests for running an app from the output folder. As @ericstj suspected, running from the output folder when a RID was specified and there were conflicts on runtime items was broken.

I resolved this by switching from using the ResolvePublishAssemblies task in _GetLockFileAssemblies to depending on ComputePrivateAssetsPackageReferences, which gets the files that will be copied to the output folder if CopyLocalLockFileAssemblies is set. This uses the items from the assets file that flow through MSBuild like we have discussed. I don't know that the resulting items will be exactly the same as would come from ResolvePublishAssemblies, but I think that if there are differences it would be that the publish resolver excludes publishing certain files, which we may not want to exclude from conflict resolution anyway.

dsplaisted · 2017-04-10T14:27:49Z

@dotnet-bot test Windows_NT_FullFramework Debug

eerhardt · 2017-04-10T15:28:10Z

src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.Sdk.props

  <PropertyGroup>
    <MSBuildAllProjects>$(MSBuildAllProjects);$(MSBuildThisFileFullPath)</MSBuildAllProjects>
+
+    <!-- This property disables the conflict resolution logic from the Microsoft.Packaging.Tools package, which is superceded by the logic here in the SDK -->


I assume the Packaging.Tools package will change to not be enabled for SDK projects, and then this property can just go away. Do we have a bug logged to track that?

I've filed dotnet/standard#298

ericstj · 2017-04-10T16:47:03Z

src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.ConflictResolution.targets

+
+  <Target Name="_GetLockFileAssemblies"
+          DependsOnTargets="_ComputeLockFileCopyLocal">
+    <!-- We need to find all the files that will be loaded from deps for conflict resolution.


This looks much better. Any reason you didn't use @(AllCopyLocalItems)? Those look like they'll have the NuGetPackageId metadata which HandlePackageFileConflicts depends on.

You can probably eliminate this as a separate target entirely and just have HandlePackageFileConflicts depend on _ComputeLockFileCopyLocal.

The AllCopyLocalItems has the Path metadata set to the path of the file within the package, not the target path it will be copied to

… to avoid conflicts with the version from Microsoft.Packaging.Tools

…-974

…0191030.9 (dotnet#1052) - Microsoft.AspNetCore.Analyzers - 3.1.0-preview3.19530.9 - Microsoft.AspNetCore.Mvc.Api.Analyzers - 3.1.0-preview3.19530.9 - Microsoft.AspNetCore.Mvc.Analyzers - 3.1.0-preview3.19530.9 - Microsoft.AspNetCore.Components.Analyzers - 3.1.0-preview3.19530.9

dnfclas added the cla-already-signed label Mar 29, 2017

dsplaisted added 10 commits April 3, 2017 14:44

Add conflict resolution and trimming tasks

bc7b4b9

Copied from dotnet/standard repo, commit a6be13f

Add conflict resolution tasks to Microsoft.NET.Build.Tasks project

7543da3

Update namespace for conflict resolution

ab15112

Update license header from conflict resolution code to match other files

3bdd8e0

Add conflict resolution .targets file to SDK

ddfd332

Add tests for conflict resolution scenarios

cd23629

Add test for conflict resolution when targeting .NET Framework, updat…

01576d2

…e other conflict resolution tests to cover more packages

Create IConflictItem interface to make ConflictResolver more testable

660bca1

Fix incorrect logic (probably copy/paste error) in conflict resolver …

9c7dae0

…item type logic

Add unit tests for conflict resolver

5d4ecd8

dsplaisted force-pushed the resolve-conflicts-974 branch from 4dbddc2 to 5d4ecd8 Compare April 3, 2017 23:55

Don't report a conflict between two items that don't resolve to a fil…

1e9d2cf

…e on disk This avoids conflicts being reported in log files when the same FrameworkAssembly reference is declared in multiple NuGet packages

dsplaisted mentioned this pull request Apr 4, 2017

RemoveDepsFileConflicts errors when using .NET Core 2.0 #1043

Closed

eerhardt reviewed Apr 4, 2017

View reviewed changes

dsplaisted added 2 commits April 4, 2017 08:14

Make conflict resolution strings localizable

e2debd1

Fix test failures: Don't resolve publish assemblies for a RID, and do…

0157d3f

…n't do it if assets file doesn't exist

dsplaisted force-pushed the resolve-conflicts-974 branch from d45ca29 to 0157d3f Compare April 4, 2017 15:15

Skip test asset path length check on non-Windows

0937875

dsplaisted force-pushed the resolve-conflicts-974 branch from 8f7848e to 0937875 Compare April 4, 2017 15:39

eerhardt reviewed Apr 4, 2017

View reviewed changes

ericstj reviewed Apr 4, 2017

View reviewed changes

eerhardt reviewed Apr 6, 2017

View reviewed changes

dsplaisted added 5 commits April 6, 2017 14:30

Consolidate NuGetUtils and NuGetUtilities classes

5bc3735

Consolidate ITaskItemExtensions and ItemUtilities

b3712b5

Apply code review feedback

ea4431a

Disable conflict resolution logic from Microsoft.Packaging.Tools package

d525ece

Add tests for deps.json conflict removal

4b04b4e

dsplaisted added 3 commits April 7, 2017 14:25

Disable test on full framework until dotnet#1077 is fixed

5e4ca3a

Remove out of date comments about how to disable conflict resolution

d43b9a7

Add tests for running app from output folder, fix issues with conflic…

1ab0fb7

…t resolution when RID is specified

eerhardt reviewed Apr 10, 2017

View reviewed changes

eerhardt approved these changes Apr 10, 2017

View reviewed changes

ericstj reviewed Apr 10, 2017

View reviewed changes

dsplaisted added 5 commits April 10, 2017 17:42

Make conflict resolution targets private

09c45a6

Don't use Path metadata for destination path in conflict resolution

35e681b

The AllCopyLocalItems has the Path metadata set to the path of the file within the package, not the target path it will be copied to

Rename HandlePackageFileConflicts task to ResolvePackageFileConflicts…

da7ad2e

… to avoid conflicts with the version from Microsoft.Packaging.Tools

Merge remote-tracking branch 'upstream/master' into resolve-conflicts…

77c53df

…-974

Update test merged from master to work with bundled .NET Core version

517b4d5

This was referenced Apr 11, 2017

Remove conflict resolution from NETStandard.Library package closure dotnet/standard#298

Closed

After conflict resolution, remove libraries from the deps file that no longer include any assets #1109

Closed

dsplaisted merged commit 1d81050 into dotnet:master Apr 11, 2017

dsplaisted mentioned this pull request Apr 11, 2017

Update SDK to 2.0.0-alpha-20170411-3 dotnet/cli#6304

Merged

dsplaisted deleted the resolve-conflicts-974 branch April 12, 2017 00:45

dsplaisted mentioned this pull request Apr 13, 2017

Apply conflict resolution to RuntimeTargets items #1121

Merged

Add conflict resolution #1052

Add conflict resolution #1052

Uh oh!

Conversation

dsplaisted commented Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsplaisted commented Apr 7, 2017

Uh oh!

dsplaisted commented Apr 10, 2017

Uh oh!

dsplaisted commented Apr 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dsplaisted commented Mar 29, 2017 •

edited

Loading

eerhardt Apr 7, 2017 •

edited

Loading