Migrate to csproj #703

SteveSandersonMS · 2017-02-22T18:24:26Z

This PR moves all the projects to .csproj and rebuilds the .sln to work with VS017RC. I'll squash all these commits before merging it into dev, but it's probably easier to review split up into all these commits.

There's still one more thing to be done - change the templates projects so they use RC4 tooling by default (currently they contain both project.json and .csproj files, and have a global.json that says to use preview2 tooling, because the project generator can emit projects of both types), but that won't affect the actual .csproj files which are the main aspect of the review I think.

SteveSandersonMS · 2017-02-22T18:26:33Z

src/Microsoft.AspNetCore.NodeServices.Sockets/Microsoft.AspNetCore.NodeServices.Sockets.csproj

+    <PackageTags>aspnetcore;aspnetcoremvc;nodeservices</PackageTags>
+    <RepositoryType>git</RepositoryType>
+    <RepositoryUrl>git://github.com/aspnet/javascriptservices</RepositoryUrl>
+    <NetStandardImplicitPackageVersion>1.6.1-*</NetStandardImplicitPackageVersion>


I'm not sure why VS puts this NetStandardImplicitPackageVersion specification into all the projects. @natemcmaster - do you know if we need it?

Depends. Should these packages target NETStandard.Library 1.6.0 or 1.6.1? The SDK defaults to 1.6.0. NetStandardImplicitPackageVersion will change that version.

OK, thanks. It seems they are needed, because some of the dependency packages target 1.6.1, and if this one doesn't specify the same, it produces downgrade warnings. I'll move these lines to a common build props file.

natemcmaster

Overall, there are a lot of lines of code (most likely generated by dotnet-migrate) that are redundant and can be removed.

I suggest putting all common properties -- such AssemblyOriginatorKeyFile, RepositoryUrl, etc -- into a single file the repo and using "Import" to pull them into individual projects. See https://github.com/aspnet/FileSystem/blob/dev/build/common.props for example.

Also, consider using the MSBuild properties <Company>, <Copyright>, and <Product>. The SDK can use these to auto-generate AssemblyInfo.cs and will put these in the nuspec of the generated nuget packages.

natemcmaster · 2017-02-22T18:28:43Z

JavaScriptServices.sln

 Global
 	GlobalSection(SolutionConfigurationPlatforms) = preSolution
 		Debug|Any CPU = Debug|Any CPU
+		Debug|x64 = Debug|x64


This was probably added automatically by VS or dotnet-migrate, but in general we don't need the x64 and x86 solution configurations for our projects. Consider removing these.

natemcmaster · 2017-02-22T18:31:34Z

global.json

-    "projects": ["src"],
-    "sdk": { "version": "1.0.0-preview2-1-003177" }
-}
+    "projects": ["src"]


Delete this whole file. The "projects" value is deprecated.

natemcmaster · 2017-02-22T18:32:09Z

samples/angular/MusicStore/MusicStore.csproj

+
+  <PropertyGroup>
+    <TargetFramework>netcoreapp1.1</TargetFramework>
+    <PreserveCompilationContext>true</PreserveCompilationContext>


Nit: you can remove the following settings as they are the default value:

PreserveCompilationContext
AssemblyName
OutputType
RuntimeFrameworkVersion

natemcmaster · 2017-02-22T18:37:32Z

samples/angular/MusicStore/MusicStore.csproj

+  </PropertyGroup>
+
+  <ItemGroup>
+    <Content Update="appsettings.json;ClientApp;node_modules;typings\**\*;Views\**\*;tsconfig.json;tsd.json;web.config;webpack.*.js;wwwroot\**\*">


Test it, but I'm pretty sure this can be removed. Microsoft.Net.Sdk.Web will publish these files by default. If anything, you may want to exclude some of these from publish, such as node_moduels, tsconfig, webpack, etc.

natemcmaster · 2017-02-22T18:40:30Z

samples/misc/NodeServicesExamples/NodeServicesExamples.csproj

+    <AssemblyName>NodeServicesExamples</AssemblyName>
+    <OutputType>Exe</OutputType>
+    <PackageId>NodeServicesExamples</PackageId>
+    <RuntimeFrameworkVersion>1.1.0</RuntimeFrameworkVersion>


This is an unsupported mix of runtime version and TFM. Change the framework to netcoreapp1.1 and remove this property.

This is true of several other projects in the PR.

natemcmaster · 2017-02-22T18:50:02Z

src/Microsoft.AspNetCore.NodeServices/Microsoft.AspNetCore.NodeServices.csproj

+    <PackageId>Microsoft.AspNetCore.NodeServices</PackageId>
+    <PackageTags>aspnetcore;aspnetcoremvc;nodeservices</PackageTags>
+    <RepositoryType>git</RepositoryType>
+    <RepositoryUrl>git://github.com/aspnet/javascriptservices</RepositoryUrl>


I suggest changing this to https:// instead. git:// isn't browser-friendly.

natemcmaster · 2017-02-22T18:50:22Z

src/Microsoft.AspNetCore.NodeServices/Microsoft.AspNetCore.NodeServices.csproj

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


Likewise here. These globs are probably not required.

There are files to be embedded from Content, so leaving this as-is, but please let me know if you still think this is wrong.

Removed the Exclude part

natemcmaster · 2017-02-22T18:51:13Z

templates/WebApplicationBasic/WebApplicationBasic.csproj

+    <OutputType>Exe</OutputType>
+    <PackageId>WebApplicationBasic</PackageId>
+    <RuntimeFrameworkVersion>1.1.0</RuntimeFrameworkVersion>
+    <PackageTargetFallback>$(PackageTargetFallback);dotnet5.6;portable-net45+win8</PackageTargetFallback>


Can these be dropped? I didn't see anything in the list of dependencies that would require these fallback frameworks anymore.

I don't know why the migration util put those there. I don't really even know what effect they would ever have. Removing.

natemcmaster · 2017-02-22T18:51:41Z

templates/WebApplicationBasic/WebApplicationBasic.csproj

+
+  <ItemGroup>
+    <Compile Remove="node_modules\**\*" />
+    <Content Update="appsettings.json;Views\**\*;web.config;wwwroot\**\*">


This can be removed. It is already the default for Sdk.Web projects.

natemcmaster · 2017-02-22T18:58:48Z

samples/angular/MusicStore/MusicStore.csproj

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


On all of your samples and template projects, consider adding <IsPackable>false</IsPackable>. This will allow you to execute dotnet pack on the solution and get packages for only those projects that should actually produce nuget packages.

Eilon

+1 to what @natemcmaster said, will look again once his comments are addressed.

MarkPieszak · 2017-02-23T01:27:15Z

With these updates everything is now only VS2017+ only, correct? @SteveSandersonMS @natemcmaster

natemcmaster · 2017-02-23T01:39:07Z

@MarkPieszak it means building this repo requires VS 2017, VS Code + C# 1.7, or .NET Core CLI 1.0.0-rc4

davidfowl · 2017-02-23T11:27:09Z

JavaScriptServices.sln

+Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "test", "test", "{BDE761D9-8E1C-4FB1-BB86-110393C59176}"
+EndProject
+Project("{E24C65DC-7377-472B-9ABA-BC803B73C61A}") = "test", "test\", "{A01B46C2-53D8-499D-8CDE-68E3E7B52804}"
+	ProjectSection(WebsiteProperties) = preProject


wtf is this 😄

Yes indeed. When are we getting a project type that means "just show me the files and don't try to build anything"?

I've never seen this section either. Is this a carry-over from ASP.NET 4?

@natemcmaster It's a "Web site" project (as in, just a folder full of files, without any .NET). It's pretty nasty, but does VS2017 have any better way of working with front-end resources? It no longer seems to have the "TypeScript" project type. People have been talking about having an "html" project type forever, but I still don't see anything resembling that in VS2017.

Ah, thanks for clarifying. I'm not sure if VS 2017 has these project types, and I haven't run into this requirement yet. But hey, if it makes it easier to work with files in VS 2017, go for it. It shouldn't affect compilation.

SteveSandersonMS · 2017-02-23T12:13:57Z

Thanks @natemcmaster for the feedback. I learned a lot! You're right that most of this stuff was produced by the migration tool, but it's nice to get it into a cleaner state.

natemcmaster · 2017-02-23T16:57:20Z

src/common.props

+    <RepositoryType>git</RepositoryType>
+    <RepositoryUrl>https://github.com/aspnet/javascriptservices</RepositoryUrl>
+
+    <GenerateNeutralResourcesLanguageAttribute>true</GenerateNeutralResourcesLanguageAttribute>


These Generate*Attribute settings default to true so you can remove them, but you still need to set the values for the generate attributes by setting properties for <Company> and etc.

These are the default proeprties we set in all of our other aspnet projects.

<Product>Microsoft ASP.NET Core</Product> <NeutralLanguage>en-US</NeutralLanguage> <Company>Microsoft Corporation.</Company> <Authors>Microsoft</Authors> <Copyright>© Microsoft Corporation. All rights reserved.</Copyright> <ProjectUrl>https://asp.net</ProjectUrl> <RequireLicenseAcceptance>true</RequireLicenseAcceptance> <Serviceable>true</Serviceable>

To clarify something...you don't see this in the build/common.props files in other github.com/aspnet projects, but instead you will see a PackageReference to "Internal.AspNetCore.Sdk". (see below).

Thanks for the pointers. I've added a reference to Internal.AspNetCore.Sdk (in common.props) and moved a few files around to make it pretty much the same as the other ASP.NET Core projects.

natemcmaster · 2017-02-23T16:58:33Z

