Upgrade to RC.3 #1314

cesarblum · 2017-01-20T20:11:21Z

TODO:

Figure out why test run is failing on Mac - turns out it's still https://github.com/dotnet/coreclr/issues/8903
Fix benchmarks project

natemcmaster

A good start. A few things suggestions for how to simply and cleanup

natemcmaster · 2017-01-20T20:27:06Z

samples/LargeResponseApp/LargeResponseApp.csproj

@@ -0,0 +1,12 @@
+<Project Sdk="Microsoft.NET.Sdk.Web" ToolsVersion="15.0">
+  <Import Project="..\..\build\common.props" />


You can remove this reference from samples if the sample works fine without.

They don't. Do you recommend keeping it here or bringing in the relevant stuff from common.props?

Copy in the stuff it needs. In theory, we want users to be able to copy the sample and run without needing Internal.AspNetCore.Sdk.

natemcmaster · 2017-01-20T20:27:45Z

src/Microsoft.AspNetCore.Server.Kestrel.Https/Microsoft.AspNetCore.Server.Kestrel.Https.csproj

+
+  <ItemGroup>
+    <PackageReference Include="NETStandard.Library" Version="1.6.2-*" />
+    <PackageReference Include="Microsoft.Extensions.TaskCache.Sources" Version="1.2.0-*">


Nit: can use one-line syntax <PackageReference .... PrivateAssets="All" />

natemcmaster · 2017-01-20T20:27:59Z

src/Microsoft.AspNetCore.Server.Kestrel.Https/Microsoft.AspNetCore.Server.Kestrel.Https.csproj

+
+  <ItemGroup>
+    <ProjectReference Include="..\Microsoft.AspNetCore.Server.Kestrel\Microsoft.AspNetCore.Server.Kestrel.csproj" />
+  </ItemGroup>


Nit: condense with the ItemGroup below.

natemcmaster · 2017-01-20T20:28:22Z

src/Microsoft.AspNetCore.Server.Kestrel.Https/Properties/AssemblyInfo.cs

-[assembly: AssemblyCompany("Microsoft Corporation.")]
-[assembly: AssemblyCopyright("© Microsoft Corporation. All rights reserved.")]
-[assembly: AssemblyProduct("Microsoft ASP.NET Core")]
+[assembly: AssemblyMetadata("Serviceable", "True")]


This will be generated automatically by Internal.AspNetCore.Sdk. This whole file can be removed.

natemcmaster · 2017-01-20T20:28:46Z

src/Microsoft.AspNetCore.Server.Kestrel/Properties/AssemblyInfo.cs

@@ -9,7 +9,3 @@
 [assembly: InternalsVisibleTo("Microsoft.AspNetCore.Server.KestrelTests, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")]
 [assembly: InternalsVisibleTo("Microsoft.AspNetCore.Server.Kestrel.Performance, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")]
 [assembly: AssemblyMetadata("Serviceable", "True")]


Can remove this too.

We need the InternalsVisibleTo attributes otherwise tests don't compile.

I just meant you can remove that one line, not the whole file :)

natemcmaster · 2017-01-20T20:30:02Z

...re.Server.Kestrel.FunctionalTests/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests.csproj

+  </ItemGroup>
+
+  <ItemGroup Condition=" '$(TargetFramework)' == 'net451' ">
+    <Reference Include="System.Net.Http" />


Hmmm, this looks like the issue @anurse is workign on. What should we do here?

Nvm, just realized this is a test project.

natemcmaster · 2017-01-20T20:30:53Z

...re.Server.Kestrel.FunctionalTests/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests.csproj

+  </ItemGroup>
+
+  <PropertyGroup Condition=" '$(OS)' == 'Unix' ">
+    <DefineConstants>$(DefineConstants);UNIX</DefineConstants>


Would this be useful outside this repo? We could add these kinds of helpers to Internal.AspNetCore.Sdk

I used this to conditionally add [assembly: CollectionBehavior(DisableTestParallelization = true)] to our functional tests on Mac, because we hit the system's file descriptor limit when running in parallel. This isn't even the best fix since it would apply to any Unix and not just Darwin, but it's the cleanest way to do it for now.

natemcmaster · 2017-01-20T20:31:17Z

...AspNetCore.Server.Kestrel.Performance/Microsoft.AspNetCore.Server.Kestrel.Performance.csproj

+
+  <ItemGroup>
+    <ProjectReference Include="..\..\src\Microsoft.AspNetCore.Server.Kestrel\Microsoft.AspNetCore.Server.Kestrel.csproj" />
+  </ItemGroup>


Nit: condense itemgroups.

natemcmaster · 2017-01-20T20:34:20Z

makefile.shade

Could this just be dotnet run instead?

natemcmaster · 2017-01-20T20:37:19Z

tools/Microsoft.AspNetCore.Server.Kestrel.GeneratedCode/run.proj

@@ -0,0 +1,10 @@
+<Project ToolsVersion="15.0">


