-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrade to VS 2017 #138
Upgrade to VS 2017 #138
Conversation
1fcf798
to
b458125
Compare
@pranavkm would you mind reviewing? |
@@ -21,7 +21,7 @@ k-standard-goals | |||
{ "PackageCacheUploader", new [] { "netcoreapp1.0" } }, | |||
{ "DependenciesPackager", new [] { "netcoreapp1.0" } }, | |||
{ "NuGetPackageVerifier", new [] { "netcoreapp1.0" } }, | |||
{ "SplitPackages", new [] { "net451" } }, | |||
{ "SplitPackages", new [] { "net451" } } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Undo
<Project Sdk="Microsoft.NET.Sdk" ToolsVersion="15.0"> | ||
<Import Project="..\..\build\common.props" /> | ||
<PropertyGroup> | ||
<VersionPrefix>1.0.1</VersionPrefix> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, we could just commonize their version. There really isn't a reason for them to be different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do this in a follow PR along with stopping the cross-compilation. Primary goal of this PR was to preserve existing build input/output but w/ csproj.
<PackageReference Include="Microsoft.NETCore.App" Version="1.0.0" /> | ||
<PackageReference Include="System.Runtime.Loader" Version="4.0.0" /> | ||
</ItemGroup> | ||
<ItemGroup Condition=" '$(TargetFramework)' == 'net451' "> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this section needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Will remove.
<PropertyGroup> | ||
<VersionPrefix>1.0.1</VersionPrefix> | ||
<TargetFrameworks>netcoreapp1.0;net451</TargetFrameworks> | ||
<RuntimeIdentifier Condition=" '$(TargetFramework)'!='netcoreapp1.0' ">win7-x64</RuntimeIdentifier> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we invert this? $(TargetFramework)=='net451'
? We're likely to update netcoreapp1.x
but unlikely to change the net451
targeting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's very intentional. It works around issues with RID inference in the SDK. See microsoft/vstest#287
<EmbeddedResource Include="**\*.resx" /> | ||
</ItemGroup> | ||
<ItemGroup> | ||
<PackageReference Include="Microsoft.DotNet.ProjectModel" Version="1.0.0-*" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you pin this to the preview2
version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More interestingly, does this work with msbuild based projects? Does the tool need to be redesigned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will pin. Yeah, the tool only works on project.json. #127 is tracking progress on this
</ItemGroup> | ||
|
||
<ItemGroup Condition=" '$(TargetFramework)' == 'netstandard1.6' "> | ||
<PackageReference Include="System.Runtime.Serialization.Primitives" Version="4.1.1-*" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pin it?
<PropertyGroup> | ||
<Description>Copies NuGet packages form a given source folder into a specific set of folders based on a CSV file.</Description> | ||
<VersionPrefix>1.0.1</VersionPrefix> | ||
<TargetFrameworks>netcoreapp1.0;net451</TargetFrameworks> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you create a follow up to stop cross compiling this? There's seemingly only one location which runs this (https://github.com/aspnet/Coherence/blob/8f8c30b5b57e9171b444c9b505db70386a17a9f3/makefile.shade#L53) and it should be able to run it as a CoreCLR app there.
<ItemGroup Condition=" '$(TargetFramework)' == 'netcoreapp1.0' "> | ||
<PackageReference Include="Microsoft.NETCore.App" Version="1.0.0" /> | ||
</ItemGroup> | ||
<ItemGroup Condition=" '$(TargetFramework)' == 'net452' "> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove?
<PropertyGroup> | ||
<TargetFrameworks>netstandard1.6;net452</TargetFrameworks> | ||
<RuntimeIdentifier Condition=" '$(TargetFramework)'!='netcoreapp1.0' ">win7-x64</RuntimeIdentifier> | ||
<PackageTargetFallback Condition=" '$(TargetFramework)' == 'netstandard1.6' ">$(PackageTargetFallback);dnxcore50</PackageTargetFallback> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove
<PropertyGroup> | ||
<TargetFrameworks>netstandard1.6;net452</TargetFrameworks> | ||
<RuntimeIdentifier Condition=" '$(TargetFramework)'!='netcoreapp1.0' ">win7-x64</RuntimeIdentifier> | ||
<PackageTargetFallback Condition=" '$(TargetFramework)' == 'netstandard1.6' ">$(PackageTargetFallback);dnxcore50</PackageTargetFallback> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove
Port this project to VS 2017.
cc @ajaybhargavb @javiercn