Add support for pre-compiling desktop applications. #130

NTaylorMullen · 2017-05-26T23:57:27Z

Disabled net461 tests due to the corefx issue: https://github.com/dotnet/corefx/issues/20364.
Manually tested out the desktop pieces on the functional tests and they got to the point where they were exploding for only the corefx issue.
Broke out the skipping of tests into its own commit so we can revert it later.

NTaylorMullen · 2017-05-26T23:58:34Z

...t.AspNetCore.Mvc.Razor.ViewCompilation/Microsoft.AspNetCore.Mvc.Razor.ViewCompilation.csproj

  </PropertyGroup>

  <ItemGroup>
    <None Include="build\**\*" Pack="true" PackagePath="%(Identity)" />
+    <None Include="buildMultiTargeting\*" Pack="true" PackagePath="%(Identity)" />
+    <None Include="$(X86ProjectDirectory)\bin\$(Configuration)\net461\win7-x86\$(MSBuildThisFileName)-x86.exe" Pack="true" PackagePath="lib\net461\$(MSBuildThisFileName)-x86.exe" />


@pranavkm The scaffolding guys handle x86 by having their own tool have a x86 RID. Since it's compatible with x64 all just works. Is there a reason why we did this approach for so long?

Leaving as is for now because it's worked in the past; however, these bits would be significantly simpler without the extra project/x86ness.

I vaguely remember this having to do something with the RID inference during the old project.json days, but not sure.

Having a single executable with x86 makes the size of the nupkg smaller, other than that I am not opinionated about this approach or the one that scaffolding takes.

NTaylorMullen · 2017-05-27T00:00:06Z

Note that this wasn't a "just revert an old commit" I had to do most of this stuff manually and rely on tests for confidence. Lots of changes here that have never existed in this repo.

ajaybhargavb · 2017-05-27T00:16:02Z

NuGetPackageVerifier.json

