Block PackAsTool invalid TFMs by wli3 · Pull Request #2152 · dotnet/sdk

wli3 · 2018-04-18T05:52:14Z

https://github.com/dotnet/cli/issues/9073

wli3 · 2018-04-18T06:02:05Z

 {2} - Current version of platform package</comment>
  </data>
-</root>
+  <data name="PackAsToolOnlySupportNetcoreapp" xml:space="preserve">


Pending string review

@KathleenDollard for this 2 strings
"DotnetTool only supports .NET Core."
"DotnetTool does not support target framework lower than netcoreapp2.1."

What is "DotnetTool" in this message? Is this supposed to refer to the PackAsTool property?

Global tools feature officially is called DotnetTool, since it is the nuget package type. And Rob once asked in mail for official name, that is the one. However, it does not stick that well

wli3 · 2018-04-18T06:03:09Z

+    <NETSdkError Condition="'$(TargetFrameworkIdentifier)' != '.NETCoreApp'"
+                 ResourceName="PackAsToolOnlySupportNetcoreapp" />
+
+    <NETSdkError Condition="'$(_TargetFrameworkVersionWithoutV)' &lt; '2.1'"


Should I access _TargetFrameworkVersionWithoutV or calculate it again. It is created by SDK, but it is a previous process.

hard code 2.1 is fine? it is a hard number

Better to use 2.1.0 to be absolutely sure msbuild does not coerce to float instead of version.

As long as it's in one place only and we have near zero expectation of changing it again, I think hard-coding is actually fine and honestly more readable.

wli3 · 2018-04-18T06:03:34Z

+
+            // walk around attribute require static
+            string message = null;
+            if (error == "PackAsToolDoesNotSupportTFMLowerThanNetcoreapp21")


I don't know a better way to walk around it

wli3 · 2018-04-18T06:04:14Z

+        [InlineData("TargetFrameworks", "netcoreapp2.0;netcoreapp2.1", "PackAsToolDoesNotSupportTFMLowerThanNetcoreapp21")]
+        // non netcoreapp
+        [InlineData("TargetFramework", "net46", "PackAsToolOnlySupportNetcoreapp")]
+        [InlineData("TargetFramework", "netstandard2.0", "PackAsToolOnlySupportNetcoreapp")]


And I will explicit block full framework and netstarnard. They should not work in the beginning

wli3 · 2018-04-18T18:50:55Z

+
+            // walk around attribute requires static
+            string message = null;
+            if (error == "PackAsToolDoesNotSupportTFMLowerThanNetcoreapp21")


I cannot find a better way to walk around attribute need to be static

Can you use the same code that is generated for you in the properties? IE:

string message = Microsoft.NET.Build.Tasks.Strings.ResourceManager.GetString(error);

Also I'd rename error to something like expectedErrorResourceName and message to expectedErrorMessage.

Changed as suggested

wli3 · 2018-04-18T18:51:34Z

+        [FullMSBuildOnlyFact]
+        public void It_should_fail_with_error_message_on_fullframework()
+        {
+            It_should_fail_with_error_message("TargetFramework", "net46", "PackAsToolOnlySupportNetcoreapp");


I am blocking full framework. It should not work in the beginning

@KathleenDollard I am blocking full framework and netstandard at the same time.

Yes, let's notify on pack if it will fail later when we can.

I don't think I've seen a message on netstandard.

No, but for netstandard it is pretty much impossible to create one. There is no entry point for the program at all. It will fail later down the road, but in this case it can fail more explicitly with better error message.

dsplaisted

Looks good, although it looks like you still need to figure out how to do the version comparison correctly without having the versions be interpreted as floats. Also lets wait for #2157 if possible and then you can remove all the NuGetConfigWriter calls.

dsplaisted · 2018-04-18T22:50:56Z

 {2} - Current version of platform package</comment>
  </data>
-</root>
+  <data name="PackAsToolOnlySupportNetcoreapp" xml:space="preserve">


What is "DotnetTool" in this message? Is this supposed to refer to the PackAsTool property?

dsplaisted · 2018-04-18T22:51:44Z


    mimetype: application/x-microsoft.net.object.bytearray.base64
-    value   : The object must be serialized into a byte array
+    value   : The object must be serialized into a byte array 


Did you edit this file manually? Where are all the whitespace changes coming from? It would be good to revert them.

I edit by double click in VS. I don't know where they come from. But let me revert it

dsplaisted · 2018-04-18T22:55:28Z

-                .Restore(Log);
+                });
+
+            List<string> sources = new List<string>() { NuGetConfigWriter.DotnetCoreMyGetFeed };


