Enabling GeneratePackageOnBuild deleted my entire history of packages

[sharwell]

Using Visual Studio 2017 (15.3 Preview 6).

⚠️ This is a data loss bug (silent destruction of user data unrelated to the project being built).

📝 The title of this is accurate. I kept a copy of ANTLR 3 builds going back to 2011 in the folder which was cleared. I can get it back, but it will not be easy since the storage location for them changed a few times over the years.

Steps to reproduce

1. Enable <GeneratePackageOnBuild>
2. Set <PackageOutputPath> to a directory containing common packages for several unrelated projects

Expected results

Project builds and places the generated package in that folder.

Actual results

All packages is the folder are deleted.

Additional information

The build process MUST adhere to the following limitations regarding packages to delete.

1. Incremental clean SHOULD delete files created or copied by the previous build to <PackageOutputPath>
2. Incremental clean MUST NOT delete files which were not created in <PackageOutputPath> by the previous build of the project

[ChickJ]

This issue bit me today. All my project <PackageOutputPath> properties point to the same directory outside my source tree.

Relevant section in nuget.client/src/NuGet.Core/NuGet.Build.Tasks.Pack/Pack.targets:71

  <Target Name="_CleanPackageFiles"
          AfterTargets="Clean"
          Condition="'$(GeneratePackageOnBuild)' == 'true'">
    <ItemGroup>
      <_PackageFilesToDelete Include="$(PackageOutputPath)*.nupkg"/>
      <_PackageFilesToDelete Include="$(BaseIntermediateOutputPath)*.nuspec"/>
    </ItemGroup>
    <Delete Files="@(_PackageFilesToDelete)"/>
  </Target>

I believe that one should err on the conservative side when performing destructive actions. Especially when AfterTargets="Clean" injects the deletion behind the scene.

Update: Right now I decided to replace _CleanPackageFiles with my own implementation below:

<Target Name="_CleanPackageFiles"
        AfterTargets="Clean"
        Condition="'$(GeneratePackageOnBuild)' == 'true'">
    <ItemGroup>
        <_PackageFilesToDelete Include="$(PackageOutputPath)$(PackageId).$(Major).$(Minor).$(Patch)*.nupkg" />
        <_PackageFilesToDelete Include="$(BaseIntermediateOutputPath)$(PackageId).$(Major).$(Minor).$(Patch)*.nuspec" />
    </ItemGroup>
    <Delete Files="@(_PackageFilesToDelete)" />
</Target>

It works for my needs and no longer purges all packages in PackageOutputPath but there are some tricky issues related to the problem. $(Major) $(Minor) $(Patch) are my own properties so this obviously isn't a fix for you all.

Ideally I would write $(PackageOutputPath)$(PackageId).$(PackageVersion)*.nupkg but this falls apart when you have a timestamp or counter in your version string.

For example: <PackageVersion>0.0.1-preview-$([System.DateTime]::Now.ToString(yyyyMMddmmss))</PackageVersion>

The Clean target is an entry point and reevaluates the timestamp each time the project is loaded. The correct files are never included since the timestamp always changes.

The best advice I can come up with to improve the issue, is append all package file name variants to <CleanFile> and let the existing clean target take care of the files. That would be correct in the sense that only NuGet created files are deleted, but it wouldn't necessarily do what people wanted it to.

[rohit21agrawal]

@ChickJ @sharwell thanks for your feedback. I have a PR out for this to make the clean target conservative - the limitation would be that it would only work for static package versions. it would not cover the scenario that @ChickJ mentioned when package versions change dynamically with each build.

We would love to hear feedback about how common that scenario is, and based on that, we will make improvements to this in the future. The PR right now fixes the immediate problem of deleting packages that were not meant to be deleted.

[sharwell]

@rohit21agrawal Sorry I didn't get my review here earlier. The new implementation still isn't correct, with one limitation being the one you mentioned (this is an expected case, and specifically it is a case that MSBuild is designed to handle reliably). I posted details for the correct solution in the pull request.