Binding redirects should be enabled by default when target .NET Framework

[terrajobst]

We don't support .NET Framework projects via SDK-style projets yet, but many of us are dog fooding this experience already. I had to add the following two lines to my project to get binding redirects in my unit testing project:

<PropertyGroup Condition=" '$(TargetFrameworkIdentifier)' == '.NETFramework' ">
    <AutoGenerateBindingRedirects>true</AutoGenerateBindingRedirects>
    <GenerateBindingRedirectsOutputType>true</GenerateBindingRedirectsOutputType>
</PropertyGroup>

We should make sure this ends up in the default props so that customers don't have to know that -- they expect binding redirects to just work.

[eerhardt]

We have

sdk/src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.Sdk.BeforeCommon.targets

Line 46 in 1042b68

<PropertyGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework' and '$(OutputType)' == 'Exe'">

Which sets the first property when you are a .NET Framework Exe.

Is there a way to tell when the project is a test project? I don't think we would want this on for every .NET Framework class library, would we?

[terrajobst]

I don't think we would want this on for every .NET Framework class library, would we?

Logically you'll want to set it for everything that gets executed like an app, e.g. unit testing projects.

Is there a way to tell when the project is a test project?

Not sure there is an easy way to do that; you could in theory detect references to popular unit testing frameworks, but that seems fragile.

What's the harm in turning it always on for .NET Framework? Worst case the output folder contains an additional .dll.config that people just ignore, but at least they don't need to worry about binding redirects (like on all other runtimes). Also, I'm under the impression that binding redirects are also honored at build time, but that might be wrong.

Tagging a few more folks for input.

@ericstj @davkean

[bradwilson]

For dotnet xunit, we're pushing both of these settings, plus several more (taken from https://github.com/xunit/xunit/blob/9430d9978894f18ac55b91bfd648869e9b541e2a/src/dotnet-xunit/Program.cs#L92-L102):

<Project>
  <PropertyGroup>
    <AutoGenerateBindingRedirects>true</AutoGenerateBindingRedirects>
    <CopyLocalLockFileAssemblies>true</CopyLocalLockFileAssemblies>
    <CopyNuGetImplementations>true</CopyNuGetImplementations>
    <DebugType Condition=""'$(TargetFrameworkIdentifier)' != '.NETCoreApp'"">Full</DebugType>
    <GenerateBindingRedirectsOutputType>true</GenerateBindingRedirectsOutputType>
    <GenerateRuntimeConfigurationFiles>true</GenerateRuntimeConfigurationFiles>
    <GenerateDependencyFile>true</GenerateDependencyFile>
  </PropertyGroup>
</Project>

My understanding is that Microsoft.NET.Test.SDK already does similar things (but that's worth double-checking). You may not need to special-case unit test projects.

[livarcocc]

@Faizan2304 @codito to check that the test SDK is doing the right thing already.

[nguerrera]

Related to #909 in that we don't treat desktop tests as exes but should for binding redirects and for RID inference. We should probably fix both together.

[nguerrera]

The test SDK doesn't do this for you today. I just hit this myself.

Turns out you can also workaround this by setting <OutputType>exe</OutputType> because the test SDK injects a Main anyway. This works around this and #909 though it's a bit of a hack. We should indeed make both just work for test projects without changing the output type unnecessarily or having to pick a RID or set the extra bools shown in @terrajobst's report.

[nguerrera]

Perhaps we should have <TreatAsExecutable>true</TreatAsExecutable> in test SDK. And we would default TreatAsExecutable to OutputType==Exe in core SDK and replace all checks of OutputType==Exe with TreatAsExecutable==true.

[eerhardt]

FWIW - we had a similar concept in project.json world:

https://github.com/dotnet/cli/blob/rel/1.0.0-preview2/src/Microsoft.DotNet.ProjectModel/Project.cs#L126-L132

        public bool HasRuntimeOutput(string configuration)
        {
            var compilerOptions = GetCompilerOptions(targetFramework: null, configurationName: configuration);

            // TODO: Make this opt in via another mechanism
            return compilerOptions.EmitEntryPoint.GetValueOrDefault() || IsTestProject;
        }

       public bool IsTestProject => !string.IsNullOrEmpty(TestRunner);

[bradwilson]

the test SDK injects a Main anyway

This is a huge mistake in my opinion. People are already complaining to me about the fact that dotnet test can't run tests from a real executable project. There is no such limitation with the just released dotnet xunit runner.

[nguerrera]

This is a huge mistake in my opinion.

You just convinced me of that.

[terrajobst]

So, what's the summary? I'm a bit at loss what you guys are proposing here.

[nguerrera]

@terrajobst This is my proposal: #1070 (comment)

[eerhardt]

Are there other names that would fit better?

• TreatAsExecutable
• HasRuntimeOutput
• HasRunnableOutput
• HasExecutableOutput
• IsExecutable
• IsRunnable

It probably doesn't matter too much. Just wanted to throw it out there.

[nguerrera]

I don't have a strong opinion about the name. All of those are workable to me.

[nguerrera]

This will be fixed by #1178 once the test project sets HasRuntimeOutput to true, which is now tracked by microsoft/vstest#792

[JoseFMP]

Any news on this issue? Was it shipped?