Migrate solution to VS2017 #411
Conversation
…edirects in app.config
bindingRedirects
… installed can build still
…dio SDK targets. Removed TreatWarningsAsErrors
… propagated to other projects.
|
@dotnet-bot test this please |
|
@conniey Fixed :) |
|
@mmitche Life is good again! Thanks! |
| [Cmdlet(VerbsCommon.Get, "MsBuild")] | ||
| public class LocateMsBuildCmdlet : Cmdlet | ||
| { | ||
| private const string MsBuildVersion15 = "15"; |
There was a problem hiding this comment.
Why can't we only support 15?
| @@ -17,7 +17,7 @@ | |||
| <!-- Trying to mitigate the number of warnings that the solution contains by | |||
| adding this into our Release builds. As a result, it should be caught in | |||
| our PRs because of the CI builds --> | |||
| <PropertyGroup Condition=" '$(Configuration)' == 'Release' "> | |||
| <!-- <PropertyGroup Condition=" '$(Configuration)' == 'Release' "> | |||
There was a problem hiding this comment.
This was commented out because of the CS1701 warnings due to the different assembly versions it is resolving.
When building our project, we are encountering ~900 warnings like this:
warning CS1701: Assuming assembly reference 'System.Runtime, Version=4.0.20.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' used by 'Microsoft.Fx.Portability' matches identity 'System.Runtime, Version=4.1.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' of 'System.Runtime', you may need to supply runtime policy [D:\git\conniey\dotnet-apiport\src\ApiPort\ApiPort.csproj]
The reason for that is because Microsoft.Fx.Portability targets netstandard1.3 and ApiPort targets netcoreapp1.0. So at build time, Microsoft.Fx.Portability references System.Runtime 4.0.20.0... but ApiPort uses System.Runtime 4.1.0.0. This should not result in problems since we are only adding to the API surface area... but there is an issue about this aspnet/Mvc#5050 (comment). Should I make another in the right location?
There was a problem hiding this comment.
Another option is to suppress the CS1701 warning.... but I am afraid that it will result in suppressing any binding redirect errors that may happen.
| @@ -0,0 +1,87 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
There was a problem hiding this comment.
We should transition this project to PackageReference as well. Not needed for this review, but it'd be good to get done.
There was a problem hiding this comment.
I had tried this before and it resulted in a bunch of errors because the old MsBuild targets don't seem to know how to resolve/display it for these old csproj files.
| <InstallationTarget Version="[14.0,]" Id="Microsoft.VisualStudio.Pro" /> | ||
| <InstallationTarget Version="[14.0,]" Id="Microsoft.VisualStudio.Ultimate" /> | ||
| <InstallationTarget Version="[14.0,]" Id="Microsoft.VisualStudio.Premium" /> | ||
| <InstallationTarget Version="[14.0,16.0)" Id="Microsoft.VisualStudio.Pro" /> |
There was a problem hiding this comment.
Do we need all of these? I think if we just using Microsoft.VisualStudio.Community that would work as it's the lowest common denominator... but don't take my word for it...
There was a problem hiding this comment.
I looked through the vsix-extension-schema docs and it appears that it is just an enumeration... there doesn't appear to be a heirarchy.
| name.Name = assemblyIdentity.Name.Value; | ||
| #if !COREFX | ||
| name.CultureInfo = new CultureInfo(assemblyIdentity.Culture); | ||
| #if NETSTANDARD1_3 || NETSTANDARD1_6 |
There was a problem hiding this comment.
Why not use the FEATURE_[name] pattern?
This migrates our solution to Visual Studio 2017. There are a few additions/changes:
Set-VsDevCmd.ps1locate a version of 2017's VsDevCmd.bat to set environment variables.dotnet vstest, but I kept running into dotnet/cli#4837 for ApiPortVS.Test (since it is an old project)... so for new tests, I usedotnet test, but old tests, I locate vstest.dotnet msbuildbut it does not build for some reason because the MSBuild toolchain is different from VS.