commit 1ca545f06e2fd96888de1a4458ec5373ada27d87
author Mitchell Kember <mkember@google.com> Tue Sep 20 03:05:22 2022 +0000
committer CQ Bot <fuchsia-internal-scoped@luci-project-accounts.iam.gserviceaccount.com> Tue Sep 20 03:05:22 2022 +0000
tree b3176b25983aa73370cc02e4aea0a5c116ac5bbb
parent 5d405dddbdbcaebba52879ce292c14740e8d7f2e

[fidl] Add ordinals to API summaries and reject ABI-breaking changes

This CL adds ordinals to FIDL API summaries for protocol methods and for
table/union/struct members. For structs, the ordinal is the one-based
index of the member, and changing it is API-breaking because C++
bindings allow positional construction. For the rest, changing ordinals
breaks ABI, so fidl_api_diff will report APICompatibleButABIBreaking.

Some context: Before this CL, the summary/diff tooling only detected API
breakages. Ideally we'd analyze both API and ABI separately, but this is
harder to do. Instead, we're taking the simpler approach of ensuring we
also reject the class of changes that break ABI without affecting API.
The logic is conservative, i.e. some safe changes might be rejected but
no breaking change will ever be accepted.

Here is how fidl_api_diff's behavior is changing:

1. API compatible, ABI compatible: Accept
2. API breaking,   ABI breaking:   Reject
3. API compatible, ABI breaking:   Reject (NEW)
4. API breaking,   ABI compatible: Reject

We still lack the ability to distinguish (2) from (4), which means if
someone submits a breaking API change at a future API level (which is
allowed within reason), reviewers must manually check ABI compatibility.

I ran this to confirm .api_summary.json changes were as expected:

    remove_ordinals() { jq --sort-keys --indent 4 'map(del(.ordinal))'; }
    check() { diff <(git show @~:"$1") <(git show @:"$1" | remove_ordinals); }
    export -f remove_ordinals check
    # `sudo apt-get install parallel` to ensure GNU parallel, not moreutils
    git show --pretty="format:" --name-only -- '*.api_summary.json' | parallel check

Test: fx test tools/fidl/lib/apidiff
Fixed: 102427 (encode ABI in summaries)
Bug: 105046 (rename summaries)
Change-Id: I4aef7eed2ba3fceedf1d60053260ab849254f8fa
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/721565
Reviewed-by: Kendal Harland <kjharland@google.com>
API-Review: Adam Barth <abarth@google.com>
Commit-Queue: Mitchell Kember <mkember@google.com>
Reviewed-by: Adam Barth <abarth@google.com>