Add API Diff tool as Github Action

[MrSkwiggs]

This tool checks and reports changes to Ignite's public API, so that they may not go through without proper review (or at least so that they are documented!).

Not sure if this matches Ignite's direction as a package but I figured I'd give it a try to see if it sparks any interest 👌

[MrSkwiggs]

Seems like the tool does not like being ran from forks 🤓 I'll check with the authors about this & reopen if/when fixed 👌 (if there's interest for it)

[wti]

Detecting API incompatibility would be super important in a project like this!

Here's how it was done using only first-party swift tools in protobuf:

apple/swift-protobuf#1545

Essentially:

swift package diagnose-api-breaking-changes main

Protobuf checks against main. For Ignite the convention is likely to rebase on main, so that would be correct, but a more accurate solution might check against the branch point or the last release. There are other github projects demoing that.

Any solution should be easy to run locally so contributors can preflight any PR checks. Using Swift also avoid the validation risks of running third-party integrations. Also, it's unclear if 3P tools avoid listing all API diffs (i.e., including API-compatible changes); that could be pretty noisy.

But fair warning: I get false positives running this locally on the command line, the swift tool has some outstanding bugs, and compatibility can be a pretty hard problem. hmm.

Given the difficulty and unreliability: perhaps downgrade integration from CI to a recommendation to run the tool before submitting a PR? And include in any PR a discussion of any API compatibility issues? Experience might then show a path to a noiseless, helpful CI integration.

(Sorry, this comment likely belongs in a discussion on the issue, not on the PR itself...)

[twostraws]

It's a good idea, and I think projects like this one particularly would benefit, but the false positives thing puts me off – something that is useful 80% of the time but annoying or confusing 20% of the time might cause more harm than good. Would it be okay if we come back to this in the future, once the tool has stabilized?

[MrSkwiggs]

Thanks for the feedback guys! Happy to see there is some interest for this :D

Protobuf checks against main. For Ignite the convention is likely to rebase on main, so that would be correct, but a more accurate solution might check against the branch point or the last release. There are other github projects demoing that.

This workflow does exactly that; if the PR is from a release branch against main, it will actually check against the previous tag 👌

Otherwise it just runs against the target branch (and currently is only setup to run on PRs against main anyways)

Long story short; this is something we definitely have control over 👍

Any solution should be easy to run locally so contributors can preflight any PR checks. Using Swift also avoid the validation risks of running third-party integrations. Also, it's unclear if 3P tools avoid listing all API diffs (i.e., including API-compatible changes); that could be pretty noisy.

Adyen's API Diff tool only checks the public API by default, but it can also check package and private (@_spi) access levels.

But fair warning: I get false positives running this locally on the command line, the swift tool has some outstanding bugs, and compatibility can be a pretty hard problem. hmm.

I don't believe Adyen's tool has these issues (or if they do, they are very minimal) as they implement their own diffing implementation to alleviate this.

something that is useful 80% of the time but annoying or confusing 20% of the time might cause more harm than good.

I agree, although I do not believe this tool has issues with false positives so far 🤔 While it is still in its infancy, it's actively being worked on and is currently in use by Adyen themselves (so there is definitely some legitimacy to it).

Of course, it remains your call and I can see the caveats. Would it perhaps make sense to do a trial run while Ignite is still not too active with external contributions?

[goergisn]

This PR needs #278 to actually produce the correct output.

[wti]

On diffs vs compatibility:

But fair warning: I get false positives running this locally on the command line, the swift tool has some outstanding bugs, and compatibility can be a pretty hard problem. hmm.

I don't believe Adyen's tool has these issues (or if they do, they are very minimal) as they implement their own diffing implementation to alleviate this.

This is how Adyen's tool is described at https://github.com/Adyen/adyen-swift-public-api-diff

This tool allows comparing 2 versions of a swift (sdk) project and lists all changes in a human readable way.

It makes use of .swiftinterface files that get produced during the archiving of a swift project and parses them using swift-syntax.

That tool aims to list all changes, not just incompatible changes. That's pretty noisy. The goal would instead be to fail the build if any API-incompatible change is made, and that's what the Swift tool tries to flag.

It's non-trivial to use swift-syntax, and it's unclear how many people are using Ayden's tool (i.e., validating it works), so using it would mean significant quality investigation.

[MrSkwiggs]

All good points 👍

That tool aims to list all changes, not just incompatible changes. That's pretty noisy. The goal would instead be to fail the build if any API-incompatible change is made, and that's what the Swift tool tries to flag.

Well for documentation purposes it would still be good to have, but indeed maybe not for every PR. Maybe as a part of a release/tag workflow? And keep breaking-changes-only on PRs.

It's non-trivial to use swift-syntax, and it's unclear how many people are using Ayden's tool (i.e., validating it works), so using it would mean significant quality investigation.

For sure! I mean the tool's pretty new and is actively being worked on, but I believed it being from Adyen, meant it would be reliable. But maybe it needs a bit more time grow...

[twostraws]

I'm not keen to put this directly into the repository until we have some experience using it locally first – I'd like to see it deliver proven benefits (and no false positives!) with local runs, so that I have the confidence it definitely adds value before we make it mandatory in the repository. So, is it okay if we close this issue and perhaps come back to it later?

[MrSkwiggs]

By all means 👍 I'll get back to this once the pressure dies down a bit, and share findings in Slack (or someplace else you would you prefer)

[twostraws]

Thank you!