Timores commented 2 years ago

When centralizing all package references in a single file (see https://github.com/microsoft/MSBuildSdks/tree/main/src/CentralPackageVersions), using this project causes a compile-time error, as src/MSBuild.SDK.SystemWeb/Sdk/Sdk.targets defines a version for the 2 referenced packages.

Note that a work-around is simple, as one can define the property ExcludeASPNetCompilers and reference the 2 packages directly in one's own csproj.

CZEMacLeod commented 2 years ago

Interesting issue - I'm not entirely sure what you want me to do about it though. As you say, the SDK already has the ExcludeASPNetCompilers flag for this and other scenarios.

I guess the flags etc. could have better documentation. Feel free to submit PRs for the code to add comments, and/or the docs.

I had a look at the MSBuildSdks/CentralPackageVersions source and unfortunately there isn't a property defined to state that CPV is in use and enabled. Unlike with PackageReferences, there isn't a GeneratePathProperty on SDKs either.

Also, because it is imported so late on in the project, it is difficult to actually detect that the SDK is there.

The best I could come up with was:

Directory.Build.props

<Project>
    <ItemGroup Condition="'$(IncludeUnversionedASPNetCompilers)'=='true'">
        <PackageReference Include="Microsoft.Net.Compilers.Toolset" PrivateAssets="All"/>
        <PackageReference Include="Microsoft.CodeDom.Providers.DotNetCompilerPlatform" />
    </ItemGroup>
</Project>

Directory.Build.targets

<Project>
    <Import Project="Sdk.props" Sdk="Microsoft.Build.CentralPackageVersions" Version="2.0.1" />

    <!-- Your content here -->

    <Import Project="Sdk.targets" Sdk="Microsoft.Build.CentralPackageVersions" Version="2.0.1" />

    <PropertyGroup Condition="'$(EnableCentralPackageVersions)'!='false' AND '$(CentralPackagesFile)'!=''">
        <IncludeUnversionedASPNetCompilers Condition="'$(ExcludeASPNetCompilers)'=='false'">true</IncludeUnversionedASPNetCompilers>
        <ExcludeASPNetCompilers>true</ExcludeASPNetCompilers>
    </PropertyGroup>
</Project>

Packages.props

<?xml version="1.0" encoding="utf-8"?>
<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
    <ItemGroup>
        <PackageReference Update="Microsoft.Net.Compilers.Toolset"                      Version="4.0.1" />
        <PackageReference Update="Microsoft.CodeDom.Providers.DotNetCompilerPlatform"   Version="3.6.0" />
    </ItemGroup>
</Project>

I've checked that this works with other project types in the solution, and multiple MSBuild.SDK.SystemWeb projects and they don't tread on each other's toes. It also allows you to actually set ExcludeASPNetCompilers to true in one project and it work as expected, but not affect others.

I'm just not quite sure of how we could adapt that to work inside the MSBuild.SDK.SystemWeb SDK itself right now to do the auto-detect.

I'm also not sure if it is something that should actually be in the SDK itself.

@Timores Can you let me know what you think is the way forward for this issue so that we can close it out.

Timores commented 2 years ago

Thanks a lot for looking at this so quickly. I could not find a way to auto-detect the use of CentralPackageVersion either.

My idea was to add support within MsBuild.SDK.SystemWeb for a property like IncludeUnversionedASPNetCompilers.

Like changing lines 6-9 of MSBuild.SDK.SystemWeb/src/MSBuild.SDK.SystemWeb/Sdk/Sdk.targets with

  <ItemGroup Condition="'$(ExcludeASPNetCompilers)'=='false'" and '$(IncludeUnversionedASPNetCompilers)' == 'false'>
    <PackageReference Include="Microsoft.Net.Compilers.Toolset" Version="3.8.0" PrivateAssets="All"/>
    <PackageReference Include="Microsoft.CodeDom.Providers.DotNetCompilerPlatform" Version="3.6.0" />
  </ItemGroup>
  <ItemGroup Condition="'$(ExcludeASPNetCompilers)'=='false'" and '$(IncludeUnversionedASPNetCompilers)' == 'true'>
    <PackageReference Include="Microsoft.Net.Compilers.Toolset" PrivateAssets="All"/>
    <PackageReference Include="Microsoft.CodeDom.Providers.DotNetCompilerPlatform" />
  </ItemGroup>

I am not sure to have understood your suggestion above (I am a noob with MSBuild and custom SDKs). What I understood is a suggestion to adapt the files of a solution using both CentralPackageVersions and SystemWeb. The changes here seem simpler to me, but your opinion in this case is worth more than mine.

CZEMacLeod commented 2 years ago

@Timores I would need to check the evaluation order - but your suggestion is reasonable. It basically puts what I put in Directory.Build.props into the Sdk.targets file.

You would still need the following changes in Directory.Build.targets

Directory.Build.targets

<Project>
    <Import Project="Sdk.props" Sdk="Microsoft.Build.CentralPackageVersions" Version="2.0.1" />

    <!-- Your content here -->

    <Import Project="Sdk.targets" Sdk="Microsoft.Build.CentralPackageVersions" Version="2.0.1" />

    <PropertyGroup Condition="'$(EnableCentralPackageVersions)'!='false' AND '$(CentralPackagesFile)'!=''">
        <IncludeUnversionedASPNetCompilers Condition="'$(ExcludeASPNetCompilers)'=='false'">true</IncludeUnversionedASPNetCompilers>
    </PropertyGroup>
</Project>

or you could manually set IncludeUnversionedASPNetCompilers in your MsBuild.SDK.SystemWeb project(s).

The problem is that you cannot detect the CentralPackagesFile until you have imported the Sdk.targets file of Microsoft.Build.CentralPackageVersions which happens after the Sdk.targets file of MsBuild.SDK.SystemWeb

This is why you cannot use

<Sdk Name="Microsoft.Build.CentralPackageVersions" Version="2.0.1" />

and have to use the expanded form

<Import Project="Sdk.props" Sdk="Microsoft.Build.CentralPackageVersions" Version="2.0.1" />
<!-- Your content here -->
<Import Project="Sdk.targets" Sdk="Microsoft.Build.CentralPackageVersions" Version="2.0.1" />

and then put the detection code after that.

Let me think on this more; I am not sure how many people this change could impact, or how many people are using both systems together versus the amount of effort in adding these changes.

Timores commented 2 years ago

Indeed, the solution should not be too complex. I currently set ExcludeASPNetCompilers to true in my Web project. Which is not a big deal. Could you explain the role of the 2 packages that are not referenced? What would not work in a Web projet without them? (I have no issue adding them manually,)

CZEMacLeod commented 2 years ago

@Timores I have actually had feedback from @jeffkl in https://github.com/microsoft/MSBuildSdks/issues/320 regarding detecting CPV. With regards to the two packages

Microsoft.Net.Compilers.Toolset - This forces the latest (or specific) compiler tools to be used, regardless of the version installed with MSBuild and/or Visual Studio. It is mainly helpful to build servers which may have a different/older version of the compilers installed.
Microsoft.CodeDom.Providers.DotNetCompilerPlatform - This forces webforms type files to compile on the webserver with the latest roslyn based compilers rather than the native ones built into .NET 4.x. This allows you to use newer language features in your markup pages when they are compiled using aspnet_compiler or on the server. It effectively replaces the native implementations when you (or the framework) calls System.CodeDom.Compiler.CodeDomProvider. It also contains a number of fixes and workarounds for issues where the background compiler service is still running when the webapplication terminates, which may lock files in you bin folder which then prevents you from re-compiling.

For the latter, you need to make sure that the follow lines are in your web.config (and if you don't include the package, remove them - although you then lose the benefits).

  <system.codedom>
    <compilers>
      <compiler language="c#;cs;csharp" extension=".cs" type="Microsoft.CodeDom.Providers.DotNetCompilerPlatform.CSharpCodeProvider, Microsoft.CodeDom.Providers.DotNetCompilerPlatform, Version=3.6.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35" warningLevel="4" compilerOptions="/langversion:default /nowarn:1659;1699;1701" />
      <compiler language="vb;vbs;visualbasic;vbscript" extension=".vb" type="Microsoft.CodeDom.Providers.DotNetCompilerPlatform.VBCodeProvider, Microsoft.CodeDom.Providers.DotNetCompilerPlatform, Version=3.6.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35" warningLevel="4" compilerOptions="/langversion:default /nowarn:41008 /define:_MYTYPE=\&quot;Web\&quot; /optionInfer+" />
    </compilers>
  </system.codedom>

You also need to ensure the version number shown in the web.config matches the assembly version of the package. The following blog post describes the first version of this package - although I'm sure there are newer/better articles. Enabling the .NET Compiler Platform (“Roslyn”) in ASP.NET applications

With the release of Microsoft.Build.CentralPackageVersions 2.1.1, I can re-evaluate how difficult it would be to integrate the detection code. Hopefully I will be able to address that soon, time permitting.

Timores commented 2 years ago

Thanks a lot

CZEMacLeod / MSBuild.SDK.SystemWeb

Using Microsoft.Build.CentralPackageVersions causes a compile-time error #26

Directory.Build.props

Directory.Build.targets

Packages.props

Directory.Build.targets