Consider also adding a PackageReference to Internal.AspNet.Sdk to the projects in src/*.csproj that produce official packages. Internal.AspNet.Sdk is defined in our build tools repo and is how we set these common properties and build targets. We did this via package to make it easier to put common MSBuild code in one place instead of duplicating across 50-ish repos.

Internal.AspNetCore.Sdk extends the build to generate AssemblyMetadata("CommitHash", (git commit)) and AssemblyMetadata("Serviceable", "True"). These things aren't provided by the default targets from Microsoft.NET.Sdk.

natemcmaster · 2017-02-23T16:59:50Z

src/Microsoft.AspNetCore.AngularServices/Properties/AssemblyInfo.cs

-using System.Resources;
-using System.Runtime.CompilerServices;
-
-[assembly: AssemblyMetadata("Serviceable", "True")]


Unless you add Internal.AspNetCore.Sdk, you'll need to keep this one. It's not generated by Microsoft.NET.Sdk.

cref https://github.com/aspnet/BuildTools/blob/e4be7e105de6c3bf2e882bb76f3565305ca5a46d/src/Internal.AspNetCore.Sdk/build/GenerateAssemblyInfo.targets

Done (by referencing Internal.AspNetCore.Sdk)

Doesn't affect other scenarios (e.g., directly using Kestrel, which was already this fast).

…aSpa for consistency)

…l there's a better pure front-end project type

…s whenever it sees .ts files

…ld\" dir for consistency.

Eilon

Looks great, period!

Eilon · 2017-02-24T17:41:12Z

src/Microsoft.AspNetCore.NodeServices.Sockets/Microsoft.AspNetCore.NodeServices.Sockets.csproj

+  <Import Project="..\build\common.props" />
+
+  <PropertyGroup>
+    <Description>Socket-based RPC for Microsoft.AspNetCore.NodeServices</Description>


End sentence with a period

.

Eilon · 2017-02-24T17:42:06Z

src/Microsoft.AspNetCore.SpaServices/Microsoft.AspNetCore.SpaServices.csproj

+  <Import Project="..\build\common.props" />
+
+  <PropertyGroup>
+    <Description>Helpers for building single-page applications on ASP.NET MVC Core</Description>


Eilon · 2017-02-24T17:43:02Z

Oh and we should figure out why the CLA bot doesn't like you...

dnfclas added the cla-required label Feb 22, 2017

SteveSandersonMS requested review from Eilon and natemcmaster February 22, 2017 18:24

SteveSandersonMS commented Feb 22, 2017

View reviewed changes

natemcmaster suggested changes Feb 22, 2017

View reviewed changes

Eilon suggested changes Feb 23, 2017

View reviewed changes

davidfowl reviewed Feb 23, 2017

View reviewed changes

SteveSandersonMS force-pushed the csproj-migration branch from 2124890 to 56cdd09 Compare February 23, 2017 12:16

natemcmaster reviewed Feb 23, 2017

View reviewed changes

SteveSandersonMS mentioned this pull request Feb 24, 2017

Upgrade to VS 2017 #684

Closed

SteveSandersonMS added 15 commits February 24, 2017 14:54

Make HMR dramatically faster if you're using IIS Express

f97cb73

Doesn't affect other scenarios (e.g., directly using Kestrel, which was already this fast).

Migrate NodeServices and NodeServices.Sockets to csproj

1541a2b

Migrate SpaServices/AngularServices/ReactServices projects to csproj

b4e7d7b

Add some solution items

5a0cf0e

Migrate LatencyTest/Webpack/NodeServicesExamples samples to csproj

4e77054

Migrate Angular and React samples to csproj

9927664

Add SPA templates to VS .sln (also renaming Aurelia project to Aureli…

bc66842

…aSpa for consistency)

Migrate WebApplicationBasic template to csproj

765586b

Add template-builder and test dirs to VS solution as "web sites" unti…

f50a015

…l there's a better pure front-end project type

Add missing hint in solution file

f4e151a

Make sure VS doesn't litter the disk with irrelevant/harmful .js file…

7c95f7c

…s whenever it sees .ts files

Respond to csproj migration code review feedback

2c780a4

Add references to Internal.AspNetCore.Sdk. Move build files into "bui…

f125d3f

…ld\" dir for consistency.

Remove unnecessary Exclude attributes as per CR feedback

45a6460

Only reference released versions of ASP.NET Core packages

0429fbf

natemcmaster approved these changes Feb 24, 2017

View reviewed changes

Eilon approved these changes Feb 24, 2017

View reviewed changes

SteveSandersonMS force-pushed the csproj-migration branch from 9a433dd to 0429fbf Compare February 28, 2017 09:11

End package descriptions with periods

d0c3e81

SteveSandersonMS merged commit a79bc75 into dev Feb 28, 2017

SteveSandersonMS deleted the csproj-migration branch February 28, 2017 09:17

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

		<PropertyGroup>

Migrate to csproj #703

Migrate to csproj #703

Uh oh!

Conversation

SteveSandersonMS commented Feb 22, 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!

natemcmaster left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Eilon left a comment

Choose a reason for hiding this comment

Uh oh!

MarkPieszak commented Feb 23, 2017

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS Feb 23, 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!

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!

SteveSandersonMS commented Feb 23, 2017

Uh oh!

Choose a reason for hiding this comment

natemcmaster commented Feb 23, 2017 •

edited

Loading

SteveSandersonMS Feb 23, 2017 •

edited

Loading

SteveSandersonMS Feb 24, 2017 •

edited

Loading