Move targets, rzc and extension assembly in to the Sdk by pranavkm · Pull Request #2672 · aspnet/Razor

pranavkm · 2018-10-25T22:31:39Z

No description provided.

pranavkm · 2018-10-25T22:34:11Z

    </Compile>
  </ItemGroup>
+
+  <Target Name="LayoutDependencies" BeforeTargets="Build" Condition="'$(IsInnerBuild)' != 'true'">


Weird MSBuild way to make things run a target AfterBuild

What's wrong with AfterTargets="AfterBuild"?

Apparently this is the way to run something after build - dotnet/msbuild#1680 (comment)

pranavkm · 2018-10-25T22:35:17Z

+  </PropertyGroup>
+
+  <PropertyGroup Condition="'$(RazorLangVersion)'==''">
+    <RazorLangVersion Condition="'$(_TargetingNETCoreApp30OrLater)' == 'true'">2.1</RazorLangVersion>


We determine the RazorLangVersion and RazorDefaultConfiguration based on TFM. We'd need for a way to allow class libraries to specify this.

pranavkm · 2018-10-25T22:36:46Z

    <RazorSdkBuildTasksAssembly>$(RazorSdkBuildTasksDirectoryRoot)$(_RazorSdkTasksTFM)\Microsoft.NET.Sdk.Razor.Tasks.dll</RazorSdkBuildTasksAssembly>
+
+    <_TargetFrameworkVersionWithoutV>$(TargetFrameworkVersion.TrimStart('vV'))</_TargetFrameworkVersionWithoutV>
+    <_EnableAllInclusiveRazorSdk Condition="'$(_EnableInclusiveRazorSdk)' == '' AND '$(TargetFrameworkIdentifier)' == '.NETCoreApp' AND '$(_TargetFrameworkVersionWithoutV)' &gt; '2.9'">true</_EnableAllInclusiveRazorSdk>


Needs a name. The SDK pivots on this value to determine if it needs to use the compiler \ targets from the Sdk. A 3.0 RCL would need to be able to specify this.

_EnableAllInclusiveRazorSdk is dope.

Can you log a work item for 3.0 RCL support? I don't think we should try to solve that in this PR

https://github.com/aspnet/Razor/issues/2676

pranavkm · 2018-10-25T22:38:16Z

+    <!--
+      Use the suffix .Views when producing compiled view assemblies. This matches the requirements for Mvc's ViewsFeatureProvider.
+    -->
+    <RazorTargetNameSuffix Condition="'$(_EnableInclusiveRazorSdk)' == '' AND '$(RazorTargetNameSuffix)'==''">.Views</RazorTargetNameSuffix>


Need to play with this - ideally this is set based on the configuration. That said, I'm not super sure this is a particularly important problem to solve.

yeah that seems fine for now.

pranavkm · 2018-10-25T22:40:03Z


            MSBuildIntegrationTestBase.Project = ProjectDirectory.Create(_originalProjectName, _testProjectName, _baseDirectory, _additionalProjects, _language);
-            MSBuildIntegrationTestBase.TargetFramework = _originalProjectName.StartsWith("ClassLibrary") ? "netstandard2.0" : "netcoreapp2.2";
+            MSBuildIntegrationTestBase.TargetFramework = _originalProjectName.StartsWith("ClassLibrary") ? "netstandard2.0" : "netcoreapp3.0";


Yeah, this is fine for now. We should log an issue for 3.0 RCL support and solve this then.

pranavkm · 2018-10-25T22:41:27Z

  <PropertyGroup>
-    <_RazorMSBuildRoot>$(SolutionRoot)src\Microsoft.AspNetCore.Razor.Design\bin\$(Configuration)\</_RazorMSBuildRoot>
-    <RazorSdkBuildTasksDirectoryRoot>$(RazorSdkProjectDirectory)bin\$(Configuration)\</RazorSdkBuildTasksDirectoryRoot>
+    <RazorSdkDirectoryRoot>$(RazorSdkProjectDirectory)bin\$(Configuration)\sdk-output\</RazorSdkDirectoryRoot>


Building the sdk lays out binaries the same way it looks like in the package. Makes it easy to have one property to define all paths.

rynowak · 2018-10-26T14:56:43Z

👏 👏

rynowak · 2018-10-26T18:35:57Z

-            "tools/Microsoft.CodeAnalysis.CSharp.dll": "This assembly is not owned by us and does not follow our conventions.",
-            "tools/Microsoft.CodeAnalysis.dll": "This assembly is not owned by us and does not follow our conventions."
+            "tools/netcoreapp2.0/Microsoft.CodeAnalysis.CSharp.dll": "This assembly is not owned by us and does not follow our conventions.",
+            "tools/netcoreapp2.0/Microsoft.CodeAnalysis.dll": "This assembly is not owned by us and does not follow our conventions."


Do we need to ship these here now that we are in the SDK? I'm not sure how all of this stuff gets constructed.

Filed https://github.com/aspnet/Razor/issues/2675 to figure this out.

rynowak · 2018-10-26T18:37:40Z

  <PropertyGroup>
    <Description>Razor is a markup syntax for adding server-side logic to web pages. This package contains MSBuild support for Razor.</Description>
