-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Re-enable functional tests on full .NET Framework #5877
Conversation
- #5873 - creating an EXE for the test project seems to work around #5873 - also avoids dotnet/sdk#926 when building in Visual Studio nit: clean up duplicate test data
FYI I went back and forth between trying to create a minimal repro and looking for a workaround. Found the workaround first. |
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.
As long as it works 👍
<PackageTargetFallback>$(PackageTargetFallback);portable-net451+win8</PackageTargetFallback> | ||
<TargetFrameworks>netcoreapp1.1;net452</TargetFrameworks> | ||
<TargetFrameworks Condition="'$(OS)' != 'Windows_NT'">netcoreapp1.1</TargetFrameworks> | ||
<PackageTargetFallback Condition=" '$(TargetFramework)' == 'netcoreapp1.1' ">$(PackageTargetFallback);portable-net451+win8</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.
"'$(OS)' != 'Windows_NT'"
versus " '$(TargetFramework)' == 'netcoreapp1.1' "
. We should pick one spacing
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.
🙈 I find conditions easier to read with more spaces. But, no need for an engineering guideline / principle / law here. As long as people aren't adding or removing spaces in otherwise-untouched code, author's discretion to the max.
Generate an EXE to take advantage of Microsoft.NET.RuntimeIdentifierInference.targets special cases. This works | ||
around #5873. Also avoids dotnet/sdk#926 when building with msbuild.exe e.g. in Visual Studio. | ||
--> | ||
<OutputType Condition=" '$(TargetFramework)' != 'netcoreapp1.1' ">EXE</OutputType> |
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.
Do we need to file an SDK bug for this?
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.
We need a bug somewhere. I need to minimize my repro project first though.
nit: clean up duplicate test data