Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Kryptos-FR opened this issue Dec 11, 2020 · 12 comments · Fixed by #21959
Closed

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

Kryptos-FR opened this issue Dec 11, 2020 · 12 comments · Fixed by #21959
Assignees
Labels
doc-enhancement Improve the current content [org][type][category] dotnet-fundamentals/svc Pri1 High priority, do before Pri2 and Pri3

Comments

@Kryptos-FR
Copy link
Contributor

Kryptos-FR commented Dec 11, 2020

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: dotnet/sdk#13331


Document Details

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

@dotnet-bot dotnet-bot added the ⌚ Not Triaged Not triaged label Dec 11, 2020
@PRMerger14 PRMerger14 added dotnet-fundamentals/svc Pri1 High priority, do before Pri2 and Pri3 labels Dec 11, 2020
@Kryptos-FR Kryptos-FR changed the title Misleading page : change affects all version of .NET, not just .NET 5 Misleading page : change affects all versions of .NET, not just .NET 5 Dec 11, 2020
@gewarren gewarren added doc-enhancement Improve the current content [org][type][category] and removed ⌚ Not Triaged Not triaged labels Dec 11, 2020
@gewarren gewarren self-assigned this Dec 11, 2020
@dsplaisted
Copy link
Member

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
Copy link

@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
Copy link

vuplea commented Dec 15, 2020

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
Copy link
Contributor

@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
Copy link
Member

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
Copy link
Member

RussKie commented Feb 15, 2021

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 dotnet/sdk#12448 (doc'ed in #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
Copy link

NikolaosWakem commented Mar 1, 2021

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
Copy link
Member

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

@NikolaosWakem
Copy link

NikolaosWakem commented Mar 3, 2021

@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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

dsplaisted commented Mar 4, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-enhancement Improve the current content [org][type][category] dotnet-fundamentals/svc Pri1 High priority, do before Pri2 and Pri3
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants