Apply conflict resolution to RuntimeTargets items #1121

dsplaisted · 2017-04-13T14:55:29Z

RuntimeTargets items were not being included in conflict resolution, leading to failures when trying to use APIs from packages such as System.Diagnostics.TraceSource. This PR now includes these items in the in the OtherRuntimeItems parameter passed to ResolvePackageFileConflicts task.

I'm also including an update to .gitignore to exclude .binlog files, which is MSBuild's new binary logging format.

cc @ericstj @eerhardt

…kageFileConflicts

… assets

…gitignore

ericstj · 2017-04-13T15:08:32Z

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


    <ItemGroup>
      <!-- We need to find all the files that will be loaded from deps for conflict resolution.
          To do this, we look at the files that would be copied local when CopyLocalLockFileAssemblies is true


Can you update this comment?

ericstj · 2017-04-13T15:13:30Z

It's curious to me that building with RuntimeIdentifier=<rid> and CopyLocalLockFileAssemblies=true wouldn't include the RID-specific assets. That feels like a bug.

ericstj · 2017-04-13T15:15:22Z

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

      <_LockFileAssemblies Include="@(AllCopyLocalItems->WithMetadataValue('Type', 'assembly'))" />
+
+      <!--RuntimeTarget-->
+      <_RuntimeTargetItems Include="@(_ActiveTFMFileDependencies->WithMetadataValue('FileGroup', 'RuntimeTarget'))" />


Not sure about the uniqueness of these item names, but do a scrub to make sure there are no conflicts with anything.

Scrub within the context of the SDK or are you saying I should look elsewhere too? If so, where?

Just sdk is fine. I think we should probably have some naming convention for private items to help avoid conflicts but it doesn't make sense to start establishing that in this change.

ericstj

Looks ok if it fixes the problem, but you may want to understand why the copy-local items were missing RID specific assets when building with a RID. That sounds like it could impact other frameworks that use Build rather than publish to produce a flat runnable output (eg: desktop, UAP).

dsplaisted · 2017-04-13T15:22:26Z

It's curious to me that building with RuntimeIdentifier= and CopyLocalLockFileAssemblies=true wouldn't include the RID-specific assets. That feels like a bug.

By default, CopyLocalLockFileAssemblies is only true when targeting something besides .NET Core or .NET Standard. The other frameworks (primarily .NET Framework) don't support loading architecture-specific assets at runtime like this. So it looks like it usually won't matter that they aren't copied locally. On the other hand it seems like it wouldn't hurt to copy them and might make CopyLocalLockFileAssemblies work better as a "mini-publish" or something for .NET Core. Perhaps @eerhardt can provide some feedback or insight.

ericstj · 2017-04-13T15:25:08Z

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

      <!-- We need to find all the files that will be loaded from deps for conflict resolution.
          To do this, we look at the files that would be copied local when CopyLocalLockFileAssemblies is true
          -->
      <_LockFileAssemblies Include="@(AllCopyLocalItems->WithMetadataValue('Type', 'assembly'))" />


Existing issue, but we also need to do conflict resolution of native assets. Is that covered by this?

It looks like it is. NuGet.ProjectModel is returning "assembly" for the Type property for this entry in the assets file:

"runtime.debian.8-x64.runtime.native.System.Security.Cryptography.OpenSsl/4.3.0": { "type": "package", "runtimeTargets": { "runtimes/debian.8-x64/native/System.Security.Cryptography.Native.OpenSsl.so": { "assetType": "native", "rid": "debian.8-x64" } } },

So when we get to this line, it is included in what we pass to the conflict resolution.

interesting, I guess the deps file is still correct since that gets copied from assets but this is something @eerhardt might hit if we tried to reconstruct the deps file from these items.

eerhardt · 2017-04-13T15:58:07Z

test/Microsoft.NET.TestFramework/TestAsset.cs

+            }
+        }
+
+        public static string ConflictResolutionTestMethod


This seems like a pretty specific thing to be on the general TestAsset. The above method went from something that was more general "netstandard1.3. stuff", to "ConflictResolution"-specific.

Is there a better place for this conflict resolution stuff than on the general TestAsset?

eerhardt · 2017-04-13T16:38:21Z

It's curious to me that building with RuntimeIdentifier= and CopyLocalLockFileAssemblies=true wouldn't include the RID-specific assets. That feels like a bug.

By default, CopyLocalLockFileAssemblies is only true when targeting something besides .NET Core or .NET Standard. The other frameworks (primarily .NET Framework) don't support loading architecture-specific assets at runtime like this. So it looks like it usually won't matter that they aren't copied locally. On the other hand it seems like it wouldn't hurt to copy them and might make CopyLocalLockFileAssemblies work better as a "mini-publish" or something for .NET Core. Perhaps @eerhardt can provide some feedback or insight.

Native assets + RuntimeIdentifier="" don't work at all for the .NET Framework. Say you have libuv.dll built for both x86 and x64, where are you going to copy these two assemblies to? You can't copy them both to the same output directory since they have the same file name. You can't copy one to an x86\libuv.dll and one to x64\libuv.dll because the .NET Framework runtime doesn't know how to load native assemblies out of these folders. You can't rename the file, or else you will need to change your [DllImport] attributes to the renamed file.

