Migrate to MSBuild #5852

dougbu · 2017-02-23T18:13:12Z

Rebase nimullen/migration branch from PR #5749 and carry on from there. See descriptions of individual commits for more detail.

Current status: Command-line builds work cleanly. Mvc.NoFun.sln and Mvc.OnlyFun.sln generally load, build, and test cleanly in Visual Studio. Still experiencing some "Non responsive" periods and slow loads, especially if solution is loaded at same time as VS is started. Mvc.sln still tends to hang VS.

Will file a few more bugs (mostly around C# editing performance and testing experience) and add them to #5482. Have already checked off fixed bugs in #5482.

dougbu · 2017-02-23T18:14:28Z

@natemcmaster @pranavkm @Eilon please have a look at this instead of #5749. But note pushing remains blocked on VS issues.

dougbu · 2017-02-23T18:18:12Z

src/Microsoft.AspNetCore.Mvc.Core/Microsoft.AspNetCore.Mvc.Core.csproj

+    <PackageReference Include="Microsoft.AspNetCore.Routing" Version="1.2.0-*" />
+    <PackageReference Include="Microsoft.AspNetCore.Routing.DecisionTree.Sources" Version="1.2.0-*" PrivateAssets="All" />
+    <PackageReference Include="Microsoft.Extensions.ClosedGenericMatcher.Sources" Version="1.2.0-*" PrivateAssets="All" />
+    <PackageReference Include="Microsoft.Extensions.DependencyModel" Version="1.1.0-*" />


Is this the correct version of this package?

There is a 2.0.0 version being built...https://github.com/dotnet/core-setup/blob/master/src/Microsoft.Extensions.DependencyModel/. Let's add that to the list of things we upgrade when we move everything to netstandard2.0 / .NET Core 2.0. cref aspnet/Coherence#166

I think we'd leave this pinned at 1.1.0 since that's the version that corresponds to NETStandard.Library 1.6.1

@pranavkm do you mean this should be 1.1.0 and not 1.1.0-*?

Yup - the 1.1.0 package exists on nuget.org, so that's the build that'll get used: https://www.nuget.org/packages/Microsoft.Extensions.DependencyModel/1.1.0

dougbu · 2017-02-23T18:20:11Z

samples/MvcSandbox/MvcSandbox.csproj

+  <PropertyGroup>
+    <TargetFrameworks>net452;netcoreapp1.1</TargetFrameworks>
+    <TargetFrameworks Condition="'$(OS)' != 'Windows_NT'">netcoreapp1.1</TargetFrameworks>
+    <RuntimeIdentifier Condition=" '$(TargetFramework)' != 'netcoreapp1.1' ">win7-x64</RuntimeIdentifier>


I hesitate to remove this because confirming everything works in command-line and VS runs as well as our functional tests might miss something. Have at least confirmed the Microsoft.NET.Sdk.Web targets do not contain anything similar.

Anyone know if these lines in the web site project files can be removed?

dougbu · 2017-02-23T18:21:46Z

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

+  <PropertyGroup>
+    <TargetFrameworks>netcoreapp1.1;net452</TargetFrameworks>
+    <TargetFrameworks Condition="'$(OS)' != 'Windows_NT'">netcoreapp1.1</TargetFrameworks>
+    <PreserveCompilationContext>true</PreserveCompilationContext>


Was present in may test project.json files. But, was actually needed only in 2 test projects.

dougbu · 2017-02-23T18:24:29Z

.../Microsoft.AspNetCore.Mvc.Razor.Test/Internal/ViewComponentTagHelperDescriptorFactoryTest.cs

+            if (type == null)
+            {
+                type = typeof(ViewComponentTagHelperDescriptorFactoryTest).GetMethod("MethodWithOutParam").GetParameters().First().ParameterType;
+            }


@ajaybhargavb did you file a bug about the serialization / deserialization problems with the out List<char*[]> parameter's Type? Would like to reference it here.

BTW I suspect the issue is actually in serialization -- ends up as an open generic List<> type.

Not yet. I was waiting for your PR to repro this issue. I'll link it here once I am done.

Is this bug introduced by converting to C# 7.0? We had a few other, minor problems when switching to the new compiler.

Could we split this out in to a separate test? GetCSharpTypeName_ReturnsCorrectTypeNames_ForOutParameters? This is kinda ugly

natemcmaster · 2017-02-23T19:57:28Z

src/Microsoft.AspNetCore.Mvc.Core/Microsoft.AspNetCore.Mvc.Core.csproj

+    <PackageReference Include="Microsoft.Extensions.PropertyHelper.Sources" Version="1.2.0-*" PrivateAssets="All" />
+    <PackageReference Include="Microsoft.Extensions.SecurityHelper.Sources" Version="1.2.0-*" PrivateAssets="All" />
+    <PackageReference Include="System.Buffers" Version="$(CoreFxVersion)-*" />
+    <PackageReference Include="System.Diagnostics.DiagnosticSource" Version="$(CoreFxVersion)-*" />


Should be "Version="$(CoreFxVersion)". Leave the -* off. This is true in many places in this and other files.

natemcmaster · 2017-02-23T20:00:38Z

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

+    <ProjectReference Include="..\..\src\Microsoft.AspNetCore.Mvc\Microsoft.AspNetCore.Mvc.csproj" />
+    <ProjectReference Include="..\Microsoft.AspNetCore.Mvc.TestCommon\Microsoft.AspNetCore.Mvc.TestCommon.csproj" />
+
+    <PackageReference Include="Microsoft.DotNet.InternalAbstractions" Version="1.0.0" />


Can you try removing this? IIRC this was added because early builds of xunit needed it. This has since been fixed.

Unless, of course, this project uses API from that package. In which case, switch this to Microsoft.DotNet.PlatformAbstractions/1.1.0 instead.

I think Taylor tried this earlier, but we might have had newer packages since.

natemcmaster · 2017-02-23T20:07:26Z

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

+    <ProjectReference Include="..\Microsoft.AspNetCore.Mvc.TestCommon\Microsoft.AspNetCore.Mvc.TestCommon.csproj" />
+
+    <PackageReference Include="Microsoft.AspNetCore.Http" Version="1.2.0-*" />
+    <PackageReference Include="Microsoft.DotNet.InternalAbstractions" Version="1.0.0" />


Ditto...repeat for all test projects.

natemcmaster · 2017-02-23T20:10:22Z

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>


What are we using that still requires this?

See @pranavkm's comment about the WebApiCompatShim project. #5822 should cover removing these last couple of fallbacks.

natemcmaster · 2017-02-23T20:10:40Z

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

+  <Import Project="..\..\build\common.props" />
+
+  <PropertyGroup>
+    <TargetFrameworks>netcoreapp1.1;</TargetFrameworks>


Nit: use the singular form. <TargetFramework>netcoreapp1.1</TargetFramework>

We should probably add a tracking bug to re-add net4x testing. I forget why we decided to skip desktop here.

natemcmaster · 2017-02-23T20:18:51Z

test/WebSites/FilesWebSite/FilesWebSite.csproj

+
+  <ItemGroup>
+    <EmbeddedResource Include="EmbeddedResources\**" Exclude="bin\**;obj\**;**\*.xproj;packages\**;@(EmbeddedResource)" />
+    <Content Update="sample.txt;web.config">


For sure don't need web.config here, might not need sample.txt. The SDK will pick up web.config by default.

natemcmaster · 2017-02-23T20:19:34Z

test/WebSites/FilesWebSite/FilesWebSite.csproj

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


You can remove the Exclude attribute here as the "Include" wouldn't match up with anything in the Exclude patterns.

natemcmaster · 2017-02-23T20:21:06Z

test/WebSites/RazorWebSite/RazorWebSite.csproj

+    <PackageReference Include="Microsoft.Extensions.FileProviders.Embedded" Version="1.2.0-*" />
+  </ItemGroup>
+
+  <PropertyGroup Condition=" '$(TargetFramework)' == 'net452' ">


Nit: keep all property groups together at the top of the top file.

Also, why are these necessary? The SDK already defines contants per-TFM.

We're trying to verify that conditional constants defined in the project are available to views.

natemcmaster · 2017-02-23T20:21:46Z

test/WebSites/UserClassLibrary/UserClassLibrary.csproj

+  <PropertyGroup>
+    <TargetFrameworks>net452;netstandard1.3</TargetFrameworks>
+    <TargetFrameworks Condition="'$(OS)' != 'Windows_NT'">netstandard1.3</TargetFrameworks>
+    <AssemblyName>UserClassLibrary</AssemblyName>


Nit: both AssemblyName and PackageId aren't required here. These are the default values.

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

build/repo.props

+  <ItemGroup>
+    <ExcludeFromTest Include="$(RepositoryRoot)test\Microsoft.AspNetCore.Mvc.TestCommon\*.csproj" />
+    <!-- Exclude with and without extra slash in case DefaultItems.targets is fixed. -->
+    <ExcludeSolutions Include="$(RepositoryRoot)\Mvc.*Fun.sln" />


Oops. Open an issue on KoreBuild.

aspnet/KoreBuild#182

pranavkm

LGTM

pranavkm · 2017-02-23T20:49:46Z

src/Microsoft.AspNetCore.Mvc.Core/Microsoft.AspNetCore.Mvc.Core.csproj

+    <PackageReference Include="Microsoft.AspNetCore.Routing" Version="1.2.0-*" />
+    <PackageReference Include="Microsoft.AspNetCore.Routing.DecisionTree.Sources" Version="1.2.0-*" PrivateAssets="All" />
+    <PackageReference Include="Microsoft.Extensions.ClosedGenericMatcher.Sources" Version="1.2.0-*" PrivateAssets="All" />
+    <PackageReference Include="Microsoft.Extensions.DependencyModel" Version="1.1.0-*" />


I think we'd leave this pinned at 1.1.0 since that's the version that corresponds to NETStandard.Library 1.6.1

pranavkm · 2017-02-23T21:56:56Z

test/WebSites/VersioningWebSite/VersioningWebSite.csproj

+
+  <PropertyGroup>
+    <TargetFrameworks>net452;netcoreapp1.1</TargetFrameworks>
+    <TargetFrameworks Condition="'$(OS)' != 'Windows_NT'">netcoreapp1.1</TargetFrameworks>


Probably don't need this in test apps. The issue is running tests on xplat, but compiling it should be fine.

🙈 not much point in compiling these and it adds at least 10s to a Linux build in my testing. Probably more since I only compared build-compile runs.

pranavkm · 2017-02-23T21:58:00Z

test/WebSites/TagHelpersWebSite/TagHelpersWebSite.csproj

+    <TargetFrameworks>net452;netcoreapp1.1</TargetFrameworks>
+    <TargetFrameworks Condition="'$(OS)' != 'Windows_NT'">netcoreapp1.1</TargetFrameworks>
+    <RuntimeIdentifier Condition=" '$(TargetFramework)' != 'netcoreapp1.1' ">win7-x64</RuntimeIdentifier>
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>


pranavkm · 2017-02-23T21:59:54Z

test/WebSites/RazorWebSite/RazorWebSite.csproj

+    <PackageReference Include="Microsoft.Extensions.FileProviders.Embedded" Version="1.2.0-*" />
+  </ItemGroup>
+
+  <PropertyGroup Condition=" '$(TargetFramework)' == 'net452' ">


We're trying to verify that conditional constants defined in the project are available to views.

pranavkm · 2017-02-23T22:03:01Z

test/WebSites/ControllersFromServicesClassLibrary/ControllersFromServicesClassLibrary.csproj

+  <Import Project="..\..\..\build\common.props" />
+
+  <PropertyGroup>
+    <TargetFrameworks>net452;netcoreapp1.1</TargetFrameworks>


Looks like this missing the preserveCompilationContext flag from the project.json. I think project.json required an executable to produce a deps file.

pranavkm · 2017-02-23T22:05:07Z

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

+  </ItemGroup>
+
+  <ItemGroup Condition=" '$(TargetFramework)' == 'netcoreapp1.1' ">
+    <PackageReference Include="System.Xml.XmlDocument" Version="$(CoreFxVersion)-*" />


Is this still needed? The Formatter project should bring in this reference

pranavkm · 2017-02-23T22:09:28Z

.../Microsoft.AspNetCore.Mvc.Razor.Test/Internal/ViewComponentTagHelperDescriptorFactoryTest.cs

+            if (type == null)
+            {
+                type = typeof(ViewComponentTagHelperDescriptorFactoryTest).GetMethod("MethodWithOutParam").GetParameters().First().ParameterType;
+            }


Could we split this out in to a separate test? GetCSharpTypeName_ReturnsCorrectTypeNames_ForOutParameters? This is kinda ugly

pranavkm · 2017-02-23T22:11:16Z

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

+  <PropertyGroup>
+    <TargetFrameworks>netcoreapp1.1;net452</TargetFrameworks>
+    <TargetFrameworks Condition="'$(OS)' != 'Windows_NT'">netcoreapp1.1</TargetFrameworks>
+    <DefineConstants>$(DefineConstants);__RemoveThisBitTo__GENERATE_BASELINES</DefineConstants>


<DefineConstants Condition="'$(GenerateBaselines)'=='true'">$(DefineConstants);GENERATE_BASELINES</DefineConstants> <DefineConstants>$(DefineConstants);__RemoveThisBitTo__GENERATE_BASELINES</DefineConstants>

this lets you do dotnet test /p:GenerateBaseLines:true to produce baselines without editing any files. The second one exists as an option inside VS.

pranavkm · 2017-02-23T22:14:11Z

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

+  <Import Project="..\..\build\common.props" />
+
+  <PropertyGroup>
+    <TargetFrameworks>netcoreapp1.1;</TargetFrameworks>


We should probably add a tracking bug to re-add net4x testing. I forget why we decided to skip desktop here.

pranavkm · 2017-02-23T22:18:52Z

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

+    <ProjectReference Include="..\..\src\Microsoft.AspNetCore.Mvc\Microsoft.AspNetCore.Mvc.csproj" />
+    <ProjectReference Include="..\Microsoft.AspNetCore.Mvc.TestCommon\Microsoft.AspNetCore.Mvc.TestCommon.csproj" />
+
+    <PackageReference Include="Microsoft.DotNet.InternalAbstractions" Version="1.0.0" />


I think Taylor tried this earlier, but we might have had newer packages since.

dougbu · 2017-02-26T23:08:33Z

🆙📅

Not sure why the AppVeyor build failed. Many functional tests hit issues in .NET 4.5.2 runs. Every failure I've seen was a MissingMethodException hit in Microsoft.CodeAnalysis.Razor.TagHelperTypeVisitor.Create() calls. Haven't reproduced this locally but am trying things out with all the latest packages…

dougbu · 2017-02-26T23:40:16Z

… bizarre but the AppVeyor failures do not reproduce locally, even after .\.build\nuget.exe locals all -clear and git clean -dfx. And, the dev branch seems to build fine as well. Redoing the AppVeyor build of this PR…

dougbu · 2017-02-27T00:07:06Z

… latest AppVeyor build of this PR failed in the same way. Will try to mimic AppVeyor environment locally…

natemcmaster · 2017-02-27T18:28:55Z

Talked with Doug in person. It appears the tests no longer pass when .NET Framework 4.6.2 is installed, which is causing the AppVeyor failures. We should look into this as it might be a bug in BCL or Roslyn.

dougbu · 2017-02-27T18:49:15Z

🆙📅 FYI only. I squashed my changes since nimullen/migration.

dougbu · 2017-02-27T19:08:56Z

It appears the tests no longer pass when .NET Framework 4.6.2 is installed, which is causing the AppVeyor failures.

I was wrong; .NET 4.6.2 is installed on my machine. Must be some other difference between my machine and AppVeyor and @natemcmaster's machine.

@NTaylorMullen

- thanx to @NTaylorMullen for initial conversion - e.g. AssemblyInfo.cs files were already minimized or removed :) - allow `>=` RC3 CLI's to build and run MVC - work around several dotnet migration issues; see #5482 - disable full .NET Framework runs of functional tests; see #5873 - remove `Microsoft.DotNet.InternalAbstractions` and `System.Xml.XmlDocument` dependencies - remove project.json (!!), *.xproj, .notest, and web.config files Redo earlier changes: - apply test migration to .NET 4.5.2 in *.csproj world - see 63507c8 for previous, project.json work - apply dependency version downgrade from 0097e40 in *.csproj world Make other test-related changes: - make Microsoft.AspNetCore.Mvc.TestDiagnosticListener a regular class library - add support for `/p:GenerateBaselines=true` for functional and Razor.Host tests - separate `GetCSharpTypeName_ReturnsCorrectTypeNames_ForOutParameter()` test - work around inability to deserialize a odd `ref` type - xUnit and vstest now serialize / deserialze test data more often - skip poor test mentioned in #5768 - work around microsoft/vstest#392 - rename tests to avoid duplicates - work around microsoft/vstest#419 - set up created `AppDomain`s with current `ApplicationBase`

