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

8776: Upgrade Microsoft.CodeDom.Providers.DotNetCompilerPlatform to latest version #8777

Merged
merged 17 commits into from
Apr 4, 2024

Conversation

BenedekFarkas
Copy link
Member

@BenedekFarkas BenedekFarkas commented Mar 18, 2024

Fixes #8776

Why do we need this package at all:

  1. Dynamic Compilation, as in modify C# code and the module will be recompiled and loaded into memory when reloading the page - it's a useful developer feature.
  2. VS Razor IntelliSense: The ASP.NET Compiler that ships with .NET Framework, even in the latest versions is an old one that doesn't support C# language features above version 5. The projects that have Razor files need this DLL to be loaded in memory (but AFAIR not necessarily strict about having it as a reference, just need to have some web.config entries).
  3. Static Razor compilation: When building the solution with MvcBuildViews=true, Razor files are compiled in-memory as an extra validation step (but they remain as .cshtml files, so they aren't compiled into a DLL like in ASP.NET Core). This is the pickiest out of these 3 features and every project that has Razor files needs this DLL reference (and some configuration).

Further changes and details besides package update:

  • Overrode /langversion:default that comes from the NuGet package upgrade in web.configs and set it to 7.3.
    The officially supported C# language version since .NET Framework 4.8 is 7.3 (and it probably won't go higher than that due to netstandard2.1). It is possible to build a project using a higher version (because it depends on which version your VS supports), but according to Microsoft it's not recommended to go higher than that due to potential runtime issues.
  • Instead of having <LangVersion>latest</LangVersion> in csprojs for both the Release and Debug configuration, <LangVersion>7.3</LangVersion> is set just once regardless of configuration.
  • "Downgraded" a small piece of src\Orchard.Web\Modules\Orchard.Email\Services\SmtpMessageChannel.cs to C# 7.3.
  • We had <MvcBuildViews>false</MvcBuildViews> in the csprojs that use this package until now, we don't need it anymore.
  • The CopyRoslynFilesToOutputFolder target from Orchard.Web.csproj is removed, because we're using the target with the same purpose (CopyRoslynCompilerFilesToOutputDirectory) supplied by the package.
  • On top of the previous, Orchard.Web also defines RoslynToolPath explicitly (= where the tools are copied from). Without it the tools are only copied on the 2nd-3rd rebuild for some reason.
  • Orchard.proj: Factored out a part of the Compile target into the DevCompile target and removed the BuildViews target.
    • For a simple local build (just to validate that the solution builds successfully, but don't want to run any tests), we don't actually need that second step that copies files to the output folder, so you can just use DevCompile instead of Compile.
    • CI builds (Test, Spec, etc.) that operate on the build output folder are unaffected, because Compile calls DevCompile.
    • The BuildViews target is not really necessary, just call any other target (Compile, DevCompile, Spec, etc.) with /p:MvcBuildViews=true instead.

@BenedekFarkas BenedekFarkas self-assigned this Mar 18, 2024
@MatteoPiovanelli-Laser
Copy link
Contributor

I have two questions:
Why is this pinning the LangVersion?
Do we need this package in so many projects?

@@ -7,7 +7,8 @@
<!-- Registering Roslyn as a compiler for Razor IntelliSense. -->
<system.codedom>
<compilers>
<compiler language="c#;cs;csharp" extension=".cs" type="Microsoft.CodeDom.Providers.DotNetCompilerPlatform.CSharpCodeProvider, Microsoft.CodeDom.Providers.DotNetCompilerPlatform, Version=2.0.1.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35" warningLevel="4" compilerOptions="/langversion:latest" />
<compiler language="c#;cs;csharp" extension=".cs" warningLevel="4" compilerOptions="/langversion:7.3 /nowarn:1659;1699;1701;612;618" type="Microsoft.CodeDom.Providers.DotNetCompilerPlatform.CSharpCodeProvider, Microsoft.CodeDom.Providers.DotNetCompilerPlatform, Version=4.1.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35" />
<compiler language="vb;vbs;visualbasic;vbscript" extension=".vb" warningLevel="4" compilerOptions="/langversion:default /nowarn:41008,40000,40008 /define:_MYTYPE=\&quot;Web\&quot; /optionInfer+" type="Microsoft.CodeDom.Providers.DotNetCompilerPlatform.VBCodeProvider, Microsoft.CodeDom.Providers.DotNetCompilerPlatform, Version=4.1.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why add VB compiler?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's added automatically when you install/update the package. We can remove it, but it doesn't do anything and leaving it there makes updating this package through VS NuGet package management easier.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will trust you. I feel like this was removed for a reason though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, you're right that it's unnecessary (but it doesn't affect anything), but it was easy to get rid of them, so it's better without it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got rid of other stuff too from all the csprojs (other than Orchard.Web.csproj) that we don't actually need, but they will be re-added automatically when we update the package again through VS, so we can just not commit those changes.

On the other hand, I've re-added the VB compiler configuration to Orchard.Web's Web.config, because rebuilding the solution restores it, but that's OK, it doesn't do anything.

@BenedekFarkas
Copy link
Member Author

I have two questions: Why is this pinning the LangVersion? Do we need this package in so many projects?

Version pinning: The officially supported C# language version since .NET Framework 4.8 is 7.3 (and it probably won't go higher than that due to netstandard2.1). It is possible to build a project using a higher version (because it depends on which version your VS supports), but according to Microsoft it's not recommended to go higher than that due to potential runtime issues.


Package reference: This whole saga of adding Microsoft.CodeDom.Providers.DotNetCompilerPlatform.CSharpCodeProvider to the solution/projects serves 3 purposes - the necessary configuration has some overlap between them.

  1. Dynamic Compilation, as in modify C# code and the module will be recompiled and loaded into memory when reloading the page - it's a useful developer feature.
  2. VS Razor IntelliSense: The ASP.NET Compiler that ships with .NET Framework, even in the latest versions is an old one that doesn't support C# language features above version 5. The projects that have Razor files need this DLL to be loaded in memory (but AFAIR not necessarily strict about having it as a reference, just need to have some web.config entries).
  3. Static Razor compilation: When building the solution with MvcBuildViews=true, Razor files are compiled in-memory as an extra validation step (but they remain as .cshtml files, so they aren't compiled into a DLL like in ASP.NET Core). This is the pickiest out of these 3 features and every project that has Razor files needs this DLL reference (and some configuration).

…NetCompilerPlatform

See https://github.com/aspnet/RoslynCodeDomProvider?tab=readme-ov-file#build-time-options
- We only need Orchard.Web to include the Roslyn tools in its bin folder, the other csprojs only need the DLL reference
- We could simply remove the targets import in these csprojs, but it will be re-added when the package is updated, so this is cleaner/easier
…ided target (CopyRoslynCompilerFilesToOutputDirectory) instead of our custom one
…Compile target and removing BuildViews target

- For a simple local build (just to validate that the solution builds), we don't actually need that second build that copies files to the output folder.
- CI builds (Test, Spec, etc.) that operate on the build output folder are unaffected, because Compile calls DevCompile
- The BuildViews target is not really necessary, just call any other target (Compile, DevCompile, Spec, etc.) with "/p:MvcBuildViews=true"
@BenedekFarkas
Copy link
Member Author

Made further changes and explained them in the PR description + added my previous answers there too, so they are all in one place.

@BenedekFarkas BenedekFarkas merged commit fb1aa73 into 1.10.x Apr 4, 2024
2 checks passed
BenedekFarkas added a commit that referenced this pull request Apr 4, 2024
…n 4.1.0 in dev-only extensions too

Continuation of #8777
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.

Upgrade Microsoft.CodeDom.Providers.DotNetCompilerPlatform to latest version
3 participants