msbuild
msbuild copied to clipboard
dotnet
No error for Item update in target
Ran into this while creating a workaround to patch an existing target (https://github.com/dotnet/cli/issues/6397#issuecomment-347989664) and naively using the item update syntax inside a target to update the link metadata of an item. However, using the update syntax inside a target seems to update the metadata of all items, which is unexpected.
Steps to reproduce
Project file
<Project DefaultTargets="PrintResults">
<ItemGroup>
<SomeStaticItem Include="Item1" SomeMeta="MetaVal1" />
<SomeStaticItem Include="Item2" SomeMeta="MetaVal2" />
<SomeStaticItem Include="Item3" SomeMeta="MetaVal3" />
<SomeStaticItem Update="Item2" SomeMeta="ChangedMetaVal2" />
</ItemGroup>
<Target Name="CreateRuntimeUpdatedItems">
<ItemGroup>
<SomeRuntimeItem Include="@(SomeStaticItem)" />
<SomeRuntimeItem Update="Item2" SomeMeta="ChangedMetaVal2" />
</ItemGroup>
</Target>
<Target Name="PrintResults" DependsOnTargets="CreateRuntimeUpdatedItems">
<Message Importance="high" Text="static item: %(SomeStaticItem.Identity): SomeMeta=%(SomeStaticItem.SomeMeta)" />
<Message Importance="high" Text="runtiime item:%(SomeRuntimeItem.Identity): SomeMeta=%(SomeRuntimeItem.SomeMeta)" />
</Target>
</Project>
Command line
dotnet msbuild
Expected behavior
Error for Update syntax not being allowed inside targets
or
static item: Item1: SomeMeta=MetaVal1
static item: Item2: SomeMeta=ChangedMetaVal2
static item: Item3: SomeMeta=MetaVal3
runtiime item:Item1: SomeMeta=MetaVal1
runtiime item:Item2: SomeMeta=ChangedMetaVal2
runtiime item:Item3: SomeMeta=MetaVal3
Actual behavior
static item: Item1: SomeMeta=MetaVal1
static item: Item2: SomeMeta=ChangedMetaVal2
static item: Item3: SomeMeta=MetaVal3
runtiime item:Item1: SomeMeta=ChangedMetaVal2
runtiime item:Item2: SomeMeta=ChangedMetaVal2
runtiime item:Item3: SomeMeta=ChangedMetaVal2
Environment data
dotnet msbuild /version output: tested on 15.5.179.9764 and 15.6.12.27473 .
OS info:
If applicable, version of the tool that invokes MSBuild (Visual Studio, dotnet CLI, etc): macOS 10.13.2, dotnet cli
Looks like a duplicate of https://github.com/Microsoft/msbuild/issues/1618, but that should have been fixed long ago (by throwing an error on the not-actually-supported Update-inside-a-target syntax).
If you change your target to use syntax like https://github.com/Microsoft/msbuild/issues/1618#issuecomment-275691043, it should work.
I repro the problem with 15.1.548.43366, so it looks like the error never worked :(
yep the old syntax works perfectly, I assumed that since there was no Update metadata added it actually tried to update (which is exactly what's discussed on the linked issue).
@rainersigwald I'm afraid adding the error now would break at least https://github.com/dotnet/sdk/blob/cb4cf799e0afd5a6c8e0630638f550b95e3eb35e/src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.ConflictResolution.targets#L47 https://github.com/dotnet/sdk/blob/cb4cf799e0afd5a6c8e0630638f550b95e3eb35e/src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.PackageDependencyResolution.targets#L450-L453 https://github.com/dotnet/sdk/blob/cb4cf799e0afd5a6c8e0630638f550b95e3eb35e/src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.PackageDependencyResolution.targets#L578-L580
@dasMulli Yup, looks like it. Unfortunately, that means we can't do this in a minor update as it'd be a breaking change.
@rainersigwald We should fix this in Dev16.
Bugs like these make msbuild very hard to use. If you want to avoid breaking changes please consider introducing some compat switch that can be used to opt-in new behavior.
it looks like the error never worked :(
Was it ever implemented? AFAICT, #1618 was closed because it was included in #1124 (another bug), not because it was fixed.
Maybe we should consider making it actually work as one expects rather than erroring in Dev16? The examples shown above so far would not break. I am nervous as always about breaking changes, so we should search for more data on whether there are cases of an update on anything but the entire set, that are actually depending on their bug.
Looks like we missed 6.0 GA but can this be a candidate for a toolset update in a band release?
@clairernovotny the nature of the breaking change makes that not seem like a great idea to me. We've found instances of this in basically all .NET repos as well as user projects. It's not clear to me that the benefit is worth the change, since it could be a subtle silent behavior change to presumably-working targets (after the authors worked around the bad behavior somehow to get it to meet their needs).
I don't know what we need to do about this bug, but we need to do something.
How about emitting a warning here? "Warning: 'Update' for item updates within targets does not have any effect and all items will be changed. See ..."
This communicates to the author that the project file has some possibly unexpected behavior while not immediately breaking all builds (except for those with warnings as errors).
We consider adding a warning a breaking change, based on painful experience.
The best solution here is going to be a warning that users can opt into, which is why this is marked in the "warning waves" tag, which will be rolled up in the analyzers work that's underway (this makes sense for the early set #9630 IMO).
Instead of going the roundabout way, why not make the Update attribute work? At this point, it would be better than adding a warning when people expect it to work!
If you do support an opt-in behavior change, also support an opt-in warning (maybe a common msbuild errors analyzer, since we've found a number of ways in msbuild to shoot yourself in the foot) so all instances can be identified and validated for the opt-in behavior change.
@rainersigwald, sounds like even nowarn options are insufficient to consider automatic new warnings a non-breaking change... leaves me curious and sounds like a painful development impediment.
If you do support an opt-in behavior change, also support an opt-in warning (maybe a common msbuild errors analyzer, since we've found a number of ways in msbuild to shoot yourself in the foot) so all instances can be identified and validated for the opt-in behavior change.
@rainersigwald, sounds like even nowarn options are insufficient to consider automatic new warnings a non-breaking change... leaves me curious and sounds like a painful development impediment.
Indeed it is painful! That's why we're investing a lot right now on a path forward which we're calling analyzers and tracking with https://github.com/dotnet/msbuild/issues?q=is%3Aissue+is%3Aopen+label%3A%22Feature%3A+Warning+Waves%22+.
Instead of going the roundabout way, why not make the
Updateattribute work? At this point, it would be better than adding a warning when people expect it to work!
or make it work properly after users opt in
It would have to be opt-in since it's a confusing breaking behavior change otherwise. I'm not super opposed to that we'd need the warning when not opted in anyway, so let's get that then reevaluate.
This issue reminds me of #8527. The MSB4120 warning was added then (#8581). And later its priority was lowered (#9228).
I'm more than ever convinced now that we need a mode where item updates in targets work as expected
Stepped on this one this week because the sentence in the docs is very subtly different than the three previous ones. A reading comprehension failure, of course, just one that caused us a bit of an issue. Our specific use case was adding <Pack>false</Pack> to Content items added from Grpc.Core as it incorrectly added its DLLs to the contentFiles directory of some of our NuGet packages.
