Convert MVC to CSproj. #5749

NTaylorMullen · 2017-02-02T01:00:32Z

Allow >= RC3 CLI's to build and run MVC.
Added extra sln's so they could be opened in VS. Currently VS' project system can't currently handle Mvc.sln.
Worked around several dotnet migration issues. They are listed in the re-attempted migration section here: [Tracker] MSBuild conversion issues #5482
One large feature bit that couldn't be worked around was the functional tests running on desktop; it represented several known vstest issues. Removed desktop running of functional tests.
Skipped an ActionContextAccessor test due to a vstest appdomain problem.

Note: You cannot use the Test Explorer in VS for this migration pass. Working with VS folks to figure that out. Wont merge until that's resolved.

natemcmaster · 2017-02-02T01:07:56Z

this might take me a while to review. I'll take a look tomorrow.

pranavkm · 2017-02-02T02:24:21Z

makefile.shade

+k-standard-goals
+
+#restore-nuget-packages target='initialize'
+    exec program='${Path.Combine(Directory.GetCurrentDirectory(), ".build", "nuget.exe")}' commandline='restore Mvc.sln'


The equivalent code in Razor was to support building the vsix, so I don't think this is needed here. If you're trying to "select" the sln, you need to use dotnet restore

+1 remove if possible

pranavkm · 2017-02-02T02:24:29Z

samples/MvcSandbox/MvcSandbox.csproj

+
+  <PropertyGroup>
+    <TargetFrameworks>net451;netcoreapp1.1</TargetFrameworks>
+    <RuntimeIdentifiers>win7-x64;win7-x86;osx.10.10-x64;osx.10.11-x64;ubuntu.14.04-x64;ubuntu.16.04-x64;centos.7-x64;rhel.7.2-x64;debian.8-x64;fedora.23-x64;opensuse.13.2-x64</RuntimeIdentifiers>


Can remove.

pranavkm · 2017-02-02T02:24:58Z

samples/MvcSandbox/MvcSandbox.csproj

+  </PropertyGroup>
+
+  <ItemGroup>
+    <Content Update="Views\**\*;web.config;wwwroot">


<Content Update="Views\**\*;web.config;wwwroot" CopyToPublishDirectory="PreserveNewest" />

I think this whole line can be removed. IIUC this is handled by default by Microsoft.NET.Sdk.Web

It knows about the Views folder?

Talked offline, it knows about *.cshtml

pranavkm · 2017-02-02T02:25:29Z

samples/MvcSandbox/MvcSandbox.csproj

+
+  <ItemGroup>
+    <PackageReference Include="Microsoft.AspNetCore.Diagnostics" Version="1.2.0-*" />
+    <PackageReference Include="Microsoft.AspNetCore.Razor" Version="1.2.0-*" />


Remove this + the next one.

Will do, why was it added in the project.json world?

No idea. Maybe a remnant from when we did not have Microsoft.AspNet.Mvc and had individual projects?

pranavkm · 2017-02-02T02:26:34Z

src/Microsoft.AspNetCore.Mvc.Abstractions/Microsoft.AspNetCore.Mvc.Abstractions.csproj

+
+  <ItemGroup>
+    <PackageReference Include="Microsoft.AspNetCore.Routing.Abstractions" Version="1.2.0-*" />
+    <PackageReference Include="Microsoft.Extensions.ClosedGenericMatcher.Sources" Version="1.2.0-*">


<PackageReference Include="Microsoft.Extensions.ClosedGenericMatcher.Sources" Version="1.2.0-*" PrivateAssets="All" /> and similarly below.

pranavkm · 2017-02-02T02:40:38Z

test/WebSites/ApiExplorerWebSite/ApiExplorerWebSite.csproj

+  <PropertyGroup>
+    <TargetFrameworks>net451;netcoreapp1.1</TargetFrameworks>
+    <RuntimeIdentifiers>win7-x64;win7-x86;osx.10.10-x64;osx.10.11-x64;ubuntu.14.04-x64;ubuntu.16.04-x64;centos.7-x64;rhel.7.2-x64;debian.8-x64;fedora.23-x64;opensuse.13.2-x64</RuntimeIdentifiers>
+    <RuntimeIdentifier Condition=" '$(TargetFramework)' == 'net451' ">win7-x86</RuntimeIdentifier>


