Generate binding redirects should be on by default

[jcouv]

A number of customers ran into issues with ValueTuple in .NET Standard 2.0 in unittests with EntityFramework. It turns out the problem was missing binding redirects.
@weshaggard recommended two settings for such projects:

    <AutoGenerateBindingRedirects>true</AutoGenerateBindingRedirects>
    <GenerateBindingRedirectsOutputType>true</GenerateBindingRedirectsOutputType>

Those settings are apparently set for executable project templates, but not for unittests.

dotnet/efcore#9046 (comment)
ErikEJ/EntityFramework.SqlServerCompact#463 (comment)

FYI @ericstj @terrajobst @nguerrera

[ericstj]

I really think MSBuild should just fix the defaults. /cc @rainersigwald

[rainersigwald]

@ericstj Can you convince me that that's not a breaking change? It certainly seems like it would be.

[ericstj]

It can't be completely breaking since we're making it in various SDKs and templates. I know we don't want to enable it for web projects since they need redirects in the source app.config. Other than that I think it should be safe to turn it on.

[davkean]

It will be breaking to switch this on, SDK and templates are new code and doesn't change the behavior under a project.

[ericstj]

Sure, that's fine to make the change in these template for now. I agree that templates are new code and its fine to change the default. It just feels like this is a whack-a-mole.

I think MSBuild should do the work to understand what is actually breaking about such a change and make a reasonable compromise between a minimally breaking change and not having to require every project carry this state. @davkean perhaps you can share how you think this is breaking so that MSBuild could perhaps come up with a non-breaking version of the fix. I've proposed that it be turned on by default for everything but web projects, can you describe what's broken there and how we refine that?

[rainersigwald]

@ericstj I find your position here really confusing.

This is how I think of it:

1. The .NET assembly loader respects binding redirects. Having them can result in an observable runtime change.
2. Common targets don't generate binding redirects by default.
3. If we changed the default, binding redirects would be created for some projects when built with new toolsets, but not created on older toolsets.
4. That makes changing the default an observable (breaking) change.

Am I missing something here? You seem to think that there's a way to create binding redirects when they are both a) required and b) not currently present, but not break people. I don't see what that is, other than the current "you should have binding redirects" warnings.

[ericstj]

You need to make the fix so that the only observable runtime change is removing a exception. We have long standing precedent in DevDiv Compat for allowing a breaking change (with docs) when that breaking change is merely removing an exception. Theoretically this fix should be implementable in a way that its merely removing the runtime exception: that was the whole basis for the original feature. I'm saying do the work to make that true. If folks are claiming that its not possible you need to be specific about those concerns so that you can determine if you can code around them.

[terrajobst]

I also feel like we need to apply a different bar for (potentially) breaking changes between "launching an existing app binary on a few framework" and "building an existing code base with a new toolset". In the later, breaking changes can be mitigated by giving people an opt-out (which exists for automatic binding redirects), and is generally considered acceptable (which doesn't hold for the former). We need to weigh the potential disruption for developers with the value gain. As @ericstj said: with binding redirects we've reached the point where we're now playing whack-a-mole, for example here is another one. Hence, I'm in favor this being automatically on.

[davkean]

@ericstj I'm not sure I understand how we could detect that we would be removing an exception. It's perfectly valid to load two versions of the same assembly in the Load context, either by deployment (having both in the GAC) or by hooking onto assembly resolve and loading the binary yourself. How exactly would you detect those situations? It basically exactly the same argument for not changing the binding policy.

[davkean]

Either way, this isn't a project system concern - this is a handshake between .NET <-> MSBuild. I'm moving it to the MSBuild repo.

[davkean]

This issue was moved to dotnet/msbuild#2481