dougbu · 2017-03-01T06:19:27Z

acfad83

natemcmaster · 2017-03-01T17:00:45Z

🎉

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

dougbu assigned pranavkm Feb 23, 2017

dougbu mentioned this pull request Feb 23, 2017

Convert MVC to CSproj. #5749

Closed

dougbu commented Feb 23, 2017

View reviewed changes

dougbu mentioned this pull request Feb 23, 2017

Update to Microsoft.NETCore.App 2.0 \ NETStandard.Library 2.0.0 aspnet/Coherence#166

Closed

12 tasks

natemcmaster suggested changes Feb 23, 2017

View reviewed changes

pranavkm approved these changes Feb 23, 2017

View reviewed changes

dougbu force-pushed the dougbu/migration branch 3 times, most recently from 91285cb to d350769 Compare February 26, 2017 22:00

ctaggart mentioned this pull request Feb 27, 2017

[WIP] add source link support to portable pdbs #5860

Closed

natemcmaster approved these changes Feb 27, 2017

View reviewed changes

dougbu force-pushed the dougbu/migration branch from d350769 to d7d35f1 Compare February 27, 2017 18:48

dougbu force-pushed the dougbu/migration branch from a136c86 to acfad83 Compare March 1, 2017 05:50

dougbu merged commit acfad83 into dev Mar 1, 2017

dougbu deleted the dougbu/migration branch March 29, 2017 21:55

Migrate to MSBuild #5852

Migrate to MSBuild #5852

Conversation

dougbu commented Feb 23, 2017

dougbu commented Feb 23, 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

pranavkm 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

dougbu commented Feb 26, 2017

dougbu commented Feb 26, 2017

dougbu commented Feb 27, 2017

natemcmaster commented Feb 27, 2017

dougbu commented Feb 27, 2017

dougbu commented Feb 27, 2017

dougbu commented Mar 1, 2017

natemcmaster commented Mar 1, 2017