Msbuild: '$(MSBuildToolsVersion)' should compare as a version or number

[rainersigwald]

See concerns/ideas in https://github.com/Microsoft/msbuild/issues/3778#issuecomment-441779192 and https://github.com/Microsoft/msbuild/issues/3778#issuecomment-457799037

Now that $(MSBuildToolsVersion) is Current, user projects or extensions that condition against it are likely to be broken, with an error like

A numeric comparison was attempted on "$(MSBuildToolsVersion)" that evaluates to "Current" instead of a number, in condition " '$(MSBuildToolsVersion)' == '' OR '$(MSBuildToolsVersion)' < '4.0' ".

[rainersigwald]

cc interested parties from #3778 -- @rseanhall and @ericstj.

While it's a bit odd to have a special-case in a numeric comparison for the string Current, I agree that it's probably less bad than the current errors, and unlikely to break someone (you'd have to be _depending_ on 'Current' > '3.0' throwing an invalid-project exception).

[rseanhall]

Thanks. The main thing WiX needs is for the condition " '$(MSBuildToolsVersion)' == '' OR '$(MSBuildToolsVersion)' < '4.0' " to evaluate to false. It would be nice if you replace it with $(VisualStudioVersion) like you are considering doing for https://github.com/Microsoft/msbuild/issues/4149.

[rainersigwald]

It would be nice if you replace it with $(VisualStudioVersion) like you are considering doing for #4149.

That's . . . a really interesting idea. I think there are 3 options and I now like that one best:

1. In the context of a numeric comparison, treat the string Current as the hard-coded VisualStudioVersion.
2. In the context of a numeric comparison, treat the string Current as +∞.
3. In the context of a numeric comparison, treat the string Current as 15.0 (last time it was set to such a value).

Anyone have a different preference?

[robmen]

I have no real preference as any one of those three should "unbreak" the WiX Toolset. So consider this just a :+1: for the fix in 16.0. :smiley:

[nguerrera]

I think I like (1) best.

But to be sure, how/where is VisualStudioVersion maintained?

[nguerrera]

So literally 'Current' == '16.0' will be true. Or only '$(MSBuildToolsVersion)' == '16.0' ?

[jeffkl]

Is this too breaky to insist that people switch to comparing VisualStudioVersion or MSBuildAssemblyVersion? Obviously if the impact is tremendous, I'd vote to make it work. But if we can get away with it, I'd rather migrate people to a different property since the concept of "tools version" was deprecated in 15.0.

[robmen]

We've actually removed the use of MSBuildToolsVersion in the next version of the WiX Toolset. The problem is there are lots of existing users that have the "old" targets that are designed to work with both MSBuild 3.5 and MSBuild 4.0. The Current change breaks all of our existing users.

[rainersigwald]

But to be sure, how/where is VisualStudioVersion maintained?

It changes once/VS version, and is defined here:

https://github.com/Microsoft/msbuild/blob/e03296fe14152b4c84e6610d2fd2fd10f1ecba82/src/Shared/Constants.cs#L59-L62

So literally 'Current' == '16.0' will be true. Or only '$(MSBuildToolsVersion)' == '16.0' ?

Conveniently, it looks like enough state is passed down that I can do the latter. Which makes me feel better about the whole thing.

Is this too breaky to insist that people switch to comparing VisualStudioVersion or MSBuildAssemblyVersion?

I think so based on the feedback we've gotten from the limited preview users so far, including WiX here. Especially with the scoping to only the right variable expansion, this is a cheap compat shim for a real problem and I think it's worth doing.

[chrpai]

What version of the VS 2019 preview will this show up in? For example I just updated to preview 2.2.

[rainersigwald]

@chrpai preview 4, which is not yet released (it's not in preview 3 which came out today).

[chrpai]

@rainersigwald Thank you very much!

[natemcmaster]

In the meantime, is there a workaround? We're using Wix 3.11 in https://github.com/aspnet/AspNetCore, and this is one of the last issues preventing us from upgrading to VS2019. I tried overriding MSBuildToolsVersion, but it's reserved property. And based on the way wix.targets is written, I don't think I can avoid evaluation of code that compares MSBuildToolsVersion

error MSB4004: The "MSBuildToolsVersion" property is reserved, and cannot be modified.

[chrpai]

@natemcmaster email me and I can give you information on a work around.

[natemcmaster]

Actually, I think I found a simple workaround for aspnet. It seems the only usage of MSBuildToolsVersion is in $(WixInstallPath)\wix.targets. It's using MSBuildToolsVersion to determine if it needs to import targets for MSBuild 2.0 - 3.5. I can avoid the troublesome evaluation by importing $(WixInstallPath)\wix2010.targets instead since I know my repo definitely has MSBuild > 4.