-    <TargetFrameworks>netstandard2.0;net46</TargetFrameworks>
+    <TargetFrameworks>netcoreapp2.2;netstandard2.0;net46</TargetFrameworks>


Required to reference Razor.Tools. Once we reference it we can make the build order more reliable and avoid file contention.

rynowak · 2018-10-26T18:39:45Z

+    <!-- Binaries that should be signed by corefx/roslyn -->
+    <ExcludePackageFileFromSigning Include="$(SdkOutputPath)tools\netcoreapp2.0\Microsoft.CodeAnalysis.CSharp.dll" />
+    <ExcludePackageFileFromSigning Include="$(SdkOutputPath)tools\netcoreapp2.0\Microsoft.CodeAnalysis.dll" />
+    <ExcludePackageFileFromSigning Include="$(SdkOutputPath)tools\netcoreapp2.0\runtimes\unix\lib\netstandard1.3\System.Text.Encoding.CodePages.dll" />


This seems really fragile

I agree. Maybe addressing https://github.com/aspnet/Razor/issues/2675 would fix this issue. That said, it's been copied over from the Razor.Design project so this isn't new:

https://github.com/aspnet/Razor/blob/master/src/Microsoft.AspNetCore.Razor.Design/Microsoft.AspNetCore.Razor.Design.csproj#L39-L42

rynowak · 2018-10-26T18:41:07Z

  -->
  <PropertyGroup>
+    <!-- Continue setting this property to maintain compat with legacy Razor.Design -->
+    <IsRazorCompilerReferenced>true</IsRazorCompilerReferenced>


So if you are using the 3.0 SDK on a 2.0 project ..... how does this behave?

This file only gets imported if a property is set (set by default for netcoreapp 3.0 or later projects).

rynowak · 2018-10-26T18:41:47Z

+  <!--
+      Initialize properties and items inferred using the project version. This file is not imported in projects referencing
+      MVC \ Razor 2.2 or earlier since values specified here are provided via targets and props imported from NuGet packages
+      such as Microsoft.AspNetCore.Razor.Design.


Should IsRazorCompilerReferenced be here?

rynowak · 2018-10-26T18:42:35Z

+    <!--
+      MVC uses a ProvideApplicationPartFactoryAttribute on the generated assembly to load compiled views from assembly. Set this to false, to prevent generating this attribute.
+    -->
+    <GenerateProvideApplicationPartFactoryAttribute>true</GenerateProvideApplicationPartFactoryAttribute>


Should this be in a .props? can the user's project override this

Or have a condition

Condition seems better. We want to set these up in 3.0 projects, a props file is too early for checking this.

rynowak · 2018-10-26T18:45:47Z

+  -->
+  <PropertyGroup Condition="'$(RazorDefaultConfiguration)'==''">
+    <RazorDefaultConfiguration Condition="'$(_TargetingNETCoreApp30OrLater)' == 'true'">MVC-2.1</RazorDefaultConfiguration>
+  </PropertyGroup>


FYI we have a work item logged to create a 3.0 version of all of these assets. This is fine for now.

rynowak · 2018-10-26T18:47:24Z

+      Defines the ability to understand the configuration for the Razor language service provided by
+      the runtime/toolset packages. Introduced in 2.1
+    -->
+    <ProjectCapability Include="DotNetCoreRazorConfiguration"/>


Should this be conditional? How would this work for a project targeting 2.0 but using a new VS/new SDK?

Yup. I can move this to .Configuration.targets

rynowak · 2018-10-26T18:53:18Z

    <Company>Microsoft</Company>
    <Description>ClassLibrary Description</Description>
+
+    <_EnableAllInclusiveRazorSdk>true</_EnableAllInclusiveRazorSdk>


hmmmm - I think I want to understand this

These are technically 3.0 targeting RCLs (without references to Razor.Design). Consequently we need for a way to tell the SDK to use the targets it ships. Essentially this - https://github.com/aspnet/Razor/issues/2676. There's an existing 2.1 RCL testapp that does not require this

rynowak

Lets discuss some of these deeets

rynowak

based on replies

pranavkm requested a review from rynowak October 25, 2018 22:31

pranavkm force-pushed the prkrishn/move-into-sdk branch 2 times, most recently from b1a2050 to 6797e7f Compare October 25, 2018 22:33

pranavkm commented Oct 25, 2018

View reviewed changes

pranavkm force-pushed the prkrishn/move-into-sdk branch from 6797e7f to a35d348 Compare October 26, 2018 00:29

rynowak reviewed Oct 26, 2018

View reviewed changes

rynowak suggested changes Oct 26, 2018

View reviewed changes

rynowak approved these changes Oct 27, 2018

View reviewed changes

pranavkm force-pushed the prkrishn/move-into-sdk branch from a35d348 to 406d0ca Compare October 30, 2018 20:46

Move targets, rzc and extension assembly in to the Sdk

1b00d79

pranavkm force-pushed the prkrishn/move-into-sdk branch from 406d0ca to 1b00d79 Compare October 30, 2018 20:48

pranavkm merged commit 0bd6d13 into master Oct 30, 2018

pranavkm deleted the prkrishn/move-into-sdk branch October 30, 2018 21:30

Conversation

pranavkm commented Oct 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rynowak commented Oct 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rynowak left a comment

Choose a reason for hiding this comment

Uh oh!

rynowak left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects