[Bug]: NU5050 doesn't look at source file prior to determining if target is duplicate

jzabroski commented 3 years ago

NuGet Product Used

dotnet.exe

Product Version

dotnet.exe --version 5.0.301

Worked before?

Anything prior to https://github.com/NuGet/NuGet.Client/pull/3923

Impact

I'm unable to use this version

Repro Steps & Context

@Zkat Thanks for working on this previous PR. It saved me a bunch of time tracking down problems yesterday. However, it took me awhile to troubleshoot how to fix this error. After thinking about why it too me so long, I realized: There are two types of duplicates, and this rule doesn't look at a far enough range of metadata to know which type of duplicate is the root cause of duplication.

Duplication one, trivial example:

Custom NuspecFile Passed in via csproj `<NuSpecFile>` element

In this example, we're packing a WinForms application and copying over environment files like appSettings.xml, appSettings.Alpha.xml, appSettings.Beta.xml, which our deployment tool (Octopus Deploy) would then transform using XSLT. Same for Web.config.

<?xml version="1.0" encoding="utf-8" ?>
<package xmlns="http://schemas.microsoft.com/packaging/2010/07/nuspec.xsd">
  <metadata>
    <id>$Id$</id>
    <version>$Version$</version>
    <description>$Description$</description>
    <authors>$Authors$</authors>
    <copyright>$Copyright$</copyright>
  </metadata>
  <files>
    <file src="bin\release\**\*" target="" />
    <file src="*.config" />
    <file src="*.xml" />
  </files>
</package>

Our fix to this type of duplicate was to set .config and .xml to Content and let MSBuild move them to the bin directory.

Note, in this case, the files are most likely identical duplicates because their upstream source is the same. NU5050 doesn't take this into account, since it only looks at file mappings with ambiguous targets, because IPackageFile doesn't know anything about source paths. I think, in that regard, the validation logic should operate on a INuspecFile (if such an interface exists), since it would be best able to partition the cause of duplicates back to their upstream sources, and then further determine if the files are duplicated or not.

The next example also uses a custom nuspec file, but for some reason the generated nupkg on .NET 5.0.2 SDK does not contain duplicate files even though we get a NU5050 error and cannot generate the nupkg on .NET 5.0.3 SDK.

The specific error we get is error NU5050: Attempted to pack multiple files into the same location(s). The following destinations were used multiple times: bin\roslyn\csc.exe.config, bin\roslyn\csi.exe.config, bin\roslyn\vbc.exe.config, bin\roslyn\VBCSCompiler.exe.config, bin\MyCompany.MyProjectName.Website.dll.config.

The below example is packing a ASP.NET Classic Website using a custom nuspec file:

<?xml version="1.0" encoding="utf-8" ?>
<package xmlns="http://schemas.microsoft.com/packaging/2010/07/nuspec.xsd">
  <metadata>
    <id>$Id$</id>
    <version>$Version$</version>
    <description>$Description$</description>
    <authors>$Authors$</authors>
    <copyright>$Copyright$</copyright>
  </metadata>
  <files>
    <file src="bin\**\*" target="bin" />
    <!-- We need to exclude the bin directory contents from each of the following includes to avoid NU5050 duplicate error. -->
    <file src="**\*.ascx" />
    <file src="**\*.asmx" />
    <file src="**\*.aspx" />
    <file src="**\*.config" />
    <file src="**\*.cshtml" />
    <file src="**\*.css" />
    <file src="**\*.eot" />
    <file src="**\*.gif" />
    <file src="**\*.jpg" />
    <file src="**\*.js" />
    <file src="**\*.map" />
    <file src="**\*.master"/>
    <file src="**\*.png" />
    <file src="**\*.sass" />
    <file src="**\*.scss" />
    <file src="**\*.skin" />
    <file src="**\*.svg" />
    <file src="**\*.ttf" />
    <file src="**\*.woff" />
    <file src="**\*.woff2" />
    <file src="*.asax" />
    <file src="*.htm" />
    <file src="*.xml" />
    <file src="*.sitemap" />
  </files>
</package>

Duplication two, PackageFiles are included twice somehow

The most likely way to reproduce this for regression testing purposes is to simply force a PackageFiles itemgroup include, and also configure nuget to pack the bin directory.

  <ItemGroup>
    <Content Include="Web.config" />
    <Content Include="..\..\MyContentFiles\**\*.*">
      <!-- If you add %(RecursiveDir), it will avoid a "included twice somehow" condition. 
      Let's just assume for regression sake that the user had this long-standing typo in their csproj
      and forgot to add %(RecursiveDir) -->
      <Link>%(Filename)%(Extension)</Link>
      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
    </Content>
  </ItemGroup>

In this scenario, it is more likely that Web.config not only be a duplicate target, but the rolling byte comparison of the source files be non-unique. I suspect this scenario is the most dangerous one that NU5050 should try to catch, but NU5050 doesn't distinguish this from "overlapping mappings from the same source file".

Verbose Logs

dotnet.exe --version
> 5.0.301

dotnet.exe pack .\Source\MyCompany.MyProject.sln --configuration Release --output packages --no-build -p:PackageVersion=1.0.94.1438 -bl:C:\temp\msbuild.binlog
> C:\Program Files\dotnet\sdk\5.0.301\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(221,5): error NU5050: Attempted to pack multiple files into the same location(s). The following destinations were used multiple times: App.Alpha.config, App.Test.config, App.Beta.config, App.Production.config, AppSettings.Alpha.xml, AppSettings.Beta.xml, AppSettings.Test.xml, AppSettings.Production.xml, AppSettings.xml [D:\Users\John.Zabroski\source\trunk\Source\MyCompany.MyProject.WinForms.UI\MyCompany.MyProject.WinForms.UI.csproj]
> 
> 
> C:\Program Files\dotnet\sdk\5.0.301\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(221,5): error NU5050: Attempted to pack multiple files into the same location(s). The following destinations were used multiple times: bin\roslyn\csc.exe.config, bin\roslyn\csi.exe.config, bin\roslyn\vbc.exe.config, bin\roslyn\VBCSCompiler.exe.config, bin\MyCompany.MyProject.Website.dll.config [D:\Users\John.Zabroski\source\trunk\Source\MyCompany.MyProject.Website\MyCompany.MyProject.Website.csproj]

jzabroski commented 3 years ago

@kartheekp-ms This is a follow up to the code review comments I left yesterday on the original PR for Nu5050.

jzabroski commented 3 years ago

Note, it is also possible for a nupkg to generate that's silently corrupt by removing "all duplicates". Trying to open the nupkg w/ PeaZip or NugetPackageExplorer.exe will fail with a fatal error. So, there is probably another missing package validation rule here. Logged here: https://github.com/NuGet/Home/issues/11053

NuGet / Home