pranavkm · 2017-02-02T02:41:37Z

test/WebSites/ApiExplorerWebSite/ApiExplorerWebSite.csproj

+  </PropertyGroup>
+
+  <ItemGroup>
+    <Content Update="web.config">


We could remove these. We don't actually do any IIS publishing tests so this isn't super necessary. I'll leave it up to you

We can remove it be Microsoft.NET.Sdk.Web finds the web.config file by default.

pranavkm · 2017-02-02T02:42:25Z

test/WebSites/ApiExplorerWebSite/ApiExplorerWebSite.csproj

+    <PackageReference Include="Microsoft.AspNetCore.StaticFiles" Version="1.2.0-*" />
+  </ItemGroup>
+
+  <ItemGroup Condition=" '$(TargetFramework)' == 'net451' ">


pranavkm · 2017-02-02T02:42:45Z

test/WebSites/BasicWebSite/BasicWebSite.csproj

+    <PackageReference Include="Microsoft.AspNetCore.StaticFiles" Version="1.2.0-*" />
+  </ItemGroup>
+
+  <ItemGroup Condition=" '$(TargetFramework)' == 'net451' ">


pranavkm · 2017-02-02T02:43:43Z

test/WebSites/FilesWebSite/FilesWebSite.csproj

+  </PropertyGroup>
+
+  <ItemGroup>
+    <EmbeddedResource Include="EmbeddedResources\**" Exclude="bin\**;obj\**;**\*.xproj;packages\**;@(EmbeddedResource)" />


Think you can remove the the Exclude node.

natemcmaster · 2017-02-02T18:29:33Z

appveyor.yml

+deploy: off
+# Required for dotnet-test to work
+os: Visual Studio 2015


Visual Studio 2017 RC

natemcmaster · 2017-02-02T18:30:00Z

makefile.shade

+k-standard-goals
+
+#restore-nuget-packages target='initialize'
+    exec program='${Path.Combine(Directory.GetCurrentDirectory(), ".build", "nuget.exe")}' commandline='restore Mvc.sln'


+1 remove if possible

natemcmaster · 2017-02-02T18:30:43Z

samples/MvcSandbox/MvcSandbox.csproj

+  <PropertyGroup>
+    <TargetFrameworks>net451;netcoreapp1.1</TargetFrameworks>
+    <RuntimeIdentifiers>win7-x64;win7-x86;osx.10.10-x64;osx.10.11-x64;ubuntu.14.04-x64;ubuntu.16.04-x64;centos.7-x64;rhel.7.2-x64;debian.8-x64;fedora.23-x64;opensuse.13.2-x64</RuntimeIdentifiers>
+    <RuntimeIdentifier Condition=" '$(TargetFramework)' == 'net451' ">win7-x86</RuntimeIdentifier>


To make this work, you need to invert the logic:

Condition=" '$(TargetFramework)' != 'netcoreapp1.1' "

natemcmaster · 2017-02-02T18:31:24Z

samples/MvcSandbox/MvcSandbox.csproj

+  </PropertyGroup>
+
+  <ItemGroup>
+    <Content Update="Views\**\*;web.config;wwwroot">


I think this whole line can be removed. IIUC this is handled by default by Microsoft.NET.Sdk.Web

natemcmaster · 2017-02-02T18:31:45Z

samples/MvcSandbox/MvcSandbox.csproj

+  </ItemGroup>
+
+  <ItemGroup>
+    <DotNetCliToolReference Include="Microsoft.DotNet.Watcher.Tools" Version="1.0.0-msbuild3-final" />


Use version 1.0.1-* (latest on dev feed)

natemcmaster · 2017-02-02T19:09:12Z

src/Microsoft.AspNetCore.Mvc.Razor/Microsoft.AspNetCore.Mvc.Razor.csproj

+    <VersionPrefix>1.2.0</VersionPrefix>
+    <TargetFrameworks>net451;netstandard1.6</TargetFrameworks>
+    <NoWarn>$(NoWarn);CS1591</NoWarn>
+    <WarningsAsErrors>true</WarningsAsErrors>


FYI: this is now TreatWarningsAsErrors

natemcmaster · 2017-02-02T19:10:20Z

