ApiCheck: Print out breaking changes to be added as exceptions #238

smitpatel · 2017-04-21T01:07:50Z

Remove concept of modification
Print out breaking changes to be added as exceptions
Change format of exception files
Use word breaking changes instead of exceptions/exclusions
Upgrade to net46
Remove compact output option
Make netframework & netcore specific baseline/breaking change.

Sample output:

C:\Users\smpatel\.nuget\packages\internal.aspnetcore.sdk\2.0.0-rc2-t00459e38b\build\ApiCheck.targets(19,5): error : ERROR: Verifying breaking changes for framework net451 failed. [D:\Universe\.r\Microsoft.Data.Sqlite\src\Microsoft.Data.Sqlite.Core\Microsoft.Data.Sqlite.Core.csproj]


  The following types/interfaces have one or more members removed from them.

      public class Microsoft.Data.Sqlite.SqliteDataReader : System.Data.Common.DbDataReader
          public override System.Data.DataTable GetSchemaTable()

      public class Microsoft.Data.Sqlite.SqliteParameter : System.Data.Common.DbParameter
          public override System.Void set_SourceVersion(System.Data.DataRowVersion value)

C:\Users\smpatel\.nuget\packages\internal.aspnetcore.sdk\2.0.0-rc2-t00459e38b\build\ApiCheck.targets(19,5): error : ERROR: The following exclusion is in the exclusion file, but is no longer necessary: [D:\Universe\.r\Microsoft.Data.Sqlite\src\Microsoft.Data.Sqlite.Core\Microsoft.Data.Sqlite.Core.csproj]
  {
    "TypeId": "public class Microsoft.Data.Sqlite.SqliteParameter : System.Data.Common.DbParameter",
    "MemberId": "public override System.Void setfsdf_SourceVersion(System.Data.DataRowVersion value)",
    "Kind": "Removal"
  }

C:\Users\smpatel\.nuget\packages\internal.aspnetcore.sdk\2.0.0-rc2-t00459e38b\build\ApiCheck.targets(19,5): error : ERROR: The following exclusion is in the exclusion file, but is no longer necessary: [D:\Universe\.r\Microsoft.Data.Sqlite\src\Microsoft.Data.Sqlite.Core\Microsoft.Data.Sqlite.Core.csproj]
  {
    "TypeId": "public class Microsoft.Data.Sqlite.SqliteDataReader : System.Data.Common.DbDataReader",
    "MemberId": "public override System.Data.DatfsdaTable GetSchemaTable()",
    "Kind": "Removal"
  }



  Following is the list of exclusions that either need to be added to the list of breaking changes, or the breaking changes themselves need to be reverted:
  [
    {
      "TypeId": "public class Microsoft.Data.Sqlite.SqliteDataReader : System.Data.Common.DbDataReader",
      "MemberId": "public override System.Data.DataTable GetSchemaTable()",
      "Kind": "Removal"
    },
    {
      "TypeId": "public class Microsoft.Data.Sqlite.SqliteParameter : System.Data.Common.DbParameter",
      "MemberId": "public override System.Void set_SourceVersion(System.Data.DataRowVersion value)",
      "Kind": "Removal"
    }
  ]

  The process for breaking changes is described in: https://github.com/aspnet/Home/wiki/Engineering-guidelines#breaking-changes
  The process to add an exclusion to this tool is described in: https://github.com/aspnet/BuildTools/wiki/Api-Check#apicheck-exceptions

Build FAILED.

TODO: update wiki files.

Reaction PRs:

dnfclas · 2017-04-21T01:07:55Z

@smitpatel,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

smitpatel · 2017-04-21T01:12:31Z

src/Internal.AspNetCore.Sdk/build/ApiCheck.targets

      <_ApiListingFileSuffix Condition=" '$(_ApiListingFileSuffix)' == '' ">netcore.json</_ApiListingFileSuffix>
      <_ApiListingFilePath>$(MSBuildProjectDirectory)\baseline.$(_ApiListingFileSuffix)</_ApiListingFilePath>
-      <_ApiExclusionsFilePath>$(MSBuildProjectDirectory)\exceptions.$(_ApiListingFileSuffix)</_ApiExclusionsFilePath>
+      <_ApiExclusionsFilePath>$(MSBuildProjectDirectory)\breakingchanges.$(_ApiListingFileSuffix)</_ApiExclusionsFilePath>


