search

OrchardCMS / Orchard

Orchard is a free, open source, community-focused Content Management System built on the ASP.NET MVC platform.
https://orchardproject.net
BSD 3-Clause "New" or "Revised" License
2.37k stars 1.12k forks source link

Updated ClickToBuild.cmd to support VS2019 (v16) and VS2022 (v17) #8764

Closed dmhiggins23 closed 3 months ago

dmhiggins23 commented 4 months ago

Removed support for eariler MSBuild versions (they cannot compile SmtpMessageChannel.cs)

Updated lib/nuget/nuget.exe to latest version (6.9.1.3) (required for package restoration)

Modified Obsolete attribute in SmtpSettingsPart.cs so the solution builds with VS2019

dmhiggins23 commented 4 months ago

@dotnet-policy-service agree

sebastienros commented 4 months ago

This is removing support for VS 11,12,14,and 15? Is there a reason to not support them anymore?

dmhiggins23 commented 4 months ago

To my knowledge, the version of msbuild that comes with those earlier versions does not support the advanced language features that are currently in SmtpMessageChannel.cs

https://github.com/OrchardCMS/Orchard/blob/50d416c9f2b1ebecd56bfbb0c8933768493fc89f/src/Orchard.Web/Modules/Orchard.Email/Services/SmtpMessageChannel.cs#L157

So, even if ClickToBuild.cmd found and used one of those earlier versions. The build would fail. As it was, I updated SmtpSettingsPart.cs so version 16 (vs 2019) would work

https://github.com/OrchardCMS/Orchard/blob/50d416c9f2b1ebecd56bfbb0c8933768493fc89f/src/Orchard.Web/Modules/Orchard.Email/Models/SmtpSettingsPart.cs#L34

I only tested vs 2017 and later. The current head of the dev branch compiles with Visual Studio 2017 (version 15). The current head of the 1.10.x branch does not. The problem appears to be restricted to the files I referenced above.

If you want to keep the door open for the earlier versions of msbuild in ClickToBuild.cmd, I can redo it so that it just adds support for versions 16 and 17, and leave support for the other versions in there. Then you could defer the question of the use of the advanced c# features to a later time. It is probably the most conservative approach.

sebastienros commented 3 months ago

Let's merge and see who would be broken. @MatteoPiovanelli-Laser are you using a recent version of VS ? > 15

sebastienros commented 3 months ago

@MatteoPiovanelli-Laser please merge if this doesn't break you

BenedekFarkas commented 3 months ago

Interpolated strings were introduced in C# 6 and we were supposed to support C# 7.3 - at least in dev. The above changes (= the fact that there was only one file that was incompatible with VS 2017) tell me that we intentionally did not use higher-version language features (but I don't remember :( ), but in that case the csprojs should reflect (by language version restriction) which VS version we want to remain compatible with.

Better yet, setting langversion to default will follow the .NET Framework version, which is already 4.8 on both 1.10.x and dev, where 7.3 is the default, which is compatible with VS 2019 (VS version 16).

We were actually restricting the language version specifically to 7.3 in each csproj, but it was overwritten to "latest" some time ago. That's a separate problem, because AFAIR it's still not recommended to go above 7.3 for .NET FW applications due to possible runtime issues, even if it compiles.

In any case, at least according to this Wikipedia article, even VS 2015's MSBuild should be able to compile interpolated strings, so if that's true, then the changes in this PR are in conflict with each other.

dmhiggins23 commented 3 months ago

I believe the language features in question are:

BenedekFarkas commented 3 months ago

Oh, I see, that's because the interpolated string is used in an attribute that requires a constant. I glossed over that, you're right! Thanks for the clarification.