Migrate to csproj #703
6 participants
| + <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.
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.
| EndProject | ||
| 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.
| - "projects": ["src"], | ||
| - "sdk": { "version": "1.0.0-preview2-1-003177" } | ||
| -} | ||
| + "projects": ["src"] |
| @@ -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.
| + | ||
| + <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
| + </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.
| + | ||
| + <PropertyGroup> | ||
| + <TargetFramework>netcoreapp1.0</TargetFramework> | ||
| + <PreserveCompilationContext>true</PreserveCompilationContext> |
Nit: likewise here, remove the default values.
(Repeat same comment for most of the projects below)
| + <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.
| + </ItemGroup> | ||
| + | ||
| + <ItemGroup> | ||
| + <PackageReference Include="Microsoft.AspNetCore.Diagnostics" Version="1.1.0" /> |
Consider using Microsoft.AspNetCore metapackage instead. It would trim this section to 3 dependencies:
Microsoft.AspNetCore
Microsoft.AspNetCore.Mvc
Microsoft.Extensions.Logging.Debug
| + | ||
| + <PropertyGroup> | ||
| + <Description>Helpers for building Angular 2 applications on ASP.NET Core.</Description> | ||
| + <VersionPrefix>1.1.0-beta2</VersionPrefix> |
The more conventional way to express this is
<VersionPrefix>1.1.0</VersionPrefix>
<VersionSuffix>beta2</VersionSuffix>| + <RepositoryType>git</RepositoryType> | ||
| + <RepositoryUrl>git://github.com/aspnet/javascriptservices</RepositoryUrl> | ||
| + <NetStandardImplicitPackageVersion>1.6.1-*</NetStandardImplicitPackageVersion> | ||
| + <GenerateNeutralResourcesLanguageAttribute>false</GenerateNeutralResourcesLanguageAttribute> |
Consider removing AssemblyInfo.cs and letting the SDK generate these assembly attributes.
| + <PackageReference Include="Microsoft.AspNetCore.Mvc.TagHelpers" Version="1.1.0" /> | ||
| + </ItemGroup> | ||
| + | ||
| + <ItemGroup Condition=" '$(TargetFramework)' == 'net451' "> |
This whole ItemGroup is usually redundant because your app will reference NETStandard.Library. It can be removed.
| + <PackageTags>aspnetcore;aspnetcoremvc;nodeservices</PackageTags> | ||
| + <RepositoryType>git</RepositoryType> | ||
| + <RepositoryUrl>git://github.com/aspnet/javascriptservices</RepositoryUrl> | ||
| + <NetStandardImplicitPackageVersion>1.6.1-*</NetStandardImplicitPackageVersion> |
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.
| + </PropertyGroup> | ||
| + | ||
| + <ItemGroup> | ||
| + <EmbeddedResource Include="Content\**\*" Exclude="bin\**;obj\**;**\*.xproj;packages\**;@(EmbeddedResource)" /> |
Pretty sure you can remove this line. It doesn't appear that this project would embed anything in these folders by default.
It does embed resources from the Content folder (some .js files built from the TypeScript source). Let me know if you still think this is wrong.
Oh, didn't realize embedded js was intended. You can probably remove the "Exclude" value since it Content\**\* doesn't match any of the exclude patterns. Otherwise this looks good.
Yes, you're right - the Exclude values can be removed. Done.
| + | ||
| + <PropertyGroup> | ||
| + <Description>Invoke Node.js modules at runtime in ASP.NET Core applications.</Description> | ||
| + <VersionPrefix>1.1.1-alpha</VersionPrefix> |
Some of these projects are RTM and some are not, so they probably don't all version the same.
| + <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.
| + </PropertyGroup> | ||
| + | ||
| + <ItemGroup> | ||
| + <EmbeddedResource Include="Content\**\*" Exclude="bin\**;obj\**;**\*.xproj;packages\**;@(EmbeddedResource)" /> |
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.
| + <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.
| + | ||
| + <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.
|
With these updates everything is now only VS2017+ only, correct? @SteveSandersonMS @natemcmaster |
|
@MarkPieszak it means building this repo requires VS 2017, VS Code + C# 1.7, or .NET Core CLI 1.0.0-rc4 |
| +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 |
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.
|
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. |
| + <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.
|
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 |
| -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.
| + <Import Project="..\build\common.props" /> | ||
| + | ||
| + <PropertyGroup> | ||
| + <Description>Socket-based RPC for Microsoft.AspNetCore.NodeServices</Description> |
| + <Import Project="..\build\common.props" /> | ||
| + | ||
| + <PropertyGroup> | ||
| + <Description>Helpers for building single-page applications on ASP.NET MVC Core</Description> |
|
Oh and we should figure out why the CLA bot doesn't like you... |
This PR moves all the projects to
.csprojand rebuilds the.slnto work with VS017RC. I'll squash all these commits before merging it intodev, but it's probably easier to review split up into all these commits.There's still one more thing to be done - change the
templatesprojects so they use RC4 tooling by default (currently they contain bothproject.jsonand.csprojfiles, and have aglobal.jsonthat says to use preview2 tooling, because the project generator can emit projects of both types), but that won't affect the actual.csprojfiles which are the main aspect of the review I think.