search

dotnet / docs

This repository contains .NET Documentation.
https://learn.microsoft.com/dotnet
Creative Commons Attribution 4.0 International
4.27k stars 5.9k forks source link

Misleading page : change affects all versions of .NET, not just .NET 5 #21952

Closed Kryptos-FR closed 3 years ago

Kryptos-FR commented 3 years ago

The page is misleading. It states that the change only affects .NET 5, but in fact it also affect any version of .NET framework if the SDK for .NET 5 has been installed (regardless of whether any other SDKs are also installed).

I have a .NET 4.5.2 solution with a few WPF and Winforms projects including one that was explicitly set to <OutputType>Exe</OutputType> to make sure the console was displayed. Upon updating Visual Studio (without any code or config change), it started to break (no console shown).

I would have expected the rule rewriting the OutputType to be guarded and check that .NET 5 is part of the targeted frameworks. Or this page to be found in a more reachable section of the documentation: it took me a while to find it because I wasn't expected a change in .NET 5 to affect .NET Framework.

Steps to reproduce

Create a new winforms app. Change the target framework to net452. Set the output type to Exe explicitly --> no console.

Related issue: https://github.com/dotnet/sdk/issues/13331

Document Details

⚠ Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

dsplaisted commented 3 years ago

Thanks for reporting this, the breaking change documentation is not correct. It should say that it applies to the .NET SDK 5.0.100 and higher.

However, it is possible that we should have restricted the inference of the WinExe output type to projects targeting .NET 5 and higher (ie matching what the breaking change notification currently says). We will consider doing so, however it likely wouldn't make it until the February update of the SDK, so most people who would hit this may already have done so by then.

StevenBonePgh commented 3 years ago

@dsplaisted Or as I mentioned in a linked issue, you could restrict the 'infer' behavior to function only if the output type was not explicitly set. Which is what the word 'infer' implies. If I tell you explicitly what output type I want and you silently ignore my request and do something else - that is NOT inference.

If you would only infer when not otherwise explicitly set you would have achieved all of your goals without having a breaking change. At the minimum, with this sort of breaking change, I would expect a compiler warning if the output type is set without disabling inference - it would have broken me but only until I looked at the compiler warning output.

And yes, you should fix it. This is going to start hitting people as they attempt to migrate legacy projects to the SDK project format to port to .NET 5, and that will be a continual process where people can run into this for years. I lost a few days on this, no luck when searching for a possible cause, etc. My guess is others landing here have similar stories of lost time.

vuplea commented 3 years ago

I think there are many places where the breaking change potential of newer Sdks is not acknowledged.

I am guessing that projects should use global.json to not risk breaking changes (VS updates or Sdk installs can affect development builds, and there are CIs that do might come with an unexpected Sdk update, e.g. Microsoft-hosted agents that come with the latest VS installed - arguably a dangerous choice to such a system that does not "freeze" images though).

So I think global.json usage should be recommended, instead of considering it an advanced feature. If this is correct, I can open a separate doc issue.

gewarren commented 3 years ago

@PriyaPurkayastha do you have any thoughts about using global.json to not risk breaking changes in the SDK? Also, I'm wondering if we need a separate section in the breaking change docs for SDK updates.

dsplaisted commented 3 years ago

We do not generally recommend pre-emptively using global.json to pin the SDK version to avoid breaking changes. We do try to avoid breaking changes, and global.json is more there as an escape hatch in case you run into one. Even then, you don't necessarily need to use global.json - you may be able to simply set up your CI to use the version of the .NET SDK that doesn't have the break, until it is fixed or you've worked around it.

@StevenBonePgh The OutputType for a project that doesn't specify it is already Library, so we couldn't change that. It looks like we will probably change the logic so it only converts Exe to WinExe when targeting .NET 5 and higher. So folks could still run into this when writing or porting projects to .NET 5, but it won't change the behavior for projects that were building with previous SDKs.

RussKie commented 3 years ago

We're observing the same behaviour with .NET Core 3.1 Windows Forms app. The sample provided here doesn't behave as expected with either Exe or WinExe unless DisableWinExeOutputInference=true is defined. And whilst https://github.com/dotnet/sdk/pull/12448 (doc'ed in https://github.com/dotnet/docs/issues/20373) may not be source of the issue, the breaking doc does look somewhat misleading in the absence of another doc describing the changed behaviour.

NikolaosWakem commented 3 years ago

Making this change apply to old 3.1 targeted apps was a mistake and should be fixed ASAP..

This change suddenly breaks apps that just happen to be rebuilt on 1 Mar 2021 on DevOps that are happily targeted to 3.1

This is a ticking time bomb for lots of people..

dsplaisted commented 3 years ago

@NikolaosWakem Is there something special about March 1st that means people will now be hitting this who weren't before?

NikolaosWakem commented 3 years ago

@NikolaosWakem Is there something special about March 1st that means people will now be hitting this who weren't before?

No.. was just and example of me getting randomly taken out by the change, when I made a small unrelated change to a different 3.1 app in the same repo causing the build to automatically run and by complete luck I noticed the problem before rolling out the new version of the suite of apps. Maybe a land mine is better metaphor than time bomb.. still not good

Kryptos-FR commented 3 years ago

To add to that, I still don't see the point of that change. What scenario doesn't it serve?

Surely, anyone making a new WPF or Winforms application would just use the template in VS which would have set the target type to WinExe by itself (like it did until now). Existing applications would also have set the proper value. So for me the audience of that change is zero people/project.

Other the other hand it does break every project like mine where we have relied for years on the difference between Exe and WinExe to decide whether to display the console or not.

i strongly believe the dotnet team must own the fact that it was a mistake and revert that for .NET 6 (or even now in .NET 5). You have been telling us for years that in most scenario you can't make breaking changes (e.g. why ArrayList is still a thing in .NET Core even though it has zero legitimate use). But you did make a big breaking change that was almost invisible without even explaining why this was needed.

I'm losing faith in you doing the right thing in the future. I don't like how things are going. The community involvement of the Wpf team has been sub-par compared to other teams (e.g. WinForms team, language team), and this just one more example. At least you should have conducted a survey to ask us developers what we thought of that change. Breaking 10 years of existing practices is not something that would be acceptable in any team.

dsplaisted commented 3 years ago

I acknowledge that we made a mistake in making this change apply to existing target frameworks. It should have only applied to projects targeting .NET 5 and higher.

The reason we did the change at all was so that .NET MAUI projects would be simpler. They will multi-target between different target platforms, and need to have an OutputType of WinExe when targeting Windows, but Exe for other targets. Without this change, they would need to look something like this:

  <PropertyGroup>
    <TargetFrameworks>net6.0-ios14.0;net6.0-android;net6.0-windows10.0.19041</TargetFrameworks>
    <OutputType>Exe</OutputType>
    <OutputType Condition="'$([MSBuild]::GetTargetPlatformIdentifier('$(TargetFramework)'))' == 'Windows'">WinExe</OutputType>
  </PropertyGroup>

With this change, the complicated and ugly Condition on OutputType isn't necessary:

  <PropertyGroup>
    <TargetFrameworks>net6.0-ios14.0;net6.0-android;net6.0-windows10.0.19041</TargetFrameworks>
    <OutputType>Exe</OutputType>
  </PropertyGroup>
dsplaisted commented 3 years ago

The community involvement of the Wpf team has been sub-par compared to other teams (e.g. WinForms team, language team), and this just one more example.

Also FYI this wasn't at all the WPF team's fault. While we ran it by other teams including WPF, I drove this change and it was my fault for not realizing that it shouldn't be applied to existing target frameworks.