Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Mar 15, 2024

Conversation

dmhiggins23
Copy link
Contributor

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

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
Copy link
Contributor Author

@dotnet-policy-service agree

@sebastienros
Copy link
Member

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

@dmhiggins23
Copy link
Contributor Author

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

secureSocketOptions = smtpConfiguration.EncryptionMethod switch {

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

[Obsolete($"Use {nameof(AutoSelectEncryption)} and/or {nameof(EncryptionMethod)} instead.")]

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
Copy link
Member

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

@sebastienros
Copy link
Member

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

@MatteoPiovanelli MatteoPiovanelli merged commit 1900b35 into OrchardCMS:1.10.x Mar 15, 2024
2 checks passed
@BenedekFarkas
Copy link
Member

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
Copy link
Contributor Author

I believe the language features in question are:

  • switch expressions (C# version 8?)
  • Constant interpolated strings (required for attributes; C# version 10?)

@BenedekFarkas
Copy link
Member

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.

@BenedekFarkas BenedekFarkas linked an issue Apr 4, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Building latest 1.10.x from command line
4 participants