Add warning for ref to netstandard >= 1.5 from netfx without built-in support #2330

wli3 · 2018-06-13T17:40:18Z

Still TODO:

Confirm final error message text with PMs
Make sure fwlink points to valid content
Use aka.ms link which owned by us. We could change the destination later. It is pointing to microsoft.com
Tests
- The existing tests of the extensions adding support files probably just need to be updated to also check for the correct warning or lack thereof. The matrix is the same as the cases where we do/do not add support files:
- .NETFramework < 4.7.2 -> .NETStandard >= 1.5 --> warning
- .NETFramework any -> .NETStandard < 1.5 --> no warning
- .NETFramework >= 4.7.2 -> .NETStandard any --> no warning
- Warning can be suppressed

NOTE: Error code may still change as this will conflict with Implement framework-dependent application with apphost. #2282

… support

wli3 · 2018-06-13T17:42:53Z

@dsplaisted Only "Warning can be suppressed" is pending. Could you have a look if the test matrix is enough?

dsplaisted · 2018-06-13T21:29:25Z

@terrajobst @weshaggard @AlexGhiondea can you provide the link to the page with the known issues for .NET Standard 2.0 (and 1.5+) on .NET Framework before 4.7.2? We want to point the aka.ms link in this warning at that page for now.

dsplaisted · 2018-06-13T21:33:11Z

src/Tasks/Common/Resources/Strings.resx

    <comment>{StrBegin="NETSDK1068: "}</comment>
  </data>
+  <data name="NETFrameworkToNonBuiltInNETStandard" xml:space="preserve">
+    <value>NETSDK1069: This project uses a library that targets .NET Standard 1.5 or higher and targets a version of .NET Framework that doesn't have built-in support for that version of .NET Standard. Visit https://aka.ms/net-standard-known-issues for a set of known issues. Consider retargeting to .NET Framework 4.7.2.</value>


Reading this message again it's not clear that "and targets a version of .NET Framework" refers to the project and not the library. So how about this:

This project uses a library that targets .NET Standard 1.5 or higher, and the project targets a version of .NET Framework that doesn't have built in support for that version of .NET Standard. Visit https://aka.ms/net-standard-known-issues for a set of known issues. Consider retargeting to .NET Framework 4.7.2.

Agree, changed to it

dsplaisted · 2018-06-13T21:47:46Z

src/Tests/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildADesktopExeWtihNetStandardLib.cs

+        public void It_generate_warning_depends_on_framework_and_library_netstandard_version(string framework, string libraryNetstandard, bool shouldHaveWarning, bool isSdk, string description)
+        {
+            var testAsset = _testAssetsManager
+                .CopyTestAsset(GetTemplateName(isSdk), identifier: isSdk.ToString())


The identifier should be unique for each test case within a theory. So here I think you'd want to combine isSdk with description.

I see, although description is not unique. I will use framework + libraryNetstandard + isSdk.ToString() + description

dsplaisted · 2018-06-13T21:50:12Z

src/Tests/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildADesktopExeWtihNetStandardLib.cs

+                        {
+                            project.Root.Element(ns + "PropertyGroup")
+                                        .Element(ns + "TargetFramework")
+                                        .Value = parsedFramework.GetShortFolderName();


Nitpicking here, but I think in this case you can just use framework directly, instead of parsing it into a NuGet framework and then getting the short version of it.

dsplaisted · 2018-06-13T22:00:09Z

src/Tests/Microsoft.NET.TestFramework/Assertions/CommandResultAssertions.cs

+        public AndConstraint<CommandResultAssertions> HaveStdOutContaining(Func<string, bool> predicate, string description = "")
+        {
+            Execute.Assertion.ForCondition(predicate(_commandResult.StdOut))
+                .FailWith(AppendDiagnosticsTo($"The command output did not contain expected result: {predicate.Method.Name} {description} {Environment.NewLine}"));


I think we should not use predicate.Method.Name here. It's not generally expected behavior to use the method name of a func you pass into a function, and if you use a lambda or anonymous delegate (perfectly reasonable to do), then this would be a compiler-generated method name.

fixed, changed to just explicitly pass the description.

livarcocc · 2018-06-13T23:42:27Z

FYI @Pilchie @tmeschter

Pilchie · 2018-06-14T00:22:26Z

Thanks @wli3 @nguerrera @livarcocc !

Quppa · 2018-07-11T00:59:28Z

I'm seeing this warning in VS 15.8.0 Preview 4, but the link (https://aka.ms/net-standard-known-issues) doesn't appear to be live - I just get redirected to the Microsoft homepage.

Is it meant to point to dotnet/standard#567, and if so, isn't this only a problem for .NET 4.6.1 projects running on .NET 4.7.1? All our full framework projects already target .NET 4.7.1, so is this warning still valid?

AlexGhiondea · 2018-07-11T04:44:35Z

Hey @Quppa -- you are correct, the link is not yet live. We are still working on publishing the document and will update the link once it is ready. We are continuing to work on the experience for when we should show the warning.

The issue you are referring to contains a few of the problems you might run into when using .NET Standard on .NET Framework 4.7.1.
However, it is not easy to understand what all the issues are and in what state they are (ie. fixed, with workaround, etc).

We are working on a document that puts all that information in a single place to make it easier to view the issues and their possible resolution -- this is where the link will point to.

NickCraver · 2018-07-11T23:23:37Z

This adds a lot of needless warning noise to our libraries. How do you disable this warning? We are using APIs that are supported by net462 that we're targeting. In every single case I'm seeing this, it's an invalid warning. Please, disable this by default.

.NET 4.7.2 just landed on Windows Update literally yesterday. Targeting it for a library is extremely unreasonable for most people at this point in time. This should not be a warning. Warnings should be actionable things.

Even if it only warns on executables, requiring everyone to have .NET 4.7.2 to run a test suite is equally unreasonable.

NickCraver · 2018-07-11T23:32:31Z

From @nguerrera, here's how to disable this for anyone else ending up here before Preview 5:

<MSBuildWarningsAsMessages>NETSDK1069</MSBuildWarningsAsMessages>

dsplaisted · 2018-07-11T23:32:50Z

@NickCraver Yes, in #2390 we've backed out this warning. It won't be present in the next Preview release of VS 15.8. We plan to bring it back (and add documentation explaining known issues in more detail) after .NET 4.7.2 is more widely deployed.

See dotnet/sdk#2330 for details.

camainc · 2018-07-13T17:19:41Z

Where does this go in the project file?

<MSBuildWarningsAsMessages>NETSDK1069</MSBuildWarningsAsMessages>

nguerrera · 2018-07-13T17:23:08Z

In an PropertyGroup with no conditions. You probably already have one of those that you can add it to.

<PropertyGroup>
  <MSBuildWarningsAsMessages>NETSDK1069</MSBuildWarningsAsMessages>
</PropertyGroup>

nguerrera and others added 5 commits June 13, 2018 10:31

Add warning for ref to netstandard >= 1.5 from netfx without built-in…

21a3c3b

… support

Add test matrix for warning

d2e562b

Resolve merge conflict

faef6a1

Add loc

5a44fed

Format

6abdd91

wli3 mentioned this pull request Jun 13, 2018

[WIP] Add warning for ref to netstandard >= 1.5 from netfx without built-in support #2316

Closed

7 tasks

wli3 requested review from dsplaisted and livarcocc June 13, 2018 17:42

Add test for suppress warning

d299fc4

wli3 changed the title ~~WIP Add warning for ref to netstandard >= 1.5 from netfx without built-in support~~ Add warning for ref to netstandard >= 1.5 from netfx without built-in support Jun 13, 2018

dsplaisted approved these changes Jun 13, 2018

View reviewed changes

William Li added 2 commits June 13, 2018 15:21

Tweak the message

fa452af

Address code review

37fb166

livarcocc added this to the 2.1.4xx milestone Jun 13, 2018

livarcocc added the Approved to merge label Jun 13, 2018

wli3 merged commit 78b4b0b into dotnet:release/2.1.4xx Jun 14, 2018

wli3 deleted the netstandard-warning2 branch June 14, 2018 00:40

NickCraver added a commit to StackExchange/StackExchange.Redis that referenced this pull request Jul 12, 2018

Disable 15.8 Preview 4 build warnings

b34c858

See dotnet/sdk#2330 for details.

Add warning for ref to netstandard >= 1.5 from netfx without built-in support #2330

Add warning for ref to netstandard >= 1.5 from netfx without built-in support #2330

Uh oh!

Conversation

wli3 commented Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wli3 commented Jun 13, 2018

Uh oh!

dsplaisted commented Jun 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wli3 Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

livarcocc commented Jun 13, 2018

Uh oh!

Pilchie commented Jun 14, 2018

Uh oh!

Quppa commented Jul 11, 2018

Uh oh!

AlexGhiondea commented Jul 11, 2018

Uh oh!

NickCraver commented Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NickCraver commented Jul 11, 2018

Uh oh!

dsplaisted commented Jul 11, 2018

Uh oh!

camainc commented Jul 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nguerrera commented Jul 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

wli3 commented Jun 13, 2018 •

edited

Loading

wli3 Jun 13, 2018 •

edited

Loading

NickCraver commented Jul 11, 2018 •

edited

Loading

camainc commented Jul 13, 2018 •

edited

Loading