This is why we have the following logic: https://github.com/dotnet/sdk/blob/master/src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.RuntimeIdentifierInference.targets#L18-L100 and https://github.com/dotnet/sdk/blob/master/src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.RuntimeIdentifierInference.targets#L138-L151 which infers a RuntimeIdentifier on .NET Framework when using native assets.

On .NET Core, we have the .deps.json file, that tells the host about these native assets, so it knows that if I'm running x86, to find the file under runtimes\x86\libuv.dll.

/cc @nguerrera who did this inference logic in SDK 1.0 for .NET Framework.

eerhardt · 2017-04-13T16:41:13Z

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

+      <_RuntimeTargetItems Include="@(_ActiveTFMFileDependencies->WithMetadataValue('FileGroup', 'RuntimeTarget'))" />
+      <__RuntimeTargetPublishItems Include="@(FileDefinitions)" Exclude="@(_RuntimeTargetItems)" />
+      <_RuntimeTargetPublishItems Include="@(FileDefinitions)" Exclude="@(__RuntimeTargetPublishItems)" />
+      <!--<RuntimeTargetPublishItems Include="%(_RuntimeTargetPublishItems.ResolvedPath)" />-->


Can we remove this comment? It seems unnecessary.

ericstj · 2017-04-13T17:00:53Z

Native assets + RuntimeIdentifier="" don't work at all for the .NET Framework

Yeah understood. I thought the issue here was native issues with RID-set, I used <rid> in the comment but GitHub omitted that because i didn't escape the brackets. Simple misunderstanding. Carry on.

eerhardt · 2017-04-13T17:10:52Z

I thought the issue here was native issues with RID-set, I used

Just an FYI for everyone, RuntimeTargets are what is used for native/RID-specific assets in the RID-less section of the NuGet assets file. They don't appear in a RID-specific section, as far as I know, because then it would just be a regular "runtime" or "native" asset.

ericstj · 2017-04-13T17:13:28Z

Yep, understood.

dsplaisted · 2017-04-13T19:26:45Z

@ManishJayaswal for approval.

ManishJayaswal · 2017-04-13T19:31:40Z

@dsplaisted @livarcocc ask mode template?

dsplaisted · 2017-04-13T20:09:35Z

@ManishJayaswal Here you go:

Customer scenario

Target .NET Core 2.0 or .NET Standard 2.0 and use NuGet packages that target 1.x versions of .NET Core or .NET Standard

Bugs this fixes:

n/a

Workarounds, if any

n/a

Risk

Low - this includes an additional type of item in conflict resolution, which had been included before I switched how these items were collected.

Performance impact

Low - processing some additional items which were already processed in a previous version of the code.

Is this a regression from a previous update?

It's a regression introduced in #1052, to functionality that hasn't shipped yet.

Root cause analysis:

#1052 switched to a cleaner way to get the runtime items for conflict resolution (using items already flowing through MSBuild instead of running a "fake" publish task), but the new logic didn't include items of type RuntimeTarget.

How was the bug found?

Failures when inserting updated SDK into .NET CLI repo: dotnet/cli#6304

…121.10 (dotnet#1121) - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.19571.10

dsplaisted added 3 commits April 13, 2017 07:11

Include RuntimeTarget items in OtherRuntimeItems passed to ResolvePac…

20d6230

…kageFileConflicts

Update conflict resolution tests to cover package with runtimeTargets…

0071f52

… assets

Add .binlog extension (which is MSBuild's new binary log format) to .…

176e559

…gitignore

dnfclas added the cla-already-signed label Apr 13, 2017

ericstj reviewed Apr 13, 2017

View reviewed changes

ericstj approved these changes Apr 13, 2017

View reviewed changes

ericstj reviewed Apr 13, 2017

View reviewed changes

eerhardt reviewed Apr 13, 2017

View reviewed changes

eerhardt approved these changes Apr 13, 2017

View reviewed changes

livarcocc approved these changes Apr 13, 2017

View reviewed changes

Apply code review feedback

c7036c5

dsplaisted added the Pending Shiproom Approval label Apr 13, 2017

ManishJayaswal added the Approved to merge label Apr 13, 2017

dsplaisted merged commit aa0f30e into dotnet:master Apr 13, 2017

dsplaisted mentioned this pull request Apr 13, 2017

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

Merged

mmitche pushed a commit to mmitche/sdk that referenced this pull request Jun 5, 2020

Update dependencies from https://github.com/dotnet/arcade build 20191…

683bab6

…121.10 (dotnet#1121) - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.19571.10

Uh oh!

Apply conflict resolution to RuntimeTargets items #1121

Apply conflict resolution to RuntimeTargets items #1121

Uh oh!

Conversation

dsplaisted commented Apr 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericstj commented Apr 13, 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!

ericstj left a comment

Choose a reason for hiding this comment

Uh oh!

dsplaisted commented Apr 13, 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

Uh oh!

eerhardt commented Apr 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericstj commented Apr 13, 2017

Uh oh!

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

Uh oh!

Uh oh!

ericstj commented Apr 13, 2017

Uh oh!

dsplaisted commented Apr 13, 2017

Uh oh!

ManishJayaswal commented Apr 13, 2017

Uh oh!

dsplaisted commented Apr 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ericstj commented Apr 13, 2017 •

edited

Loading

eerhardt commented Apr 13, 2017 •

edited

Loading