@Eilon - Though not necessary, we should change the name of exception files. We call it "exception" file at the same time we call them "exclusion" in ApiCheck source code. They are neither exception/exclusion, more like known breaking changes.

Ah this sounds reasonable to me.

smitpatel · 2017-04-21T01:13:34Z

src/Microsoft.AspNetCore.BuildTools.ApiCheck/ApiListingComparer.cs

        }

-        public ApiComparisonResult GetDifferences()
+        public IList<BreakingChange> GetDifferences()


The sole task or ApiListingComparer is to find differences between 2 ApiListing, removing known breaking changes should not be part of its tasks.

@Eilon I asked @javiercn about the errors output for extraneous exceptions. He said you had wanted that feature and considered the extra exceptions errors. IIRC the purpose was to make sure we'd reported all real breaking changes at the end of a milestone. Has something changed?

Never mind, I now see the feature remains. Only change is that this class isn't involved.

smitpatel · 2017-04-21T01:14:18Z

src/Microsoft.AspNetCore.BuildTools.ApiCheck/ApiListingComparer.cs

-                {
-                    breakingChanges.Add(new BreakingChange(type, "New members were added to the following interface"));
-                }
+                breakingChanges.AddRange(newMembers.Select(member => new BreakingChange(newType.Id, member.Id, ChangeKind.Addition)));


This part finds actually newly added members on interface (or renamed ones because they are remove/add)

smitpatel · 2017-04-21T01:15:00Z

src/Microsoft.AspNetCore.BuildTools.ApiCheck/Microsoft.AspNetCore.BuildTools.ApiCheck.csproj


  <PropertyGroup>
-    <VersionPrefix>1.0.2</VersionPrefix>
+    <VersionPrefix>1.0.3</VersionPrefix>


@pranavkm - This version to bump?

Eilon

Looks very nice! Just a few misc comments.

Eilon · 2017-04-22T00:54:37Z

src/Internal.AspNetCore.Sdk/build/ApiCheck.targets

      <_ApiListingFileSuffix Condition=" '$(_ApiListingFileSuffix)' == '' ">netcore.json</_ApiListingFileSuffix>
      <_ApiListingFilePath>$(MSBuildProjectDirectory)\baseline.$(_ApiListingFileSuffix)</_ApiListingFilePath>
-      <_ApiExclusionsFilePath>$(MSBuildProjectDirectory)\exceptions.$(_ApiListingFileSuffix)</_ApiExclusionsFilePath>
+      <_ApiExclusionsFilePath>$(MSBuildProjectDirectory)\breakingchanges.$(_ApiListingFileSuffix)</_ApiExclusionsFilePath>


Ah this sounds reasonable to me.

Eilon · 2017-04-22T00:55:54Z

src/Microsoft.AspNetCore.BuildTools.ApiCheck/ApiListingComparer.cs

-                        {
-                            breakingChanges.Add(typeChange);
-                        }
+                        breakingChanges.Add(new BreakingChange(type.Id, /*memberId:*/ null, ChangeKind.Removal));


Can still use a named param! And then use a named param for the last param too.

Sure. in EF we use commented out if its in middle and non-commented if its last. I will change this.

Eilon · 2017-04-22T01:10:21Z

src/Microsoft.AspNetCore.BuildTools.ApiCheck/BreakingChange.cs