I think this workaround will be unnecessary once we merge #2157

workaround removed

dsplaisted · 2018-04-18T23:00:02Z

+
+            // walk around attribute requires static
+            string message = null;
+            if (error == "PackAsToolDoesNotSupportTFMLowerThanNetcoreapp21")


Can you use the same code that is generated for you in the properties? IE:

string message = Microsoft.NET.Build.Tasks.Strings.ResourceManager.GetString(error);

Also I'd rename error to something like expectedErrorResourceName and message to expectedErrorMessage.

dsplaisted · 2018-04-18T23:01:06Z

+            result.StdOut.Should().Contain(message);
+        }
+
+        [FullMSBuildOnlyFact]


I think this should be WindowsOnlyFact instead of FullMSBuildOnlyFact.

will windows always has full framework? my understanding is fullmsbuild will have fullframework. And windows can have only dotnet core like nano

We don't run our tests on nano though. There are lots of other tests that assume that full framework is available on Windows.

If we want to support running the tests on Windows without the full framework (and full framework reference assemblies) installed, then we can make our Fact/Theory attributes more granular to represent this, but for now we should get the coverage on the .NET CLI as well as full MSBuild.

wli3 · 2018-04-19T05:28:53Z

+                 ResourceName="DotnetToolOnlySupportNetcoreapp" />
+
+    <!-- Compare version in System.Version type, if it is lower than 2.1 -->
+    <NETSdkError Condition='$([System.Version]::Parse("$(_TargetFrameworkVersionWithoutV)").CompareTo($([System.Version]::Parse("2.1")))) &lt; 0'


@nguerrera as we discussed. use msbuild compare may cause problem on 2.11 vs 2.2. This looks ugly but it works. @rainersigwald is it supported? I cannot find a direct document about it.

Also $([System.Version]::Parse("2.1")) seems keep the type. If I just replace it with string 2.1 it will error and ask for Version type.

The alternative is to write a Task. It is a lot of code and also it might not be cleaner than that.

Reverted to the original

Only netcoreapp2.1 and up is valid. Everything else will fail later down the road.

wli3 commented Apr 18, 2018

View reviewed changes

wli3 changed the title ~~[WPI] [QUESTIONS] Block tfm lower21~~ [WIP] [QUESTIONS] Block tfm lower21 Apr 18, 2018

wli3 requested review from a team, dsplaisted, nguerrera and peterhuene April 18, 2018 18:40

wli3 commented Apr 18, 2018

View reviewed changes

wli3 changed the title ~~[WIP] [QUESTIONS] Block tfm lower21~~ Block tfm lower21 Apr 18, 2018

wli3 changed the title ~~Block tfm lower21~~ Block PackAsTool invalid TFMs Apr 18, 2018

dsplaisted approved these changes Apr 18, 2018

View reviewed changes

wli3 commented Apr 19, 2018

View reviewed changes

wli3 changed the title ~~Block PackAsTool invalid TFMs~~ WIP Block PackAsTool invalid TFMs Apr 19, 2018

wli3 force-pushed the block-tfm-lower21 branch 2 times, most recently from dd64791 to dc544da Compare April 19, 2018 18:28

Block the invalid TFM in PackAsTool

3db9471

Only netcoreapp2.1 and up is valid. Everything else will fail later down the road.

wli3 force-pushed the block-tfm-lower21 branch from dc544da to 3db9471 Compare April 19, 2018 18:41

wli3 changed the title ~~WIP Block PackAsTool invalid TFMs~~ Block PackAsTool invalid TFMs Apr 19, 2018

wli3 added the Pending M2 Approval label Apr 19, 2018

MattGertz added Pending Shiproom Approval and removed Pending M2 Approval labels Apr 19, 2018

livarcocc added Approved to merge and removed Pending Shiproom Approval labels Apr 19, 2018

wli3 merged commit a7f41d6 into dotnet:release/2.1.3xx Apr 19, 2018

wli3 deleted the block-tfm-lower21 branch April 19, 2018 19:55

antonioaversa mentioned this pull request Nov 27, 2023

Use auto generation for DotnetToolSettings.xml SonarSource/sonar-scanner-msbuild#1764

Closed

Conversation

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

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wli3 Apr 18, 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!

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 Apr 18, 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!

dsplaisted left a comment

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

wli3 commented Apr 18, 2018 •

edited

Loading

wli3 Apr 18, 2018 •

edited

Loading

wli3 Apr 18, 2018 •

edited

Loading