...Microsoft.AspNetCore.Mvc.Abstractions.Test/Microsoft.AspNetCore.Mvc.Abstractions.Test.csproj

+  </ItemGroup>
+
+  <ItemGroup>
+    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="15.0.0-*" />


Nit: sort alphabetically

natemcmaster · 2017-02-02T19:10:41Z

...Microsoft.AspNetCore.Mvc.Abstractions.Test/Microsoft.AspNetCore.Mvc.Abstractions.Test.csproj

+  </ItemGroup>
+
+  <ItemGroup Condition=" '$(TargetFramework)' == 'netcoreapp1.1' ">
+    <PackageReference Include="System.Diagnostics.TraceSource" Version="4.4.0-*" />


This package isn't actually required. cc @pranavkm I'm not sure why we added it to all of our test projects, but I've been removing this.

natemcmaster · 2017-02-02T19:11:21Z

test/Microsoft.AspNetCore.Mvc.Core.Test/Infrastructure/ActionContextAccessorTests.cs

@@ -16,7 +16,7 @@ private static void DomainFunc()
            accessor.ActionContext = new ActionContext();
        }

-        [Fact]
+        [Fact(Skip = "https://github.com/Microsoft/vstest/issues/419")]


Try the AppDomainSetup.ApplicationBase workaround I mentioned in microsoft/vstest#419

Any idea where that guy lives in coreclr world?

The equivalent in .net core is System.AppContext.BaseDirectory

natemcmaster · 2017-02-02T19:12:46Z

test/Microsoft.AspNetCore.Mvc.Core.Test/Microsoft.AspNetCore.Mvc.Core.Test.csproj

+  </ItemGroup>
+
+  <ItemGroup Condition=" '$(TargetFramework)' == 'netcoreapp1.1' ">
+    <PackageReference Include="System.Diagnostics.TraceSource" Version="4.4.0-*" />


Again, probably not required.

Poking around to find out where it is required. Some of our tests do need it.

natemcmaster

A few more things:

natemcmaster · 2017-02-02T19:15:36Z

test/Microsoft.AspNetCore.Mvc.Razor.Host.Test/Microsoft.AspNetCore.Mvc.Razor.Host.Test.csproj

+  </PropertyGroup>
+
+  <ItemGroup>
+    <Compile Remove="TestFiles\**" />


Consider instead setting <DefaultItemExcludes>$(DefaultItemExcludes);TestFiles\**</DefaultItemExcludes>

natemcmaster · 2017-02-02T19:17:45Z

test/Microsoft.AspNetCore.Mvc.TestCommon/Microsoft.AspNetCore.Mvc.TestCommon.csproj

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


Btw, .notest was a simple workaround. When we get repo.targets, we can control the test projects better.

natemcmaster · 2017-02-02T19:18:45Z

test/WebSites/ApplicationModelWebSite/ApplicationModelWebSite.csproj

+    <PackageReference Include="Microsoft.AspNetCore.StaticFiles" Version="1.2.0-*" />
+  </ItemGroup>
+
+  <ItemGroup Condition=" '$(TargetFramework)' == 'net451' ">


the references in this itemgroup are made redundant by the reference to NETStandard.Library.

natemcmaster · 2017-02-02T19:19:09Z

test/WebSites/BasicWebSite/BasicWebSite.csproj

+  </PropertyGroup>
+
+  <ItemGroup>
+    <Content Update="Areas\Area1\Views\**\*;Views\**\*;web.config;wwwroot\**\*">


Can be removed. Microsoft.NET.Sdk.Web should pick these up.

NTaylorMullen · 2017-02-03T01:28:29Z

🆙 📅

dougbu

Have you tried running / debugging the test/WebSites from VS or the command line? I'd guess many of the views now work only when invoked from the functional tests due to the odd <Content> requirements in the new world.
Further, if .cshtml edits aren't picked up on browser refresh, will slow future development down.

dougbu · 2017-02-04T21:36:48Z

samples/MvcSandbox/MvcSandbox.csproj

+  <PropertyGroup>
+    <TargetFrameworks>net451;netcoreapp1.1</TargetFrameworks>
+    <RuntimeIdentifier Condition=" '$(TargetFramework)' != 'netcoreapp1.1' ">win7-x64</RuntimeIdentifier>
+    <PreserveCompilationContext>true</PreserveCompilationContext>