+        {
+            unchecked
+            {
+                var hashCode = TypeId.GetHashCode();


Perhaps use https://github.com/aspnet/Common/tree/dev/shared/Microsoft.Extensions.HashCodeCombiner.Sources instead so we don't have random/arbitrary hash code combiners scattered in teh codez.

Eilon · 2017-04-22T01:10:34Z

src/Microsoft.AspNetCore.BuildTools.ApiCheck/ChangeKind.cs

    public enum ChangeKind
    {
        Removal,
-        Modification,


Eilon · 2017-04-22T01:11:28Z

src/Microsoft.AspNetCore.BuildTools.ApiCheck/Program.cs

        private const int Error = 1;

-        public static int Main(string[] args)
+        public static int Main(string [] args)


Eilon · 2017-04-22T01:12:48Z

src/Microsoft.AspNetCore.BuildTools.ApiCheck/Program.cs

+                {
+                    Console.WriteLine();
+                    Console.WriteLine();
+                    Console.WriteLine("Error: Following types/interfaces have one or more members removed from them.");


The following ...

Eilon · 2017-04-22T01:13:10Z

src/Microsoft.AspNetCore.BuildTools.ApiCheck/Program.cs

+                {
+                    Console.WriteLine();
+                    Console.WriteLine();
+                    Console.WriteLine("Error: Following interfaces have one or more members added from them.");


The following...

... added *to* them.

Eilon · 2017-04-22T01:20:04Z

src/Microsoft.AspNetCore.BuildTools.ApiCheck/Program.cs

+            {
+                Console.WriteLine();
+                Console.WriteLine();
+                Console.WriteLine("Following is the list of exclusions which are needed to be added.");


Following is the list of exclusions that either need to be added to the list of breaking changes, or the breaking changes themselves need to be reverted:

Eilon · 2017-04-22T01:20:57Z

src/Microsoft.AspNetCore.BuildTools.ApiCheck/Program.cs

+                    Console.WriteLine(
+                        "The process for breaking changes is described in: https://github.com/aspnet/Home/wiki/Engineering-guidelines#breaking-changes");
+                    Console.WriteLine(
+                        "For how to add an exclusion to Api-check go to: https://github.com/aspnet/BuildTools/wiki/Api-Check#apicheck-exceptions");


The process to add an exclusion to this tool is described in: ...

Eilon · 2017-04-22T01:21:51Z

test/ApiCheck.Test/ApiListingComparerTests.cs

-            var change = Assert.Single(changes.BreakingChanges, bc => bc.Item.Id == "public class ComparisonScenarios.PublicToInternalClass");
+            var expected = new BreakingChange(
+                "public class ComparisonScenarios.PublicToInternalClass",
+                /*memberId:*/ null,


Should use named params instead of comments. See my earlier comment.

smitpatel · 2017-04-24T17:46:38Z

Updated.

smitpatel · 2017-04-24T20:15:00Z

Removed compact output

smitpatel · 2017-04-24T20:21:40Z

src/Internal.AspNetCore.Sdk/build/ApiCheck.targets

  <Target Name="ApiCheck" Condition=" '$(EnableApiCheck)' == 'true' ">
    <PropertyGroup>
-      <_ApiListingFileSuffix Condition=" '$(TargetFrameworkIdentifier)' == '.NETFramework' ">net45.json</_ApiListingFileSuffix>
+      <_ApiListingFileSuffix Condition=" '$(TargetFrameworkIdentifier)' == '.NETFramework' ">net46.json</_ApiListingFileSuffix>


@Eilon - This one is bit interesting.
for .NETFramework tfm we used to use net45.json which is inconsistent now because we upgraded to net46. Though all our baselines say, net45 only. On the other side of world (in netcore/standard) anything non-net framework we used netcore (without exact version) as suffix. Should we change this one to something other than net46? Like netframework & netcore baselines?

React to aspnet/BuildTools#238

Eilon · 2017-04-25T04:42:24Z

src/Internal.AspNetCore.Sdk/build/ApiCheck.targets

  <Target Name="ApiCheck" Condition=" '$(EnableApiCheck)' == 'true' ">
    <PropertyGroup>
-      <_ApiListingFileSuffix Condition=" '$(TargetFrameworkIdentifier)' == '.NETFramework' ">net45.json</_ApiListingFileSuffix>
+      <_ApiListingFileSuffix Condition=" '$(TargetFrameworkIdentifier)' == '.NETFramework' ">netframework.json</_ApiListingFileSuffix>


Eilon

Just some teeny-tiny comments, then !

Eilon · 2017-04-25T04:43:17Z

src/Microsoft.AspNetCore.BuildTools.ApiCheck.Task/ApiCheckTask.cs

        {
-            if (string.IsNullOrEmpty(singleLine) || singleLine.StartsWith("  ", StringComparison.Ordinal))
+            // Since tool prints out formatted list of breaking changes,
+            // anything that starts with Error will be considered error rest is user information.


Change comment: ... considered an error; the rest is ...

Eilon · 2017-04-25T04:47:09Z

src/Microsoft.AspNetCore.BuildTools.ApiCheck/Program.cs

+            if (removedTypes.Count > 0)
+            {
+                Console.WriteLine();
+                Console.WriteLine("The following types/interfaces have been removed.");


BTW types/interfaces is redundant. All interfaces are types. Can clear this up in other messages below as well.

Print out breaking changes to be added as exceptions Change format of exception files Use word breaking changes instead of exceptions/exclusions Upgrade to net46 Remove unused option of compact output.

React to aspnet/BuildTools#238

dougbu · 2017-04-25T17:50:10Z

src/Microsoft.AspNetCore.BuildTools.ApiCheck/Program.cs

+                        {
+                            NullValueHandling = NullValueHandling.Ignore
+                        }));
+                Console.WriteLine();


@smitpatel why write anything but this block and the bit about no-longer-necessary exclusions? Everything else written to the console in this method is redundant.

Yes, it is intentionally redundant.
The main task of tool is to inform the developer about breaking changes so that it can be determined if the breaking change is desired or was by mistake. We print out the list of breaking changes in a human readable format first.
The rest of parts, no-longer-necessary exceptions & json-to-add is to help user update the breaking changes file. Those JSON are not so easily readable hence redundancy.

dnfclas added the cla-already-signed label Apr 21, 2017

smitpatel force-pushed the exceptionPrettyPrinter branch 3 times, most recently from c581f07 to a7afff3 Compare April 21, 2017 01:11

smitpatel commented Apr 21, 2017

View reviewed changes

smitpatel requested a review from Eilon April 21, 2017 01:48

Eilon requested a review from javiercn April 22, 2017 00:54

Eilon suggested changes Apr 22, 2017

View reviewed changes

smitpatel force-pushed the exceptionPrettyPrinter branch from a7afff3 to aff63c0 Compare April 24, 2017 17:44

smitpatel force-pushed the exceptionPrettyPrinter branch 2 times, most recently from dca2620 to ce7960e Compare April 24, 2017 20:14

smitpatel commented Apr 24, 2017

View reviewed changes

smitpatel force-pushed the exceptionPrettyPrinter branch from ce7960e to f198dcb Compare April 24, 2017 21:38

smitpatel added a commit to aspnet/Microsoft.Data.Sqlite that referenced this pull request Apr 24, 2017

Update API Check related files

22b22ca

React to aspnet/BuildTools#238

smitpatel mentioned this pull request Apr 24, 2017

Update API Check related files aspnet/Microsoft.Data.Sqlite#359

Merged

smitpatel force-pushed the exceptionPrettyPrinter branch from f198dcb to 0e914ea Compare April 24, 2017 21:46

smitpatel added a commit to aspnet/Microsoft.Data.Sqlite that referenced this pull request Apr 24, 2017

Update API Check related files

eee8cfd

React to aspnet/BuildTools#238

smitpatel added a commit to aspnet/DependencyInjection that referenced this pull request Apr 24, 2017

Update API Check related files

d36f4af

React to aspnet/BuildTools#238

smitpatel mentioned this pull request Apr 24, 2017

Update API Check related files aspnet/DependencyInjection#519

Merged

smitpatel added a commit to aspnet/FileSystem that referenced this pull request Apr 24, 2017

Update API Check related files

c76cc36

React to aspnet/BuildTools#238

smitpatel mentioned this pull request Apr 24, 2017

Update API Check related files aspnet/FileSystem#271

Merged

smitpatel added a commit to aspnet/Configuration that referenced this pull request Apr 24, 2017

Update API Check related files

bd22a8e

React to aspnet/BuildTools#238

smitpatel mentioned this pull request Apr 24, 2017

Update API Check related files aspnet/Configuration#653

Merged

smitpatel added a commit to aspnet/Logging that referenced this pull request Apr 24, 2017

Update API Check related files

46b60bf

React to aspnet/BuildTools#238

Eilon reviewed Apr 25, 2017

View reviewed changes

Eilon approved these changes Apr 25, 2017

View reviewed changes

ApiCheck: Remove concept of modification

7e7ac94

Print out breaking changes to be added as exceptions Change format of exception files Use word breaking changes instead of exceptions/exclusions Upgrade to net46 Remove unused option of compact output.

smitpatel force-pushed the exceptionPrettyPrinter branch from b89e192 to 7e7ac94 Compare April 25, 2017 13:51

smitpatel added a commit to aspnet/Configuration that referenced this pull request Apr 25, 2017

Update API Check related files

de66962

React to aspnet/BuildTools#238

smitpatel added a commit to dotnet/efcore that referenced this pull request Apr 25, 2017

Update API Check related files

177d5fe

React to aspnet/BuildTools#238

smitpatel added a commit to aspnet/Hosting that referenced this pull request Apr 25, 2017

Update API Check related files

b13a70d

React to aspnet/BuildTools#238

smitpatel added a commit to aspnet/HttpAbstractions that referenced this pull request Apr 25, 2017

Update API Check related files

38551fa

React to aspnet/BuildTools#238

smitpatel added a commit to aspnet/Mvc that referenced this pull request Apr 25, 2017

Update API Check related files

ecc471c

React to aspnet/BuildTools#238

smitpatel added a commit to aspnet/Mvc that referenced this pull request Apr 25, 2017

Update API Check related files

c533fd7

React to aspnet/BuildTools#238

smitpatel added a commit to aspnet/Hosting that referenced this pull request Apr 25, 2017

Update API Check related files

0c0dcba

React to aspnet/BuildTools#238

smitpatel merged commit 7e7ac94 into dev Apr 25, 2017

smitpatel deleted the exceptionPrettyPrinter branch April 25, 2017 15:02

smitpatel added a commit to aspnet/Antiforgery that referenced this pull request Apr 25, 2017

Update API Check related files

bb916af

React to aspnet/BuildTools#238

smitpatel added a commit to aspnet/Caching that referenced this pull request Apr 25, 2017

Update API Check related files

8f5a489

React to aspnet/BuildTools#238

smitpatel added a commit to aspnet/Configuration that referenced this pull request Apr 25, 2017

Update API Check related files

cd3c0cf

React to aspnet/BuildTools#238

smitpatel added a commit to aspnet/DependencyInjection that referenced this pull request Apr 25, 2017

Update API Check related files

93206e9

React to aspnet/BuildTools#238

smitpatel added a commit to aspnet/Diagnostics that referenced this pull request Apr 25, 2017

Update API Check related files

8c24094

React to aspnet/BuildTools#238

smitpatel added a commit to dotnet/efcore that referenced this pull request Apr 25, 2017

Update API Check related files

2ee2fb5

React to aspnet/BuildTools#238

smitpatel added a commit to aspnet/FileSystem that referenced this pull request Apr 25, 2017

Update API Check related files

839e526

React to aspnet/BuildTools#238

smitpatel added a commit to aspnet/Hosting that referenced this pull request Apr 25, 2017

Update API Check related files

a7221ca

React to aspnet/BuildTools#238

smitpatel added a commit to aspnet/HttpAbstractions that referenced this pull request Apr 25, 2017

Update API Check related files

73e4441

React to aspnet/BuildTools#238

smitpatel added a commit to aspnet/Identity that referenced this pull request Apr 25, 2017

Update API Check related files

660525a

React to aspnet/BuildTools#238

smitpatel added a commit to aspnet/Logging that referenced this pull request Apr 25, 2017

Update API Check related files

bd235d6

React to aspnet/BuildTools#238

smitpatel added a commit to aspnet/Microsoft.Data.Sqlite that referenced this pull request Apr 25, 2017

Update API Check related files

dcf74ab

React to aspnet/BuildTools#238

smitpatel added a commit to aspnet/Mvc that referenced this pull request Apr 25, 2017

Update API Check related files

bd39d36

React to aspnet/BuildTools#238

smitpatel added a commit to aspnet/Routing that referenced this pull request Apr 25, 2017

Update API Check related files

0aebdb4

React to aspnet/BuildTools#238

smitpatel added a commit to aspnet/Security that referenced this pull request Apr 25, 2017

Update API Check related files

254eb82

React to aspnet/BuildTools#238

smitpatel added a commit to aspnet/DataProtection that referenced this pull request Apr 25, 2017

Update API Check related files

edd7386

React to aspnet/BuildTools#238

dougbu reviewed Apr 25, 2017

View reviewed changes

ApiCheck: Print out breaking changes to be added as exceptions #238

ApiCheck: Print out breaking changes to be added as exceptions #238

Uh oh!

Conversation

smitpatel commented Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfclas commented Apr 21, 2017

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!

Eilon 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!

Choose a reason for hiding this comment

Uh oh!

smitpatel commented Apr 24, 2017

Uh oh!

smitpatel commented Apr 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eilon 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

smitpatel commented Apr 21, 2017 •

edited

Loading