+                        "lib/netcoreapp2.0/Microsoft.AspNetCore.Mvc.Razor.ViewCompilation.dll": "Not a class library. Docs not required for CLI tools"
+                    },
+                    "BUILD_ITEMS_FRAMEWORK": {
+                        "*": "Doesn't matter"


NTaylorMullen · 2017-05-30T17:13:33Z

@prafullbhosale could you take a look at this guy to ensure everything looks good?

natemcmaster · 2017-05-30T17:15:01Z

build/common.props

@@ -16,4 +16,8 @@
    <PackageReference Include="Internal.AspNetCore.Sdk" Version="$(InternalAspNetCoreSdkVersion)" PrivateAssets="All" />
  </ItemGroup>

+  <ItemGroup Condition="'$(TargetFrameworkIdentifier)'=='.NETFramework'">
+    <PackageReference Include="NETStandard.Library.NETFramework" Version="$(NETStandardLibraryNETFrameworkVersion)" />


Nit: PrivateAssets="All"

natemcmaster · 2017-05-30T17:18:24Z

...Compilation/build/buildMultiTargeting/Microsoft.AspNetCore.Mvc.Razor.ViewCompilation.targets

@@ -0,0 +1,18 @@
+<!--
+Copyright (c) .NET Foundation. All rights reserved.


Remove the copyright header. Per my last conversation about this with @Eilon, we don't put the copyright header into .targets/props files that ship in nuget packages.

natemcmaster · 2017-05-30T17:20:33Z

...vc.Razor.ViewCompilation/build/net461/Microsoft.AspNetCore.Mvc.Razor.ViewCompilation.targets

+  <Target
+      Name="MvcRazorPrecompile"
+      DependsOnTargets="_ResolveInputArguments"
+      Inputs="$(MSBuildThisFileFullPath);@(MvcRazorFilesToCompile);@(IntermediateAssembly);@(DocFileItem);@(_DebugSymbolsIntermediatePath);@(ReferencePath);$(MSBuildAllProjects);"


If you use MSBuildAllProjects as an input, you should also add this file to that list.

<MSBuildAllProjects>$(MSBuildAllProjects);$(MSBuildThisFileFullPath)</MSBuildAllProjects>

Example: https://github.com/aspnet/Configuration/blob/cde0c5ed3f0dd1f5bad36633a4c6d1dd7d48d2a9/src/Microsoft.Extensions.Configuration.UserSecrets/build/GenerateUserSecretsAttribute.targets#L4

Hmm? That is in the list; I think I'm misunderstanding your ask.

Sorry, let me clarify.

The reason for using MSBuildAllProjects is to invalidate target caches when project files change. If there is anything down stream from this target (i.e. publishing), you should add this file to the MSBuildAllProjects list. This is a best practice when writing incremental build targets that chain into other targets you don't control.

Ahh I gets it now 👍

natemcmaster · 2017-05-30T17:23:57Z

...vc.Razor.ViewCompilation/build/net461/Microsoft.AspNetCore.Mvc.Razor.ViewCompilation.targets

+    <CallTarget Targets="_CreateResponseFileForMvcRazorPrecompile" />
+
+    <ItemGroup Condition="'$(PlatformTarget)'=='x86'">
+      <FilesToCopy Include="$(OutputPath)$(AssemblyName).exe.config">


FilesToCopy is a fairly generic itemgroup name. My concern is that other targets may populate items into this. If they do, the Copy and Delete tasks below may fail or operation on the wrong files.

To be safe, give it a more specific name (_PreCompilationFilesToCopy) or clear it before using it <FilesToCopy Remove="@(FilesToCopy)" />.

natemcmaster · 2017-05-30T17:29:03Z

...vc.Razor.ViewCompilation/build/net461/Microsoft.AspNetCore.Mvc.Razor.ViewCompilation.targets

+      SourceFiles="@(FilesToCopy)"
+      DestinationFiles="%(Destination)" />
+
+    <Message


You can remove this log message as it doesn't really contain any useful information.

Gonna leave. It's consistent with the already existing netcoreapp2.0 variant.

natemcmaster · 2017-05-30T17:31:52Z

...icrosoft.AspNetCore.Mvc.Razor.ViewCompilation.FunctionalTests/SimpleAppX86DesktopOnlyTest.cs

+
+        public ApplicationTestFixture Fixture { get; }
+
+        [Fact(Skip = "https://github.com/dotnet/corefx/issues/20364")]


Should be fixed in latest builds of NS.Library.

Not sure why they closed that issue. Still need dotnet/sdk#1259 to be completed before everything works as expected.

natemcmaster · 2017-05-30T17:33:45Z

testapps/SimpleApp/SimpleApp.csproj

@@ -1,6 +1,7 @@
 <Project Sdk="Microsoft.NET.Sdk.Web">
  <PropertyGroup>
-    <TargetFramework>netcoreapp2.0</TargetFramework>
+    <TargetFrameworks>netcoreapp2.0;net461</TargetFrameworks>


It looks like every testapp has been updated to multi-target. Do you have any test apps that are only single TFMS? Would be good to keep some of these test cases around as that is the more common scenario.

Yup, SimpleAppDesktopOnly 👍

What about just .NET Core (for xplat)?

Hmm, is that something we typically test separately? I can see the value in having 1 single TFM test for tools because of how they're launched but I'm not seeing a reason to have a single targeting TFM for both netcoreapp2.0 and net461.

IMO there is some value to test all entry points to view compilation. There are three: buildMultiTargeting/ViewCompilation.targets, build/net461/ViewCompilation.targets and build/netcoreapp2.0/ViewCompilation.targets. It's good to ensure all of these work correctly when they are the "entry point" import for view precompilation, aka the outer-build of a project.

Previous comments are not relevant now that buildMultiTargeting stuff is gone. Should be fine as is.

natemcmaster · 2017-05-30T17:34:32Z

testapps/SimpleAppDesktopOnly/SimpleAppDesktopOnly.csproj

+  <PropertyGroup>
+    <TargetFramework>net461</TargetFramework>
+    <RuntimeIdentifier>win7-x64</RuntimeIdentifier>
+    <PreserveCompilationContext>true</PreserveCompilationContext>


PreserveCompilationContext and OutputType can be removed. These are already the default values for projects that use Microsoft.NET.Sdk.Web

natemcmaster · 2017-05-30T17:35:54Z

...Core.Mvc.Razor.ViewCompilation-x86/Microsoft.AspNetCore.Mvc.Razor.ViewCompilation-x86.csproj

@@ -0,0 +1,20 @@
+<Project Sdk="Microsoft.NET.Sdk">


Instead of having a completely separate project for the x86 version, why not just compile ViewCompilation.csproj multiple times with different configurations? It will reduce the effort required to keep the two csproj files aligned.

Was a bit curious about this one too. Waiting on @pranavkm's feedback on my comment here.

natemcmaster · 2017-05-30T17:37:00Z

...t.AspNetCore.Mvc.Razor.ViewCompilation/Microsoft.AspNetCore.Mvc.Razor.ViewCompilation.csproj

  </ItemGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.AspNetCore.Hosting" Version="$(AspNetCoreVersion)" />
    <PackageReference Include="Microsoft.AspNetCore.Mvc.RazorPages" Version="$(AspNetCoreVersion)" />
    <PackageReference Include="Microsoft.Extensions.CommandLineUtils.Sources" Version="$(AspNetCoreVersion)" PrivateAssets="All" />
+    <ProjectReference Include="$(X86ProjectDirectory)$(MSBuildThisFileName)-x86.csproj" PrivateAssets="true" ReferenceOutputAssembly="false" Condition="'$(TargetFramework)'=='net461'" />


Seems like we should be able to condense this to one project file. MSBuild is designed to handle this need w/o creating a separate csproj.

NTaylorMullen · 2017-05-30T19:38:07Z

🆙 📅

natemcmaster · 2017-05-30T19:55:53Z

...Compilation/build/buildMultiTargeting/Microsoft.AspNetCore.Mvc.Razor.ViewCompilation.targets

@@ -0,0 +1,14 @@
+<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">


The buildMultiTargeting folder should be moved up a level. It won't land in the right place in the nupkg because this is under build.

NTaylorMullen · 2017-05-30T21:54:07Z

🆙 📅

The multi-targeting bits weren't used. Removed them from the system.

prafullbhosale

Over all @natemcmaster's review captures pretty much everything.

prafullbhosale · 2017-05-30T19:57:27Z

...vc.Razor.ViewCompilation/build/net461/Microsoft.AspNetCore.Mvc.Razor.ViewCompilation.targets

+
+    <PropertyGroup Condition="'$(MvcRazorRunCommand)'==''">
+      <MvcRazorRunCommand Condition="'$(PlatformTarget)'=='x86'">$(OutputPath)$(MSBuildThisFileName)-x86.exe</MvcRazorRunCommand>
+      <MvcRazorRunCommand Condition="'$(PlatformTarget)'!='x86'">$(OutputPath)$(MSBuildThisFileName).exe</MvcRazorRunCommand>


This can be simplified by having a default and then overriding if '$(PlatformTarget)'=='x86'

muratg · 2017-05-30T22:58:52Z

👀 Could someone review this please?

natemcmaster · 2017-05-30T23:16:18Z

...r.ViewCompilation/build/netcoreapp2.0/Microsoft.AspNetCore.Mvc.Razor.ViewCompilation.targets

@@ -2,12 +2,13 @@
  <Import Project="$(MSBuildThisFileDirectory)..\common.targets" />
  <PropertyGroup>
    <MvcRazorRunCommand>dotnet</MvcRazorRunCommand>
+    <MSBuildAllProjects>$(MSBuildAllProjects);$(MSBuildThisFileFullPath)</MSBuildAllProjects>


Add to the net461 targets file, too

NTaylorMullen · 2017-05-30T23:24:32Z

🆙 📅

NTaylorMullen · 2017-05-30T23:44:49Z

Getting this in unless someone objects.

#128

NTaylorMullen requested review from pranavkm and natemcmaster May 26, 2017 23:57

dnfclas added the cla-already-signed label May 26, 2017

NTaylorMullen commented May 26, 2017

View reviewed changes

ajaybhargavb reviewed May 27, 2017

View reviewed changes

natemcmaster suggested changes May 30, 2017

View reviewed changes

natemcmaster reviewed May 30, 2017

View reviewed changes

prafullbhosale reviewed May 30, 2017

View reviewed changes

natemcmaster reviewed May 30, 2017

View reviewed changes

NTaylorMullen force-pushed the nimullen/128 branch 2 times, most recently from 96d97d7 to 883114b Compare May 31, 2017 04:17

N. Taylor Mullen added 2 commits May 30, 2017 22:25

Add support for pre-compiling desktop applications.

58fb7a8

#128

Skip desktop testing to work around corefx issue.

b7f1207

#128

NTaylorMullen force-pushed the nimullen/128 branch from 0989301 to b7f1207 Compare May 31, 2017 05:27

NTaylorMullen merged commit 27c6ad4 into dev May 31, 2017

NTaylorMullen deleted the nimullen/128 branch May 31, 2017 05:41

		@@ -0,0 +1,18 @@
		<!--
		Copyright (c) .NET Foundation. All rights reserved.


		public ApplicationTestFixture Fixture { get; }

		[Fact(Skip = "https://github.com/dotnet/corefx/issues/20364")]

		@@ -0,0 +1,14 @@
		<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">

Add support for pre-compiling desktop applications. #130

Add support for pre-compiling desktop applications. #130

Uh oh!

Conversation

NTaylorMullen commented May 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NTaylorMullen commented May 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NTaylorMullen commented May 30, 2017

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!

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!

NTaylorMullen commented May 30, 2017

Uh oh!

natemcmaster May 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NTaylorMullen commented May 30, 2017

Uh oh!

prafullbhosale left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muratg commented May 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NTaylorMullen commented May 30, 2017

Uh oh!

NTaylorMullen commented May 30, 2017

Uh oh!

Uh oh!

natemcmaster May 30, 2017 •

edited

Loading