[Bug]: Cannot merge folders with wildcards

mmarczell-graphisoft commented 1 year ago

Issue Description

Trying to use wildcards to merge the contents of two folders looks like it should be possible, but doesn't work due to bugs.

Steps to Reproduce

I have a vcxproj and two folders of resource files that I want to merge in the output. The content of the folders might be generated so I can't enumerate them in the project file, I need to use wildcards. I know wildcards are not fully supported in vcxproj so I follow the advice of the docs and put them in a Target:

<Target Name="SomeTarget" BeforeTargets="ClCompile">
   <ItemGroup>
      <Content Include="files1\**" CopyToOutputDirectory="PreserveNewest" TargetPath="files\@(RecursiveDir)%(Filename)%(Extension)"/>
      <Content Include="files2\**" CopyToOutputDirectory="PreserveNewest" TargetPath="files\@(RecursiveDir)%(Filename)%(Extension)"/>
   </ItemGroup>
</Target>

My project zipped:

CppSandbox.zip

Expected Behavior

The contents of the two folders should be merged at buildtime.

Actual Behavior

The project fails to build with the following error:

error MSB3024: Could not copy the file "D:\Dev\Sandbox\CppSandbox\files1\a.txt" to the destination file "D:\Dev\Sandbox\CppSandbox\Debug\files\", because the destination is a folder instead of a file. To copy the source file into a folder, consider using the DestinationFolder parameter instead of DestinationFiles.

Contrary to the docs, if I omit the Target and use a direct ItemGroup, it works. Another weird behaviour occurs if we consider one of the folders to be generated at buildtime, ergo not present at project evaluation time. So let's just put that in a Target:

<ItemGroup>
    <Content Include="files1\**" CopyToOutputDirectory="PreserveNewest" TargetPath="files\@(RecursiveDir)%(Filename)%(Extension)"/>         
</ItemGroup>
<Target Name="SomeTarget" BeforeTargets="ClCompile">
    <ItemGroup>
        <Content Include="files2\**" CopyToOutputDirectory="PreserveNewest" TargetPath="files\@(RecursiveDir)%(Filename)%(Extension)"/>
    </ItemGroup>
</Target>

Now MSBuild gets confused and starts overwriting the files with each other:

Copying file from "D:\Dev\Sandbox\CppSandbox\files1\c.txt" to "D:\Dev\Sandbox\CppSandbox\x64\Debug\files\c.txt". Copying file from "D:\Dev\Sandbox\CppSandbox\files1\a.txt" to "D:\Dev\Sandbox\CppSandbox\x64\Debug\files\a.txt". Copying file from "D:\Dev\Sandbox\CppSandbox\files1\b.txt" to "D:\Dev\Sandbox\CppSandbox\x64\Debug\files\b.txt". Copying file from "D:\Dev\Sandbox\CppSandbox\files2\d.txt" to "D:\Dev\Sandbox\CppSandbox\x64\Debug\files\b.txt". Copying file from "D:\Dev\Sandbox\CppSandbox\files2\d.txt" to "D:\Dev\Sandbox\CppSandbox\x64\Debug\files\c.txt". Copying file from "D:\Dev\Sandbox\CppSandbox\files2\d.txt" to "D:\Dev\Sandbox\CppSandbox\x64\Debug\files\a.txt". Copying file from "D:\Dev\Sandbox\CppSandbox\files2\e.txt" to "D:\Dev\Sandbox\CppSandbox\x64\Debug\files\b.txt". Copying file from "D:\Dev\Sandbox\CppSandbox\files2\e.txt" to "D:\Dev\Sandbox\CppSandbox\x64\Debug\files\c.txt". Copying file from "D:\Dev\Sandbox\CppSandbox\files2\e.txt" to "D:\Dev\Sandbox\CppSandbox\x64\Debug\files\a.txt". Copying file from "D:\Dev\Sandbox\CppSandbox\files2\f.txt" to "D:\Dev\Sandbox\CppSandbox\x64\Debug\files\b.txt". Copying file from "D:\Dev\Sandbox\CppSandbox\files2\f.txt" to "D:\Dev\Sandbox\CppSandbox\x64\Debug\files\c.txt". Copying file from "D:\Dev\Sandbox\CppSandbox\files2\f.txt" to "D:\Dev\Sandbox\CppSandbox\x64\Debug\files\a.txt".

Analysis

No response

Versions & Configurations

or

MSBuild version 17.5.0+6f08c67f3 for .NET Framework 17.5.0.10706

KalleOlaviNiemitalo commented 1 year ago

@(RecursiveDir) should surely be %(RecursiveDir), but I didn't test whether that works either.

mmarczell-graphisoft commented 1 year ago

@KalleOlaviNiemitalo Thanks, and no, it doesn't.

JanKrivanek commented 1 year ago

Once properly investigated and documented here - a good candidate to include in the documentation/learning effort: https://github.com/dotnet/msbuild/issues/8447

KalleOlaviNiemitalo commented 1 year ago

I tried this msbuild8527.msbuildproj

<Project>
  <Target Name="Build">
    <ItemGroup>
      <Content Include="files1\**" CopyToOutputDirectory="PreserveNewest" TargetPath="files\%(RecursiveDir)%(Filename)%(Extension)"/>
      <Content Include="files2\**" CopyToOutputDirectory="PreserveNewest" TargetPath="files\%(RecursiveDir)%(Filename)%(Extension)"/>
    </ItemGroup>

    <Message Text="Content: '%(Content.Identity)' -&gt; '%(Content.TargetPath)'" Importance="high" />
  </Target>
</Project>

with these (empty) files:

files1/abc/tups.demo
files1/eugh/floppy
files2/abc/openit.txt
files2/inroot2.bin
files2/sub/epsilon.mini