Don't need to set $(PreserveCompilationContext) when using Microsoft.NET.Sdk.Web. Applies a lot in this PR.

dougbu · 2017-02-04T21:54:25Z

test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiExplorerTest.cs

-        [InlineData("POST")]
-        public async Task ApiExplorer_HttpMethod_Single(string httpMethod)
+        [Fact]
+        public async Task ApiExplorer_HttpMethod_Single_PUT()


Get why the GET method needs to be renamed. But, can't this still be a [Theory] for POST and PUT?

@rynowak has convinced me that theories are evil 😄

dougbu · 2017-02-04T21:57:52Z

test/Microsoft.AspNetCore.Mvc.FunctionalTests/testoutput.txt

@@ -0,0 +1,53 @@
+TpTrace Verbose: 0 : 13980, 1, 2017/02/02, 12:42:31.591, 218211137537, vstest.console.dll, TestPluginCache: Discovering the extensions using extension path.


What are these two testOutput files? Why do they need to be checked in?

lol, nope, total fail on my part. will remove.

dougbu · 2017-02-04T22:00:20Z

test/Microsoft.AspNetCore.Mvc.Core.Test/xunit.runner.json

@@ -0,0 +1,3 @@
+{


Please stick a comment somewhere explaining why two test projects can't use shadow copying.

dougbu · 2017-02-04T22:03:42Z

test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultAssemblyPartDiscoveryProviderTest.cs

+                "microsoft.aspnetcore.mvc.webapicompatshim",
+                "microsoft.aspnetcore.mvc.testcommon",
+                "microsoft.aspnetcore.mvc.core.test",
+                "microsoft.aspnetcore.mvc.testdiagnosticlistener.sources",


microsoft.aspnetcore.mvc.testdiagnosticlistener.sources.dll should never exist.

dougbu · 2017-02-04T22:06:30Z

test/Microsoft.AspNetCore.Mvc.Core.Test/Microsoft.AspNetCore.Mvc.Core.Test.csproj

+  </PropertyGroup>
+
+  <ItemGroup>
+    <Compile Include="..\..\shared\Microsoft.AspNetCore.Mvc.TestDiagnosticListener.Sources\**\*.cs" />


Don't need the **\ part of this path. Same for a few other references to these shared sources.

@dougbu why not? Because there aren't any subfolders there right now, or because it's genuinely not required? If it just happens to work right now because there aren't sub-folders, we definitely need to keep it.

🆗 @Eilon. As you thought, my comment was about the current state. I also didn't think we'd be likely to add files at all -- let alone in sub-folders.

@dougbu yeah unfortunately we never know, so better safe than sorry. If there are other places where we aren't doing this, we should fix those to use a recursive glob pattern.

dougbu · 2017-02-06T02:55:55Z

test/Microsoft.AspNetCore.Mvc.FunctionalTests/Microsoft.AspNetCore.Mvc.FunctionalTests.csproj

+  <PropertyGroup>
+    <TargetFrameworks>netcoreapp1.1;</TargetFrameworks>
+    <DefineConstants>$(DefineConstants);__RemoveThisBitTo__GENERATE_BASELINES;FUNCTIONAL_TESTS</DefineConstants>
+    <PackageTargetFallback>$(PackageTargetFallback);portable-net451+win8</PackageTargetFallback>


I may have missed discussion earlier in this PR; sorry if so. Why are we no longer running the functional tests with .NET 4.5.1?

Separately, I assume the $(PackageTargetFallback) setting is much like an "imports" value in project.json file. That did exist in this project. But, do we still need this setting?

@dougbu cuz it ain't supported! .NET Core only.

Plus, in the case of this PR, the tests don't work for external reasons, so we can't run them 😄

Yeah, I asked because I wasn't aware of the specific external issues breaking these tests with desktop .NET.

dougbu · 2017-02-08T01:38:51Z

samples/MvcSandbox/MvcSandbox.csproj

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


Does this site work when started from the command line? I suspect you need a <Content Update...> because the Web SDK only sets $(CopyToPublishDirectory) for content it matches, not $(CopyToOutputDirectory).

It works, no worries 😄

- Allow >= RC3 CLI's to build and run MVC. - Added extra sln's so they could be opened in VS. Currently VS' project system can't currently handle Mvc.sln. - Worked around several dotnet migration issues. They are listed in the re-attempted migration section here: #5482 - One large feature bit that couldn't be worked around was the functional tests running on desktop; it represented several known vstest issues. Removed desktop running of functional tests. - Skipped an ActionContextAccessor test due to a vstest appdomain problem.

natemcmaster · 2017-02-09T22:59:57Z

@NTaylorMullen just updated KoreBuild to build solutions instead of individual csproj. This is the only solution that has multiple *.sln files, so you'll need to add the following to a file named "build/repo.targets"

<Project>
  <ItemGroup>
     <Solutions Include="$(MSBuildThisFileDirectory)..\Mvc.sln" />
  </ItemGroup>
</Project>

natemcmaster · 2017-02-15T23:24:04Z

Any progress on this? It's the last repo on project.json.

NTaylorMullen · 2017-02-15T23:27:52Z

Any progress on this? It's the last repo on project.json.

Still waiting on VS folks to give us a build of VS that doesn't deadlock/hang repeatedly when opening the solution.

dougbu · 2017-02-21T23:05:33Z

I'll push this work forward (well, hopefully forward) while @NTaylorMullen is OOF. Will do this in a new branch temporarily. Once things stabilize, I'll push new commits here to maintain the threads above.

dougbu · 2017-02-23T18:15:16Z

For now at least, please direct all comments to #5852.

natemcmaster · 2017-02-23T20:25:17Z

Can we just close this one?

dougbu · 2017-02-23T20:46:36Z

Can we just close this one?

I'm leaving that up to @NTaylorMullen

NTaylorMullen · 2017-02-27T18:50:07Z

Closing in favor of #5852

dnfclas added the cla-already-signed label Feb 2, 2017

NTaylorMullen requested review from pranavkm, Eilon and natemcmaster February 2, 2017 01:00

NTaylorMullen force-pushed the nimullen/migration branch from d1d5241 to cd64805 Compare February 2, 2017 01:24

pranavkm suggested changes Feb 2, 2017

View reviewed changes

natemcmaster suggested changes Feb 2, 2017

View reviewed changes

natemcmaster reviewed Feb 2, 2017

View reviewed changes

dougbu suggested changes Feb 4, 2017

View reviewed changes

dougbu reviewed Feb 6, 2017

View reviewed changes

NTaylorMullen force-pushed the nimullen/migration branch from afd02a5 to 40f72d4 Compare February 8, 2017 01:37

dougbu reviewed Feb 8, 2017

View reviewed changes

NTaylorMullen added 5 commits February 9, 2017 12:01

Addressed code review comments.

9513585

React to recent project.json changes.

8ad7799

Addressed code review comments.

b060b42

Change xunit.runner.json inclusion scheme.

d3cce19

NTaylorMullen force-pushed the nimullen/migration branch from 86781c4 to d3cce19 Compare February 9, 2017 20:38

pranavkm mentioned this pull request Feb 14, 2017

Fix building the vsix inside VS aspnet/Razor#1001

Closed

dougbu mentioned this pull request Feb 23, 2017

Migrate to MSBuild #5852

Merged

NTaylorMullen closed this Feb 27, 2017

dougbu deleted the nimullen/migration branch July 15, 2018 03:46

		@@ -0,0 +1,53 @@
		TpTrace Verbose: 0 : 13980, 1, 2017/02/02, 12:42:31.591, 218211137537, vstest.console.dll, TestPluginCache: Discovering the extensions using extension path.

Convert MVC to CSproj. #5749

Convert MVC to CSproj. #5749

Conversation

NTaylorMullen commented Feb 2, 2017

natemcmaster commented Feb 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natemcmaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NTaylorMullen commented Feb 3, 2017

dougbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natemcmaster commented Feb 9, 2017 • edited Loading

natemcmaster commented Feb 15, 2017

NTaylorMullen commented Feb 15, 2017

dougbu commented Feb 21, 2017

dougbu commented Feb 23, 2017

natemcmaster commented Feb 23, 2017

dougbu commented Feb 23, 2017

NTaylorMullen commented Feb 27, 2017

natemcmaster commented Feb 9, 2017 •

edited

Loading