-
Notifications
You must be signed in to change notification settings - Fork 389
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
Correctly show supported frameworks for .NETCore WPF and WinForms projects #5011
Conversation
For Each supportedFramework As TargetFrameworkMoniker In supportedFrameworks | ||
Dim frameworkName As New FrameworkName(supportedFramework.Moniker) | ||
If isWPForWindowsForms And frameworkName.Version.Major < 3 And String.Equals(frameworkName.Identifier, ".NETCoreApp") Then |
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 don't know if the framework identifier is case sensitive, but its worth finding out and passing in the right StringComparer.
Also, using AndAlso
instead of And
will short circuit and be infinitesimally faster :)
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.
@nguerrera @dsplaisted Can we please introduce handshake between us and the SDK so that we don't have business logic in the UI layer?
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.
Happy to work on the right handshake, but we need to understand what information the project system will need. FWIW, I can retarget a .NET Framework WPF app to .NET Framework 2.0 in Visual Studio, so the equivalent check doesn't exist for .NET Framework and I'm not sure that we necessarily need it for .NET Core.
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.
Actually, .NET Framework 2.0 does appear in the dropdown, but when you try to retarget it fails with the following dialog:
Attempted re-targeting of the project has been canceled. Required assemblies 'WindowsBase', 'PresentationCore', and 'PresentationFramework' are missing from the target framework.
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.
The listis populated from items in the sdk, right. Should we exclude/remove as appropriate in sdk?
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.
That check from legacy only runs in WPF flavor, but uses IVsFrameworkMultiTargeting to figure if those assemblies are present in the target framework. We need a data driven approach to avoid hardcoding these specific target frameworkism's in the project-system/UI layer, ie "Does this FrameworkReference exist in this TFM?"
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.
Another approach, simialr to what we did we project references, is give up UI validation at all, and just have the build tell you that these references cannot be referenced in the target. I like that approach because it works everywhere regardless of how you edit the project.
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.
From a light investigation it seems that the supported TFM's are added here but I am not sure it is possible to determine if it is a WPF or WinForms project at that time, however
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.
Created an issue here dotnet/sdk#3438 to discuss this further
Closed in dotnet/sdk#3438 |
Fix for: #5010