MinVer can fail if internal item groups are already populated

[adamralph]

This is a corner case bug that will likely never be hit, but it's simple to fix.

The internal item groups used in the MinVer target, MinVerInputs, MinVerConsoleOutput, and MinVerOutputVersion, are populated using the Include attribute. This adds the specified values to the item groups. If those item groups already contain values before the MinVer target is executed, then the MinVer target can fail or produced unexpected results.

I hit this when I was putting together IdentityServer/IdentityServer4#3163 because I was using minver-cli in a target which ran before the MinVer target and I was using the same item group names to extract the console output. It's easily worked around by using different item group names, but it caused me a huge headache trying to find out what the problem was.

[bording]

What are you thinking the fix for this would be? ItemGroups can have multiple items, and you add to the group by using Include to add to the group.

[adamralph]

@bording I think an initial

<Foo Remove="@(Foo)" />

should do it.

[bording]

Oh, so you're thinking you'd want to clear out the ItemGroup before using it? I guess that would work.

[adamralph]

Released in 1.1.0-beta.1.

[viceice]

You should issue a warning, if the internal itemgroups are not empty.

If you clear them and a user is unaware of this, then it can break the build.

Or issue a error, because internal group names shouldn't be reused.

[adamralph]

@viceice thanks for chiming in. You raise a valid point, but it's not related to this change. Before this change, the values populated in the internal item groups would be merged with any existing items which would already result in misbehaviour. After this change, the values replace any existing items, as they should have done originally. The fact that didn't happen was a bug. This change brings the internal item groups into line with all the other variables that are set by MinVer in that they are now set, rather than added to.

The remaining question then, is whether MinVer should warn before setting any variable value, not just the internal item groups. To me, that feels like overkill. It's true that someone could be using variable names prefixed with MinVer for other purposes, and when they install MinVer they encounter a clash. But I think this is really an edge case not worth worrying about. If it is user code that is using those clashing variable names, they can be changed. If it is another package, well, that's a bigger problem, but again, I think it's an edge case which will likely never happen. If it ever does, then I'd be willing to revisit this.

What do you think?

[viceice]

Ok, agree. This should hopefully never happen.