Seems like this file adds a layer we don't need. makefile.shade can just call dotnet run on the project directly with the arguments, right?

Also, to make 'dotnet run' just do the right thing by default, you can add this M.A.S.GenerateCode.csproj

<PropertyGroup> <StartArguments> "$(MSBuildProjectDirectory)/../../src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameHeaders.Generated.cs" "$(MSBuildProjectDirectory)/../../src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.Generated.cs" </StartArguments> </PropertyGroup>

cesarblum · 2017-01-20T22:43:59Z

Updated.

natemcmaster · 2017-01-20T22:57:51Z

Hopefully MSBuild will help us determine macOS vs Linux in the future. In the meantime, we can look into adding this ourselves so that we still get parallel tests running on Linux. dotnet/msbuild#539

natemcmaster · 2017-01-20T23:37:37Z

build/common.props

+  <Import Project="..\version.props" />
+
+  <PropertyGroup>
+    <Product>Microsoft .NET Extensions</Product>


Change to "Microsoft ASP.NET Core"

natemcmaster · 2017-01-20T23:39:48Z

test/Microsoft.AspNetCore.Server.KestrelTests/Microsoft.AspNetCore.Server.KestrelTests.csproj

+  </ItemGroup>
+
+  <ItemGroup>
+    <Service Include="{82a7f48d-3b50-4b1e-b82e-3ada8210c358}" />


(sigh) VS, why u need guids still?

Ugh. I had remove some of these from other projects but missed this one.

davidfowl · 2017-01-21T01:33:10Z

src/Microsoft.AspNetCore.Server.Kestrel/Microsoft.AspNetCore.Server.Kestrel.csproj

+    <GenerateDocumentationFile>true</GenerateDocumentationFile>
+    <PackageTags>aspnetcore;kestrel</PackageTags>
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
+    <NoWarn>CS1591;$(NoWarn)</NoWarn>


not needed?

We get a ton of those.

Ahhh XML docs...

davidfowl · 2017-01-21T01:45:52Z

src/Microsoft.AspNetCore.Server.Kestrel/Microsoft.AspNetCore.Server.Kestrel.csproj

+  </ItemGroup>
+
+  <ItemGroup Condition=" '$(TargetFramework)' == 'net451' ">
+    <Reference Include="System.Runtime" />


cesarblum · 2017-01-23T18:45:20Z

Problem with benchmarks is that BenchmarkDotNet generates project.json files and we can't restore those with references to the now .csproj-based Kestrel projects.

Will try and get it to restore from the packages in artifacts/.

davidfowl · 2017-01-27T00:44:19Z

samples/SampleApp/SampleApp.csproj

+  </PropertyGroup>
+
+  <ItemGroup>
+    <Content Include="testCert.pfx">


I commented on the commit but I'll comment here too 😄. Change these to attributes. Also CopyToOutputDirectory infers CopyToPublishDirectory.

<Content Include="testCert.pfx" CopyToOutputDirectory="PreserveNewest" />

cesarblum · 2017-01-28T01:43:24Z

@Eilon Should be ready to merge.

cesarblum · 2017-01-28T01:46:32Z

test/Microsoft.AspNetCore.Server.Kestrel.Performance/configs/MsBuildExecutor.cs

+
+namespace Microsoft.AspNetCore.Server.Kestrel.Performance
+{
+    public class MsBuildExecutor : IExecutor


@adamsitnik Would be glad to collaborate with you to get this more generalized and into BenchmarkDotNet 😄

@CesarBS Sure! I have got something up and running: commit but not final yet, see the description

This week I am in Redmond for the MS LEAP, so we can even meet in person and talk about it ;) btw were you able to run this auto-generated dll? When I tried similar approach (to execute with process.Start) I got some missing dlls exceptions (the directory no longer contains the executable). That's why I have switched to dotnet run executor (but it has some limitaitons)

Awesome! I'll send you an email so we can arrange when and where to meet.

I wasn't able to run the generated binaries with what came out of the box as of your latest commit. I would get this error when trying to run the benchmarks, until it failed:

/Users/Cesar/.dotnet/sdk/1.0.0-rc3-004522/Microsoft.Common.CurrentVersion.targets(3935,5): warning MSB3026: Could not copy "obj/Release/netcoreapp1.1/Microsoft.AspNetCore.Server.Kestrel.Performance.dll" to "bin/Release/netcoreapp1.1/Microsoft.AspNetCore.Server.Kestrel.Performance.dll". Beginning retry 1 in 1000ms. The process cannot access the file '/Users/Cesar/src/aspnet/KestrelHttpServer/test/Microsoft.AspNetCore.Server.Kestrel.Performance/bin/Release/netcoreapp1.1/Microsoft.AspNetCore.Server.Kestrel.Performance.dll' because it is being used by another process. [/Users/Cesar/src/aspnet/KestrelHttpServer/test/Microsoft.AspNetCore.Server.Kestrel.Performance/Microsoft.AspNetCore.Server.Kestrel.Performance.csproj]

