Fixes problems in WindowsDesktop SDK due to _TargetFrameworkVersionWithoutV being undefined sometimes

[vatsan-madhavan]

Addresses #1651 - Invalid comparison in MSBuild's ignore-conditions mode in outer build of multitargeted project

This change ensures that the WindowsDesktop SDK never deals with undefined numeric values related to TFM.

This solution leverages the fact that Items are evaluated after Properties by MSBuild. See Comparing properties and items

_TargetFrameworkVersionValue - a new Property which is defined for exclusive use within the WindowsDesktop SDK, and which is intended to act as a proxy for _TargetFrameworkVersionWithoutV - is defined in Microsoft.NET.Sdk.WindowsDesktop.targets after TargetFrameworkVersionWithoutV is guaranteed to be defined by Microsoft.NET.Sdk.targets.

Even here, we ensure that a fallback default (0.0) is provided - guaranteeing that _TargetFrameworkVersionValue will always be numeric.

When the Conditions in the various Items in Microsoft.NET.WindowsDesktop.props are evaluated (which will happen strictly after all Properties are evaluated - whether they appear in .props or .targets), _TargetFrameworkVersionValue would have been (a) well-defined and (b) guaranteed to be a numeric value. This would in turn guarantee that errors of the kind "A numeric comparison was attempted on "$(_TargetFrameworkVersionValue)" that evaluates to "" instead of a number," will no longer appear in outer-builds or in any other context.

PS: This is a fix for a regression introduced by #1027 (which shipped in Preview 6) as part of a set of fixes for improving multi-targeting experience, improved warnings and error messages during build, and improvements to project-system integration.

[dsplaisted]

Why doesn't the existing '$(_TargetFrameworkVersionWithoutV)' != '' check short-circuit the subsequent clauses and avoid this problem?

[rainersigwald]

@dsplaisted It does, but only in the cases where it's in the same condition. It doesn't apply when the is-defined check is applied at the ItemGroup level.

[vatsan-madhavan]

Why doesn't the existing '$(_TargetFrameworkVersionWithoutV)' != '' check short-circuit the subsequent clauses and avoid this problem?

I'm not the expert - @rainersigwald should confirm; it does, but in this way I think:

 <ItemGroup Condition="$(A) != '' And $(A) >= 3.0> <!-- works ok --->
  <!-- You'd think that A != '' is  a given, but it needs to be repeated here - and everywhere else involving an expression containing 'A' - otherwise it doesn't count --> 
   <Foo Condition="$(A) >= 4.0" Include="Bar" /> 
</ItemGroup>

[dsplaisted]

@vatsan-madhavan Assuming that's correct, would it not be simpler to just repeat the nonempty check in all the conditions that need to do comparisons on the version?

[vatsan-madhavan]

Assuming that's correct, would it not be simpler to just repeat the nonempty check in all the conditions that need to do comparisons on the version?

That's not an approach particularly suited to maintainability IMO. It's simple to explain - yes, but it makes for worse readability, and sets up a system where small changes could incur complex and redundant conditions that should otherwise have been guaranteed by an enclosing scope already.

It's somewhat arguable that such a change would even be even simpler. The proposed change effectively leaves all the conditional constructs intact (modulo small well defined transformations from $(A) != '' to$(Y) != '0.0'; $(A) -> $(Y)). Changing conditions all over the place also changes the semantics of how everything is evaluated - only just a bit - but enough to require more testing, more careful reviews etc. At this point in 3.0, lower the entropy of changes, the better it seems to me.