-
Notifications
You must be signed in to change notification settings - Fork 15
Conversation
Hi @ryanbrandenburg, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! The agreement was validated by .NET Foundation and real humans are currently evaluating your PR. TTYL, DNFBOT; |
Project("{8BB2217D-0F2D-49D1-97BC-3654ED321F3B}") = "Microsoft.AspNetCore.Mvc.Razor.ViewCompilation.Design.Test", "test\Microsoft.AspNetCore.Mvc.Razor.ViewCompilation.Design.Test\Microsoft.AspNetCore.Mvc.Razor.ViewCompilation.Design.Test.xproj", "{E0D75B4E-839F-4F80-9B1F-B33F616BCC5F}" | ||
EndProject | ||
Project("{8BB2217D-0F2D-49D1-97BC-3654ED321F3B}") = "Microsoft.AspNetCore.Mvc.Razor.ViewCompilation.Design", "src\Microsoft.AspNetCore.Mvc.Razor.ViewCompilation.Design\Microsoft.AspNetCore.Mvc.Razor.ViewCompilation.Design.xproj", "{D7F82DFE-7570-4824-9B2B-67255443D262}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we rename this package to Microsoft.AspNetCore.Mvc.Razor.ViewCompilation
? cc @Eilon \ @DamianEdwards
Code organization \ engineering issues aside, we can now make Microsoft.AspNetCore.Mvc
depend on this package. By setting <MvcBuildViews>true</MvcBuildViews>
(or something like that) in your project, we can make view compilation just work so you wouldn't actually need to know the package name anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DamianEdwards would have to be the one to answer that.
@@ -6,11 +6,10 @@ | |||
</PropertyGroup> | |||
<Import Project="$(VSToolsPath)\DotNet\Microsoft.DotNet.Props" Condition="'$(VSToolsPath)' != ''" /> | |||
<PropertyGroup Label="Globals"> | |||
<ProjectGuid>4339fc9b-aec6-442a-b413-a41555ed76c7</ProjectGuid> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we keep this Guid the same? Fewer diffs
<serviceable>true</serviceable> | ||
</metadata> | ||
<files> | ||
<file src="artifacts/Microsoft.AspNetCore.Mvc.Razor.ViewCompilation.Design.dll" target="build/" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the structure of the package look like? Also, does this work for net451
targeting apps? Might also need the crossTargeting target (https://github.com/aspnet/RazorTooling/blob/nimullen/msbuild.93/src/Microsoft.AspNetCore.Razor.Tools/buildCrossTargeting/Microsoft.AspNetCore.Razor.Tools.targets) to make this work for multi-targeting projects.
<id>Microsoft.AspNetCore.Mvc.Razor.ViewCompilation.Design</id> | ||
<version>1.2.0</version> | ||
<authors>Microsoft</authors> | ||
<owners>Microsoft</owners> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove
<package xmlns="http://schemas.microsoft.com/packaging/2012/06/nuspec.xsd"> | ||
<metadata> | ||
<id>Microsoft.AspNetCore.Mvc.Razor.ViewCompilation.Design</id> | ||
<version>1.2.0</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -1,6 +1,5 @@ | |||
{ | |||
"projects": [], | |||
"sdk": { | |||
"version": "1.0.0-preview2-1-003177" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.0.0-preview2-1-003180
@@ -1,6 +1,6 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<configuration> | |||
<packageSources> | |||
<add key="LocalArtifacts" value="../artifacts/build" /> | |||
<add key="LocalArtifacts" value="../artifacts" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We require the artifacts/build structure for Universe to be able to pick up packages.
@@ -0,0 +1,6 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change this to msbuild? It's weird to have a repo that's partially csproj and project.json.
@@ -0,0 +1,45 @@ | |||
<Project Sdk="Microsoft.NET.Sdk" ToolsVersion="15.0"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Microsoft.NET.Sdk.Web
<Version>1.2.0-*</Version> | ||
</PackageReference> | ||
<PackageReference Include="Microsoft.NETCore.App"> | ||
<Version>1.0.0-*</Version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.2.0-*
76f2d4d
to
498a890
Compare
🆙📅 |
<ExecArgs Condition="$(DelaySign) == true" | ||
Include="--delay-sign" /> | ||
</ItemGroup> | ||
<Exec Command="dotnet exec @(ExecArgs->'%(Identity)', ' ')" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the target effectively just executes the Microsoft.AspNetCore.Mvc.Razor.ViewCompilation.Design.dll
using dotnet exec
? Why isn't it just a task? Is this how we execute our "inside man"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We played around with converting it to a Task. Unfortunately dotnet msbuild
(the one's in the 1.0.0-preview4-004233 SDK) uses the 1.0.1 shared runtime. Precompilation references Mvc which is compiled against CoreFx 4.4.0 packages i.e versions newer than the one that are in the shared runtime running the task. This fails at runtime with errors like
D:\temp\msbuild-test\srcD:\temp\msbuild-test\src\src.csproj(4,5): error MSB4018: The "TestTask" task failed unexpectedly.\r
D:\temp\msbuild-test\src\src.csproj(4,5): error MSB4018: System.IO.FileNotFoundException: Could not load file or assembly 'System.Collections.Immutable, Version=1.2.2.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. The system cannot find the file specified.\r
D:\temp\msbuild-test\src\src.csproj(4,5): error MSB4018: File name: 'System.Collections.Immutable, Version=1.2.2.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'\r
D:\temp\msbuild-test\src\src.csproj(4,5): error MSB4018: at Microsoft.Test.TestTask.Test()\r
D:\temp\msbuild-test\src\src.csproj(4,5): error MSB4018: at Microsoft.Test.TestTask.Execute()\r
D:\temp\msbuild-test\src\src.csproj(4,5): error MSB4018: at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute()\r
D:\temp\msbuild-test\src\src.csproj(4,5): error MSB4018: at Microsoft.Build.BackEnd.TaskBuilder.<ExecuteInstantiatedTask>d__25.MoveNext()
Super simple repro here: https://github.com/pranavkm/msbuild-repro/tree/master/src. We'll follow up with the MSBuild team next week, but given that we're also going to be loading user code, I think we are going to run in to issues converting it into a task. The current approach avoids these.
@@ -1,20 +1,18 @@ | |||
use namespace="System.IO" | |||
|
|||
-BuildQuality = "preview4"; | |||
- //BuildQuality = "preview4"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
@@ -1,20 +1,18 @@ | |||
use namespace="System.IO" | |||
|
|||
-BuildQuality = "preview4"; | |||
- //BuildQuality = "preview4"; | |||
use-standard-lifecycle | |||
k-standard-goals | |||
|
|||
#repo-initialize target='initialize' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to remove this node entirely. It should probably restore just fine.
- // Temporarily no-op until we have a resolution for https://github.com/aspnet/MvcPrecompilation/issues/48 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should no longer be an issue, so we should remove this and start running tests.
<Import Project="..\build\common.props" /> | ||
|
||
<Target Name="Pack"> | ||
<MSBuild Projects="$(MSBuildThisFileDirectory)..\src\Microsoft.AspNetCore.Mvc.Razor.ViewCompilation.Design\Microsoft.AspNetCore.Mvc.Razor.ViewCompilation.Design.csproj" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you declare <SolutionRoot>$(GetDirectoryNameOfFileAbove(MvcPrecompilation.sln)/</SolutionRoot>
and use that instead of doing $(MSBuildThisFileDirectory)..\
everywhere in this file?
<Import Project="..\..\build\common.props" /> | ||
<PropertyGroup> | ||
<Description>"Build-time references required to enable Razor view compilation as part of building the application."</Description> | ||
<TargetFramework>netcoreapp1.0</TargetFramework> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
netcoreapp1.2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we would have to cross-compile the binary since we load user code in the tool. Does it work for net451 targeting projects?
|
||
<ItemGroup> | ||
<Compile Include="**\*.cs" /> | ||
<EmbeddedResource Include="**\*.resx" Exclude="bin\**;obj\**;**\*.xproj;packages\**" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make this <EmbeddedResource Include="Resources\*"/>
and remove all the exclusions?
|
||
<ItemGroup Condition=" '$(TargetFramework)' == 'netcoreapp1.0' "> | ||
<PackageReference Include="Microsoft.NETCore.App"> | ||
<Version>1.0.0-*</Version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.2.0-*
</PackageReference> | ||
</ItemGroup> | ||
|
||
<PropertyGroup Condition=" '$(Configuration)' == 'Release' "> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove
</PackageReference> | ||
</ItemGroup> | ||
|
||
<PropertyGroup Condition=" '$(Configuration)' == 'Release' "> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove
<Project Sdk="Microsoft.NET.Sdk.Web" ToolsVersion="15.0"> | ||
|
||
<PropertyGroup> | ||
<TargetFramework>netcoreapp1.0</TargetFramework> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
netcoreapp1.2
Just a reminder this needs to work with apps targeting |
<Product>Microsoft ASP.NET Core</Product> | ||
<RepositoryUrl>https://github.com/aspnet/MvcPrecompilation</RepositoryUrl> | ||
<RepositoryType>git</RepositoryType> | ||
<AssemblyOriginatorKeyFile>$(MSBuildThisFileDirectory)../tools/Key.snk</AssemblyOriginatorKeyFile> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the sake of consistency, I've been moving Key.snk into the 'build' folder.
<SignAssembly>true</SignAssembly> | ||
<PublicSign Condition="'$(OS)' != 'Windows_NT'">true</PublicSign> | ||
|
||
<GenerateNeutralResourcesLanguageAttribute>false</GenerateNeutralResourcesLanguageAttribute> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SDK will generate assembly attributes now. You can delete these 4 attributes from AssemblyInfo.cs
@@ -2,8 +2,5 @@ | |||
"projects": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can delete this whole file.
<Project Sdk="Microsoft.NET.Sdk" ToolsVersion="15.0"> | ||
<Import Project="..\..\build\common.props" /> | ||
<PropertyGroup> | ||
<Description>"Build-time references required to enable Razor view compilation as part of building the application."</Description> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the double quotes
<TargetFramework>netcoreapp1.2</TargetFramework> | ||
<PackageTags>cshtml;razor;compilation;precompilation;aspnetcore</PackageTags> | ||
<PreserveCompilationContext>true</PreserveCompilationContext> | ||
<AssemblyName>Microsoft.AspNetCore.Mvc.Razor.ViewCompilation.Design</AssemblyName> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this. This is the default value anyways.
@@ -0,0 +1,47 @@ | |||
<Project Sdk="Microsoft.NET.Sdk" ToolsVersion="15.0"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comments above apply to this file
|
||
<ItemGroup> | ||
<PackageReference Include="Microsoft.NET.Test.Sdk"> | ||
<Version>15.0.0-preview-20161024-02</Version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lift to 15.0.0-preview-20161123-03
|
||
<ItemGroup> | ||
<PackageReference Include="Microsoft.NET.Test.Sdk"> | ||
<Version>15.0.0-preview-*</Version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommend hard coding this to 15.0.0-preview-20161123-03
. The VSTest team has decided (for some reason) to publish nighlies to nuget.org instead of myget.org. They're package is pretty volatile right now.
<Version>15.0.0-preview-*</Version> | ||
</PackageReference> | ||
<PackageReference Include="xunit.runner.visualstudio"> | ||
<Version>2.2.0-beta4-*</Version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be set to 2.2.0-*
.
|
||
<ItemGroup> | ||
<EmbeddedResource Include="**\*.resx" /> | ||
<EmbeddedResource Include="compiler\resources\**\*" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this. Was probably generated by dotnet-migrate, right? Didn't see any files in this folder so it's not necessary.
@@ -3,7 +3,7 @@ Microsoft Visual Studio Solution File, Format Version 12.00 | |||
# Visual Studio 14 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
upgrade this file to dev15. You'll need to change the project GUID type from 8BB2217D-0F2D-49D1-97BC-3654ED321F3B to 9A19103F-16F7-4668-BE54-9A1E7A4F7556, and update these lines:
# Visual Studio 15
VisualStudioVersion = 15.0.26014.0
<!-- This file may be overwritten by automation. Only values allowed here are VersionPrefix and VersionSuffix. --> | ||
<Project> | ||
<PropertyGroup> | ||
<VersionPrefix>1.2.0</VersionPrefix> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this still be 1.2.0, even though it has 1.1.0 dependencies?
<MvcRazorOutputPath Condition="'$(MvcRazorOutputPath)'==''">$(OutputPath)</MvcRazorOutputPath> | ||
<MyGeneratedDll>$(AssemblyName).PrecompiledViews.dll</MyGeneratedDll> | ||
<MyGeneratedDllPath>$(OutputPath)$(MyGeneratedDll)</MyGeneratedDllPath> | ||
<BuildDependsOn>$(BuildDependsOn);MvcRazorPrecompile</BuildDependsOn> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...originally I thought this was the best way to go, but had a chat with MSBuild guys recently. 'AfterTargets' is the preferred way to do this. MSBuild is actually trying to get away from these 'dependson' chains.
9210571
to
0fd59af
Compare
@@ -3,5 +3,6 @@ | |||
<packageSources> | |||
<add key="AspNetCore" value="https://dotnet.myget.org/F/aspnetcore-ci-dev/api/v3/index.json" /> | |||
<add key="NuGet" value="https://api.nuget.org/v3/index.json" /> | |||
<add key="Local" value="D:\work\katana\Hosting\artifacts\build" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Local dev feed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working through tests.
@@ -3,5 +3,6 @@ | |||
<packageSources> | |||
<add key="AspNetCore" value="https://dotnet.myget.org/F/aspnetcore-ci-dev/api/v3/index.json" /> | |||
<add key="NuGet" value="https://api.nuget.org/v3/index.json" /> | |||
<add key="Local" value="D:\work\katana\Hosting\artifacts\build" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Temporarily until I can get aspnet/Hosting#910 in.
@@ -49,5 +51,22 @@ private string GetInformationalVersion() | |||
var attribute = assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>(); | |||
return attribute.InformationalVersion; | |||
} | |||
|
|||
private static string[] ExpandResponseFiles(string[] args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to pass in the ContentFiles
node in rather than scanning for cshtml files to be correct. Setting this up as a first step to pass in args to the tool.
@@ -0,0 +1,42 @@ | |||
<Project Sdk="Microsoft.NET.Sdk.Web" ToolsVersion="15.0"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this and other testapps files can be cleaned up. Remove redundant attributes (AssemblyName), use one-line syntax for PackageReference, remove the unnecessary 'DefineConstants RELEASE' block at the end.
@@ -0,0 +1,2598 @@ | |||
<?xml version="1.0" encoding="utf-8"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this file shouldn't be in source control.
@@ -5,7 +5,3 @@ | |||
using System.Resources; | |||
|
|||
[assembly: AssemblyMetadata("Serviceable", "True")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can remove this. Korebuild feature/msbuild adds this. cref https://github.com/aspnet/KoreBuild/blob/ee0383feffde791108402895fd0c60f5d16f1ad0/build/contentFiles/cs/any/AssemblyInfo.Serviceable.cs
|
||
<ItemGroup> | ||
<Compile Include="**\*.cs" /> | ||
<None Include="build/**/*.*" Pack="true" PackagePath="build/%(RecursiveDir)%(FileName)%(Extension)" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be Include="build\**\*"
in case there are extensionless files. Also, MSBuild convention is backslashes instead of forwardslashes (even on xplat).
Also, you can simplify to PackagePath="%(Identity)"
0fd59af
to
195658b
Compare
🆙 📅 |
195658b
to
94bba14
Compare
94bba14
to
00b24f5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per in-person discussion with Pranav, I suggest you look into including the assembly in the @(IntermediateAssembly)
item group. But this can probably be done later as (from inspection) it seems the current targets produce the desired output.
@@ -0,0 +1,23 @@ | |||
<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | |||
<Import Project="../common.targets" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use $(MSBuildThisFileDirectory)
@@ -0,0 +1,18 @@ | |||
<!-- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should be moved to buildMultiTargeting. Cref NuGet/Home#4098
@pranavkm has already made these fixes and merged them in, so I'm closing this out. |
Including Microsoft.AspNetCore.Mvc.Razor.ViewCompilation.Design should now add the Precompile target, which will run after publishing.