NuGet client traverses files in Include before package path parameters are defined

[puya-ms]

NuGet Product Used

NuGet.exe

Product Version

6.4.0.123

Worked before?

No

Impact

It bothers me. A fix would be nice

Repro Steps & Context

Create a CSPROJ file with a section like this:

<ItemGroup>
  <None Include="$(PkgSomePackage)\**">
    <Link>SomePackage\%(RecursiveDir)%(Filename)%(Extension)</Link>
    <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
  </None>
</ItemGroup>

Try running nuget.exe restore on a solution containing this project:

nuget.exe restore .\MySolution.sln -Verbosity Detailed -NonInteractive
NuGet Version: 6.4.0.123
MSBuild auto-detection: using msbuild version '17.4.1.60106' from 'C:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Current\Bin\amd64'. Use option -MSBuildVersion to force nuget to use a specific version of MSBuild.
MSBuild P2P timeout [ms]: 120000
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Current\Bin\amd64\msbuild.exe "C:\Users\user1\AppData\Local\Temp\NuGetScratch\zs4bjrwy.nay.nugetinputs.targets" /t:GenerateRestoreGraphFile /nologo /nr:false /v:q /p:NuGetRestoreTargets="C:\Users\user1\AppData\Local\Temp\NuGetScratch\acfhzpj5.ysl.nugetrestore.targets" /p:RestoreUseCustomAfterTargets="True" /p:DisableCheckingDuplicateNuGetItems="True" /p:RestoreTaskAssemblyFile="C:\ProgramData\chocolatey\lib\NuGet.CommandLine\tools\nuget.exe" /p:RestoreSolutionDirectory="C:\Path\To\Solution\\" /p:SolutionDir="C:\Path\To\Solution\\" /p:SolutionName="My.Solution"
MSBUILD : warning MSB5029: The value "\**" of the "Include" attribute in element <ItemGroup> in file "C:\Path\To\Solution\MyProject\MyProject.csproj (41,11)" is a wildcard that results in enumerating all files on the drive, which was likely not intended. Check that referenced properties are always defined.

I think correct behavior would be that =NuGet restore command does not try to iterate these files, or if for some reason this is needed, to first define the path variables.

Verbose Logs

No response

[erdembayar]

Thank you for filing this issue.
Could you be able to provide sample repro code/project?
Also, what happens if you do dotnet restore or msbuild -t:restore? Do you see same experience?

[aortiz-msft]

The behavior and the warning is coming from MsBuild, not NuGet.

[puya-ms]

@aortiz-msft Could you please explain how NuGet restore leads to MSBUILD traversing the wildcard include path? I will then open an issue against them instead.

[zivkan]

@puya-ms The way restore finds <PackageReference items is by asking MSBuild to "evaluate" the project, and then tell us all the PackageReference items.

Coupled with the fact that MSBuild's scripting language doesn't have the concept of nulls, they're treated as empty strings, this means that $(PkgSomePackage)\** is equivalent to \** when PkgSomePackage is not specified.

In short, you should add Condition=" '$(PkgSomePackage)' != '' " on the <None item, so that it gets ignored when the property is not defined.

[puya-ms]

Thank you @zivkan for the workaround. This ordering issue is not clear (and IMO should not matter) to end user. As an end user I shouldn't care about the implementation of restore and that the NuGet variable is defined after the csproj file is evaluated. Is this mentioned anywhere in the documentation?

Is this something that should be fixed / improved on msbuild in your opinion? Maybe MSBUILD should define those variables by evaluating the reference to the package before trying to evaluate anything that uses them, or implement another solution to avoid this.

[zivkan]

My gut feeling is that your proposal will cause significant breaking changes to MSBuild, and will cause more problems to existing customers than customers who will benefit from not needing to learn as much about MSBuild as a scripting language. I'm not a fan of MSBuild at all. I consider it technical debt that is makes some improvements to NuGet more difficult. But it's very tightly coupled now, and many customers use MSBuild extensibility to customize their builds in ways that would be impossible to reproduce without breaking changes, and would be very costly to implement similar (but not equivalent) extensibility.

If you're curious about learning more about MSBuild, this is a good starting place to learn about the high level process of how MSBuild loads and executes builds: https://learn.microsoft.com/en-us/visualstudio/msbuild/build-process-overview?view=vs-2022. It also explains the order of import evaluation vs property evaluation vs item evaluation vs target execution.

When .NET Core 1.0 was being developed, they tried moving away from MSBuild and created project.json, maybe you heard of it. However, in the end the people working on it decided that there was too much investment in the MSBuild ecosystem, so they reverted back to MSBuild csproj files, although with improvements to the default imports to make the minimum csproj much easier than before. But the MSBuild scripting language itself didn't fundamentally change (adding the Sdk= import is the only change I'm aware of).

Anyway, I don't work on MSBuild, you can discuss it with them if you like at https://github.com/dotnet/msbuild, but my feeling is that your request is not feasible. The unfortunate reality is that to be an advanced .NET developer, you need to learn MSBuild's quirks.

[jzabroski]

I ran into this warning due to a change in behavior in MSBuild that must've occurred in the last year or so.

I was previously relying on the following behavior to pack another dependency via transclusion:

  <ItemGroup>
    <PackageReference Include="App.Web" Version="$(AppPackageVersion)" GeneratePathProperty="true" />
  </ItemGroup>

  <ItemGroup>
    <PackageBinDirectoryContents Include="$(PkgApp_Web)\**" />
  </ItemGroup>

  <!-- Copy the files to the output directory -->
  <Target Name="CopyPackageDirectoryFiles" AfterTargets="Build">
    
    <Message Importance="High" Text="%24(PkgApp_Web) [$(PkgApp_Web)]" />
    
    <PropertyGroup>
      <ActualPackageVersion>$([System.IO.Path]::GetFileName($(PkgApp_Web)))</ActualPackageVersion>
      <!--<Version>
        $(ActualPackageVersion)+build.$(BuildNumber)
      </Version>-->
    </PropertyGroup>

    <Message Importance="High" Text="%24(ActualPackageVersion) [$(ActualPackageVersion)]" />

    <Copy SourceFiles="@(PackageBinDirectoryContents)" DestinationFiles="@(PackageBinDirectoryContents->'$(OutDir)%(RecursiveDir)%(Filename)%(Extension)')" />
  </Target>

Moving the ItemGroup that defines PackageBinDirectoryContents to inside the Task that does the transclusion operation, seems to fix the problem. I don't remember why I chose to make this a top-level ItemGroup. It's a little surprising to me that MSBuild is not evaluated linearly in the case of GeneratePathProperty expressions. Seems to partly defeat the purpose of having it.

[zivkan]

It's a little surprising to me that MSBuild is not evaluated linearly in the case of GeneratePathProperty expressions

The package path property is only set after a restore has completed (and running a target other than restore). Before restore (or during the restore target), the package path property isn't defined, hence the warning.

It works as you expect when you put the item definition inside the target, because the target says AfterTargets="Build", and by the time the build target runs, restore has already run, and therefore the package path property has now been defined.

[jzabroski]

The package path property is only set after a restore has completed (and running a target other than restore). Before restore (or during the restore target), the package path property isn't defined, hence the warning.

It used to work differently, though. I suspect the change has something to do with this open bug in MSBuild: dotnet/msbuild#5937

I do not disagree with your assessment that my new approach is semantically correct and the old one was hokey pokey. Overall, I am glad this warning exists.

I think one outcome from this discussion that would be a net positive would be to document this MSBuild warning, with more context around how it could occur. In particular, your note here was very helpful:

Coupled with the fact that MSBuild's scripting language doesn't have the concept of nulls, they're treated as empty strings, this means that $(PkgSomePackage)\** is equivalent to \** when PkgSomePackage is not specified.

-- @zivkan