I also noticed that there's a mismatch between DotNetCliBuilder and DotNetRunExecutor: the builder puts binaries in <generated project folder>\binaries, while the executor calls dotnet run, which will re-compile the project and attempt to run them from bin\Release\netcoreapp1.1.

Eilon · 2017-01-30T00:12:07Z

...AspNetCore.Server.Kestrel.Performance/Microsoft.AspNetCore.Server.Kestrel.Performance.csproj

@@ -18,7 +18,7 @@

  <ItemGroup>
    <ProjectReference Include="..\..\src\Microsoft.AspNetCore.Server.Kestrel\Microsoft.AspNetCore.Server.Kestrel.csproj" />
-    <PackageReference Include="BenchmarkDotNet" Version="0.10.1" />
+    <PackageReference Include="BenchmarkDotNet" Version="0.10.2.*" />


Eep, 4-segment package versions 😦 But it is what it is...

davidfowl · 2017-01-30T16:53:54Z

...AspNetCore.Server.Kestrel.Performance/Microsoft.AspNetCore.Server.Kestrel.Performance.csproj

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


This should be a regular console app:

Microsoft.NET.Sdk not Microsoft.NET.Sdk.Web

davidfowl · 2017-01-30T16:54:26Z

...etCore.Server.Kestrel.GeneratedCode/Microsoft.AspNetCore.Server.Kestrel.GeneratedCode.csproj

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


Same here, this should be Microsoft.NET.Sdk

cesarblum · 2017-01-30T20:00:37Z

Latest feedback addressed + fixed test execution on VS (a626218).

natemcmaster

looks good

natemcmaster · 2017-01-30T20:12:27Z

KestrelHttpServer.sln

-# Visual Studio 14
-VisualStudioVersion = 14.0.25420.1
+# Visual Studio 15
+VisualStudioVersion = 15.0.26120.4


Nit: 15.0.26117.0 is the latest RC.

dnfclas added the cla-already-signed label Jan 20, 2017

cesarblum force-pushed the cesarbs/msbuild branch from 860f15a to 5c5f401 Compare January 20, 2017 20:16

natemcmaster suggested changes Jan 20, 2017

View reviewed changes

natemcmaster mentioned this pull request Jan 20, 2017

Upgrade to VS 2017 aspnet/Universe#469

Closed

natemcmaster reviewed Jan 20, 2017

View reviewed changes

natemcmaster approved these changes Jan 20, 2017

View reviewed changes

davidfowl reviewed Jan 21, 2017

View reviewed changes

cesarblum force-pushed the cesarbs/msbuild branch from 9de4bce to 873811a Compare January 23, 2017 17:33

cesarblum mentioned this pull request Jan 23, 2017

[NoMerge] For davidfowl #1320

Closed

cesarblum force-pushed the cesarbs/msbuild branch from 873811a to b2d99b7 Compare January 26, 2017 20:47

davidfowl reviewed Jan 27, 2017

View reviewed changes

cesarblum force-pushed the cesarbs/msbuild branch from af66fe1 to 2dce18a Compare January 27, 2017 18:44

cesarblum commented Jan 28, 2017

View reviewed changes

cesarblum changed the title ~~[WIP] Upgrade to RC.3~~ Upgrade to RC.3 Jan 28, 2017

Eilon reviewed Jan 30, 2017

View reviewed changes

davidfowl reviewed Jan 30, 2017

View reviewed changes

cesarblum force-pushed the cesarbs/msbuild branch from 6c8a719 to c4570f8 Compare January 30, 2017 18:21

natemcmaster approved these changes Jan 30, 2017

View reviewed changes

cesarblum force-pushed the cesarbs/msbuild branch from e678200 to d33b682 Compare January 31, 2017 23:08

Upgrade to RC.3

1a2c438

cesarblum force-pushed the cesarbs/msbuild branch from d33b682 to 1a2c438 Compare February 1, 2017 00:23

cesarblum merged commit 1a2c438 into dev Feb 1, 2017

cesarblum deleted the cesarbs/msbuild branch February 1, 2017 00:37

		@@ -0,0 +1,12 @@
		<Project Sdk="Microsoft.NET.Sdk.Web" ToolsVersion="15.0">
		<Import Project="..\..\build\common.props" />

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

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

Upgrade to RC.3 #1314

Upgrade to RC.3 #1314

Conversation

cesarblum commented Jan 20, 2017 • edited Loading

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

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

cesarblum commented Jan 20, 2017

natemcmaster commented Jan 20, 2017 via email

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

cesarblum commented Jan 23, 2017

Choose a reason for hiding this comment

cesarblum commented Jan 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cesarblum Jan 29, 2017 • edited Loading

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

cesarblum commented Jan 30, 2017

natemcmaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cesarblum commented Jan 20, 2017 •

edited

Loading

cesarblum Jan 29, 2017 •

edited

Loading