Results of dotnet msbuild using "MSBuild version 17.6.0-preview-23108-10+51df47643 for .NET" in .NET SDK 8.0.100-preview.1.23115.2:

  Content: 'files1\abc\tups.demo' -> 'files\'
  Content: 'files1\eugh\floppy' -> 'files\'
  Content: 'files2\abc\openit.txt' -> 'files\abc\tups.demo'
  Content: 'files2\inroot2.bin' -> 'files\abc\tups.demo'
  Content: 'files2\sub\epsilon.mini' -> 'files\abc\tups.demo'
  Content: 'files2\abc\openit.txt' -> 'files\eugh\floppy'
  Content: 'files2\inroot2.bin' -> 'files\eugh\floppy'
  Content: 'files2\sub\epsilon.mini' -> 'files\eugh\floppy'

so -- it seems that, in <Content Include="files2\**" CopyToOutputDirectory="PreserveNewest" TargetPath="files\%(RecursiveDir)%(Filename)%(Extension)"/>, the metadata references like %(RecursiveDir) do not refer to the items that are being added by Include="files2\**", but rather to the items that are already in the Content item type. Which may be a useful feature in some scenario but is certainly confusing here.

If I move the metadata to child elements

<Project>
  <Target Name="Build">
    <ItemGroup>
      <Content Include="files1\**">
        <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
        <TargetPath>files\%(RecursiveDir)%(Filename)%(Extension)</TargetPath>
      </Content>
      <Content Include="files2\**">
        <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
        <TargetPath>files\%(RecursiveDir)%(Filename)%(Extension)"</TargetPath>
      </Content>
    </ItemGroup>

    <Message Text="Content: '%(Content.Identity)' -&gt; '%(Content.TargetPath)'" Importance="high" />
  </Target>
</Project>

then the result is the same as above. Well, that's at least consistent.

Instead, I think this kind of merging has to be done like so:

<Project>
  <Target Name="Build">
    <ItemGroup>
      <ContentToMerge Include="files1\**" />
      <ContentToMerge Include="files2\**" />
      <Content Include="@(ContentToMerge)" CopyToOutputDirectory="PreserveNewest" TargetPath="files\%(RecursiveDir)%(Filename)%(Extension)" />
    </ItemGroup>

    <Message Text="Content: '%(Content.Identity)' -&gt; '%(Content.TargetPath)'" Importance="high" />
  </Target>
</Project>

in which case %(RecursiveDir) etc. refer to the ContentToMerge items, and the output is as expected:

  Content: 'files1\abc\tups.demo' -> 'files\abc\tups.demo'
  Content: 'files1\eugh\floppy' -> 'files\eugh\floppy'
  Content: 'files2\abc\openit.txt' -> 'files\abc\openit.txt'
  Content: 'files2\inroot2.bin' -> 'files\inroot2.bin'
  Content: 'files2\sub\epsilon.mini' -> 'files\sub\epsilon.mini'

KalleOlaviNiemitalo commented 1 year ago

"Microsoft (R) Build Engine version 15.9.21+g9802d43bc3 for .NET Framework" behaves the same as "MSBuild version 17.6.0-preview-23108-10+51df47643 for .NET".

"Microsoft (R) Build Engine version 4.8.4084.0" likewise, except it does not support metadata as attributes.

"Microsoft (R) Build Engine Version 2.0.50727.9149" does not support ItemGroup within Target.

JanKrivanek commented 1 year ago

Thank you @KalleOlaviNiemitalo for the sample and suggested fix.

To explain the observed behavior:

Referencing the item metadata (whether via %(Item.Metadata) or %(Metadata) or @(Metadata)) leads to different expansion when used within and outside of the targets:

Outside of the target, it refers to the metadata of the item, currently being processed - so this can be used to assemble additional metadata of current item based on existing metadata. Basically what OP is trying to achieve
Inside the target (or task), this leads to batching. So the expression is expanded for each item with that particular Metadata defined. In this specific case it leads to cross-applying of the paths (unwanted).

The way out is:

Either define the additional metadata outside of the target (to prevent batching)
Or (as suggested by @KalleOlaviNiemitalo) add items into different Item and then Include and expand this to your desired Item (Content here).

Or us the item transforms to express the 1:1 desired transformation between two item lists

<Target Name="Build">
<ItemGroup>
  <Content Include="files1\**" />
  <Content Include="files2\**" />
</ItemGroup>

<Message Text="Content: '%(Content.Identity)' -&gt; '@(Content->'files\%(RecursiveDir)%(Filename)%(Extension)')'" Importance="high" />
</Target>

KalleOlaviNiemitalo commented 1 year ago

Would it be feasible to make MSBuild emit a warning about this use, and link the warning code to an article that describes what to do instead?

Warn if an ItemGroup in a Target contains an item element that matches all of these conditions:

has an Include attribute
has one or more unqualified %(metadata) references (or qualified reference attempting to reference current item)
does not reference any qualified %(itemtype.metadata) (unless itemtype referenced is identical to the currently processed item)
does not reference any @(itemtype) (including transforms)

JanKrivanek commented 1 year ago

@KalleOlaviNiemitalo Great suggestion. We've discussed this idea yesterday (without details on how to detect possible missues - your suggestion is nicely expanding that thought).

I'd probably modify the point 3 above ('does not reference any qualified %(itemtype.metadata)' to disallow %(itemtype.metadata) reference of self as well) - adding the suggestion inline, as your list is a nice reference

KalleOlaviNiemitalo commented 1 year ago

I was thinking, if someone hits the new warning but does want the current behaviour, then they can silence the warning by qualifying the metadata reference. This is why it should not warn about qualified metadata, even if the item type matches the element.

dotnet / msbuild