Skip to content

Conversation

ViktorHofer
Copy link
Member

Backport of d0a4bf1 and 6fad359

Customer Impact

Customers use APICompat / PackageValidation to validate their public API changes. While we already have a large set of compatibility rules, we were missing two major cases: a) member type validation (i.e. public string a -> public bool a which is a breaking change) and b) the "this" modifier for extension methods (public static void ExtMethod(this string a) -> public static void ExtMethod(string a) which also is a breaking change).

By taking this change, we increase our customer's confidence in our shipping tooling by validating these two common scenarios. This was reported by a customer and by dotnet/runtime folks.

Testing

Tests were added for both scenarios and they are running in CI. I also tested the changes in dotnet/runtime manually and its main branch already uses an APICompat version with these changes.

Risk

Low to Medium. Existing scenarios are validated by tests. In general, new validation in APICompat is considered a breaking change as customers need to react to those. But that's exactly the reason why customers use APICompat as they want their API changes to be validated and APICompat to error out for incompatible changes.

@ViktorHofer ViktorHofer self-assigned this Aug 29, 2023
@ghost ghost added the untriaged Request triage from a team member label Aug 29, 2023
@ViktorHofer ViktorHofer added Servicing-consider and removed untriaged Request triage from a team member labels Aug 29, 2023
Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, and I support getting this in for RC2. We should put this out for Tactics approval to at least ensure everyone's informed this is coming.

@ViktorHofer Aside from Tactics visibility, what breaking change notifications do we typically broadcast for new APICompat validations?

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Aug 29, 2023

Looks good to me, and I support getting this in for RC2. We should put this out for Tactics approval to at least ensure everyone's informed this is coming.

@carlossanlop let me know that they talked about this today in Tactics and that M2 approval is sufficient. I also sent out a mail to Tactics earlier today. Please let me know if we should do anything more.

Aside from Tactics visibility, what breaking change notifications do we typically broadcast for new APICompat validations?

Good point. APICompat / PackageValidation has received a couple of new features that are - by nature - breaking. I will file one breaking change issue for all the new features.

@joeloff joeloff merged commit e4d0b0a into release/8.0.1xx Aug 29, 2023
@joeloff joeloff deleted the ApiCompatBackports80 branch August 29, 2023 20:33
@ViktorHofer
Copy link
Member Author

ViktorHofer commented Aug 29, 2023

@MrJul again thanks for reporting #34864. Just wanted to let you know that this PR backported the change that closes your issue into .NET 8 RC2 and newer. RC2 will be available in October. If you want to depend on these changes already today you have the following options:

  1. Reference the Microsoft.DotNet.ApiCompat.Task nuget package from the dotnet9 feed: https://dnceng.pkgs.visualstudio.com/public/_packaging/dotnet9-transport/nuget/v3/index.json via a <PackageReference />
  2. Use a nightly build of the .NET 9 SDK: https://github.com/dotnet/installer#table

... otherwise, wait for the .NET 8 RC2 release.

@MrJul
Copy link

MrJul commented Aug 30, 2023

Thanks for the solving the issue quickly and letting me know, much appreciated :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants