Pare back workarounds for fixed problems #203
Conversation
- same for problems with centralized workarounds - major visible change is web sites are now x86 programs when running on .NET Framework - add lower-level workarounds to functional tests - support `/p:GenerateBaselines=true` on build command line - remaining compilation warnings cannot be avoided - dotnet/sdk#954 blocks disabling these warnings; #202 tracks an eventual fix nit: sort dependencies
| @dougbu, |
| <Import Project="..\..\build\common.props" /> | ||
| | ||
| <PropertyGroup> | ||
| <IsPackable>false</IsPackable> |
There was a problem hiding this comment.
This isn't a workaround. Can you leave the IsPackable settings?
There was a problem hiding this comment.
Huh? Build doesn't pack these projects.
| | ||
| <ItemGroup> | ||
| <DotNetCliToolReference Include="Microsoft.EntityFrameworkCore.Tools.DotNet" Version="1.2.0-*" /> | ||
| <DotNetCliToolReference Include="Microsoft.Extensions.SecretManager.Tools" Version="1.0.1-*" /> |
There was a problem hiding this comment.
This isn't a workaround. Can you leave it?
There was a problem hiding this comment.
What are the tools used for in this context? Project builds and runs fine and that's all we need for the sample.
There was a problem hiding this comment.
This tool is used to set secrets from command line. Even if not in automated tests, it's still used during "dotnet restore". This is one of the validations this project provides. Restore would fail if broke the secretmanager tool.
There was a problem hiding this comment.
No issue occurs during dotnet restore. I don't get your point here.
There was a problem hiding this comment.
No issue occurs during dotnet restore.
That means restore on Microsoft.Extensions.SecretManager.Tools is working. If we were to change this package to target netcoreapp2.0, this would cause restore to fail because dotnet-CLI does not (yet) support tools packages that target .NET Core 2.0.
There was a problem hiding this comment.
My point was restore of the web site project works with my changes in place. But, I'll revert this removal.
| </PropertyGroup> | ||
| | ||
| <PropertyGroup Condition=" '$(TargetFramework)' != 'netcoreapp1.1' "> | ||
| <!-- Work around https://github.com/dotnet/sdk/issues/926. Align with bitness of the web site projects. --> |
There was a problem hiding this comment.
If you set OutputType=exe, does this an the binding redirects issue go away?
There was a problem hiding this comment.
Yes, but changing $(OutputType) has additional implications that are irrelevant for test projects. And, it's Just Weird:tm:.
There was a problem hiding this comment.
What are those additional implications? .NET Core test apps are actually OutputType=exe under the hood.
There was a problem hiding this comment.
This isn't a .NET Core issue; see the condition 2 lines up. Binding redirects are needed only with full .NET Framework. And, tests are executed using testhost.exe or testhost.x86.exe.
There was a problem hiding this comment.
My point was I don't buy "it's weird" as a reason not to use OutputType=exe. I know this is for .NET Framework.
What are the additional implications? I'm wondering why we need 10 lines of comments and workaround when one line would do.
There was a problem hiding this comment.
What are the additional implications?
For example, bin\Debug\net452\win7-x64\FunctionalTests.exe does absolutely nothing except exit.
There was a problem hiding this comment.
Discussed offline. I'll leave this as I've got it. If prefer just the bug links to reduce the comment verbiage, LMK.
natemcmaster left a comment
natemcmaster left a comment There was a problem hiding this comment.
In person discussion. Not worth going more rounds on this one. If tests, pass 
Much appreciated. I'll make a couple of the changes you requested and get this in. |
/p:GenerateBaselines=trueon build command linenit: sort dependencies