search

novotnyllc / MSBuildSdkExtras

Extra properties for MSBuild SDK projects
MIT License
347 stars 42 forks source link

LanguageTargets should be set using MSBuildToolsVersion instead of VisualStudioVersion #127

Closed rainersigwald closed 5 years ago

rainersigwald commented 5 years ago

MSBuild is changing its ToolsVersion to no longer match VisualStudioVersion in order to make future updates to either easier. https://github.com/Microsoft/msbuild/issues/3778 has details.

@AArnott reported an error on an (internal dogfood) Visual Studio that has this change:

C:\Program Files\dotnet\sdk\2.1.300\Sdks\Microsoft.NET.Sdk\Sdk\Sdk.targets(41,3): error MSB4019: The imported project "C:\Program Files (x86)\Microsoft Visual Studio\2019\Preview\MSBuild\16.0\Bin\Microsoft.CSharp.targets" was not found. Confirm that the path in the <Import> declaration is correct, and that the file exists on disk.

The project in question uses MSBuild.Sdk.Extras, so I think what's happened is that it hit

https://github.com/onovotny/MSBuildSdkExtras/blob/8a15e5f4c67cc604231bd4715bf9fe2d66d7b5b0/Source/MSBuild.Sdk.Extras/Build/Platforms/NETFramework.targets#L13

Which is imported by

https://github.com/dotnet/sdk/blob/a120e9168437f0da045ad58259cb5022622e2c15/src/Tasks/Microsoft.NET.Build.Tasks/sdk/Sdk.targets#L41

To support dev16/MSBuild tools version Current, the path to C#/VB targets should be set using $(MSBuildToolsVersion) instead of $(VisualStudioVersion). Or using $(MSBuildToolsPath), as the SDK does.

clairernovotny commented 5 years ago

@rainersigwald would changing usage of VisualStudioVersion to be MSBuildToolsVersion be backwards compatible? I.e., does existing MSBuild set that to the correct version?

Happy to make the change, just looking to understand the impact and not break anyone :)

rainersigwald commented 5 years ago

The history of the two is kinda complicated but since this SDK works only in dev15+ it is backward compatible.

In the beginning, MSBuildToolsVersion was the .NET Framework marketing version (2.0, 3.5, 4.0, 4.5). When MSBuild started shipping with VS rather than the framework, it moved to match VisualStudioVersion (12.0, 14.0, 15.0). Now it's moving to Current with VisualStudioVersion=16.0.

rainersigwald commented 5 years ago

I'm not 100% sure this is the root cause of the reported problem; I think it might have gotten fixed with other changes from the 1.5.x branch to now--checking.

rainersigwald commented 5 years ago

~Confirmed: Nerdbank.Streams builds successfully using MSBuild.Sdk.Extras 1.6.60.~ The remaining uses should still be changed but I think they affect only a fallback case--in 1.5.4, LanguageTargets was set unconditionally, where now it's only if it's unset:

https://github.com/onovotny/MSBuildSdkExtras/blob/cf5e073e75dd1e8180d2fbe2498ded696158cba9/MSBuild.Sdk.Extras/build/platforms/languageTargets/Wpf.targets#L7-L9

Edit: just using a newer MSBuild.Sdk.Extras is not sufficient. It allows a workaround of specifying $(LanguageTargets) correctly in the body of an affected project file, though, which is what I saw when testing.

clairernovotny commented 5 years ago

Any chance you/@AArnott could PR the change to ensure it works the way you expect? I don't have any way to test it.

If you build the sdk package locally, it installs a version 42.42.42 that you can then use as an sdk reference for testing.

rainersigwald commented 5 years ago

Yeah, I can test it, just a minute.

clairernovotny commented 5 years ago

If there are additional changes that should be made so that these compose with the WPF targets better, I'm also interested... right now, they both import the .NET sdk.

I'm guessing that there are many workarounds that can be removed if building with the 3.0 sdk and deferring to the WPF targets in there.

rainersigwald commented 5 years ago

I manually patched wpf.targets in my 1.5.4 instance and it worked:

diff --git "a/S:\\nuget\\packages\\msbuild.sdk.extras\\1.5.4\\build\\platforms\\languageTargets\\Wpf.targets.1.5.4" "b/S:\\nuget\\packages\\msbuild.sdk.extras\\1.5.4\\build\\platforms\\languageTargets\\Wpf.targets"
index ccbbba7..d5532d7 100644
--- "a/S:\\nuget\\packages\\msbuild.sdk.extras\\1.5.4\\build\\platforms\\languageTargets\\Wpf.targets.1.5.4"
+++ "b/S:\\nuget\\packages\\msbuild.sdk.extras\\1.5.4\\build\\platforms\\languageTargets\\Wpf.targets"
@@ -6,7 +6,7 @@

   <PropertyGroup>
     <!-- Workaround for lack of XAML support in the new project system -->
-    <LanguageTargets Condition="'$(_SdkLanguageName)' != 'FSharp'" >$(MSBuildExtensionsPath)\$(_SdkVisualStudioVersion)\Bin\Microsoft.$(_SdkLanguageName).targets</LanguageTargets>
+    <LanguageTargets Condition="'$(_SdkLanguageName)' != 'FSharp'" >$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Bin\Microsoft.$(_SdkLanguageName).targets</LanguageTargets>
     <LanguageTargets Condition="'$(_SdkLanguageName)' == 'FSharp'" >$(MSBuildExtensionsPath)\Microsoft\VisualStudio\v$(_SdkVisualStudioVersion)\FSharp\Microsoft.FSharp.Targets</LanguageTargets>

     <!-- Don't generate assembly info for *.tmp_proj files -->

Likewise I patched current HEAD and switched to using it and it worked. Sent #128--feel free to close/rebase/whatever; sent it since I had to do 90% of it to test.

rainersigwald commented 5 years ago

I was wrong: 1.6.60 does hit this. But it enables a consumer to work around it by specifying LanguageTargets in the project:

    <LanguageTargets>$(MSBuildToolsPath)\Microsoft.CSharp.targets</LanguageTargets>

In my earlier test I had tried that, it failed, then I forgot to revert it before testing the package update.

clairernovotny commented 5 years ago

Thanks. 1.6.61 is rolling out to NuGet now with the fix