dotnet / msbuild

The Microsoft Build Engine (MSBuild) is the build platform for .NET and Visual Studio.
https://docs.microsoft.com/visualstudio/msbuild/msbuild
MIT License
5.21k stars 1.35k forks source link

HasMetadata item function cannot distinguish between "not present" and "set to empty" #1030

Open kzu opened 8 years ago

kzu commented 8 years ago

I had the need to filter components based on the presence of metadata, regardless of its value.

The HasMetadata item function, however, checks for the metadata value, rather than just the presence of the metadata name, so items with the metadata, but an empty value, are improperly filtered out.

The following snippet showcases the issue:

        <?xml version="1.0" encoding="utf-8"?>
        <Project DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
            <Target Name="Build">
                <ItemGroup>
                    <Source Include="Foo">
                        <SubPath></SubPath>
                    </Source>
                    <Source Include="Bar">
                        <SubPath>Bar</SubPath>
                    </Source>
                    <Source Include="Baz" />
                </ItemGroup>

                <ItemGroup>
                    <WithSubPath Include="@(Source -> HasMetadata('SubPath'))" />
                </ItemGroup>

                <Message Text="@(WithSubPath)" Importance="high" />
            </Target>
        </Project>

This renders "Bar", but not "Foo", which does contain the metadata item.

I know this would be a breaking change, so maybe a new item function named HasMetadataName could be provided for this behavior?

rainersigwald commented 8 years ago

HasMetadata does behave as you desire . . . mostly. The problem is that you're defining empty metadata.

Observing the /verbosity:Diagnostic output of building your sample:

Target "Build: (TargetId:2)" in project "D:\play\hasmetadata-kzu.proj" (entry point):
Added Item(s):
    Source=
        Foo
                SubPath=
Added Item(s):
    Source=
        Bar
                SubPath=Bar
Added Item(s): Source=Baz
Added Item(s):
    WithSubPath=
        Bar
                SubPath=Bar
Using "Message" task from assembly "Microsoft.Build.Tasks.Core, Version=14.0.0.0, Culture=neutral, PublicKeyToken=b03f5
f7f11d50a3a".
Task "Message" (TaskId:2)
  Task Parameter:Text=Bar (TaskId:2)
  Task Parameter:Importance=high (TaskId:2)
  Bar (TaskId:2)
Done executing task "Message". (TaskId:2)
Done building target "Build" in project "hasmetadata-kzu.proj".: (TargetId:2)

We can see that the Foo item has the metadata, and it's set to empty. When it's queried for, though, the item draws no distinction between "metadata not present" and "present but set to null or the empty string": GetMetadataEscaped returns the same thing in both cases, and the Expander for HasMetadata then drops the item from its return.

This raises an interesting design question: what should the behavior be?

I claim that the current behavior is analogous to properties, and therefore correct. There's no way to remove a property or undefine metadata (there's Clear() but that undefines all metadata), but you can achieve the same effect by setting either to empty (or by the <Propertyname /> idiom which is the same thing).

I don't like that MSBuild can't distinguish between "nonexistent" and null, but fixing that is a much larger project.

Thoughts?

kzu commented 8 years ago

Without this capability, I'm unable to distinguish VSIXSourceItems from the vssdk for example ;). They add the VSIXSubPath metadata, but unsurprisingly, it can be empty if the file is supposed to go on the root of the VSIX extension...

I resorted to a custom inline task that checks for the presence of the metadata name instead, and I think it's a generally useful addition as an item function.

Agreed on the existing semantics. It makes sense 99% of the time. For what it's worth, it's the first time I need something different.

/kzu from mobile

cdmihai commented 8 years ago

In this case, considering MSBuild's behaviour, maybe that potentially empty VSIXSubPath is a bad metadata choice to filter items on. A bool looking one like <IsVSixSource>true</IsVSixSource> might be better here (or anything else that's guaranteed to not be empty :)). Is this something you can control?

kzu commented 8 years ago

I don't control it , nope. It's the vssdk ;)

On Thu, Sep 15, 2016, 4:04 PM Mihai Codoban notifications@github.com wrote:

In this case, considering MSBuild's behaviour, maybe that VSIXSubPath is a bad metadata choice to filter items on. A bool looking one like

true might be better here (or anything else that's guaranteed to not be empty :)). Is this something you can control? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Microsoft/msbuild/issues/1030#issuecomment-247421136, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKW63Y4TwnsgnMKSbPw6Q2UGoiHBvtMks5qqZazgaJpZM4J9342 . ## /kzu from mobile