search

Kotlin / binary-compatibility-validator

Public API management tool
Apache License 2.0
759 stars 55 forks source link

Removing native targets did not cause API check to fail #234

Closed JakeWharton closed 1 week ago

JakeWharton commented 1 month ago

I removed a bunch of multiplatform targets in https://github.com/cashapp/redwood/pull/2070 but made no actual API changes.

There were three types of removals:

  1. Module with ios (arm/x86/simulator) and macos (arm/x86) were changed to be ios-only.
  2. Module with ios, macos, jvm, and js was changed to jvm and js only.
  3. Module with android and jvm were changed to be jvm only.

In all three cases the apiCheck task did not fail despite running apiDump producing significant changes to the API files (as seen in https://github.com/cashapp/redwood/pull/2077).

Points 1 and 2 are similar, both being cases of the targets in the API file being now incorrect. Point 3 is interesting because the API files in android/ and jvm/ were now dead and the API file in the root was missing.

I would have hoped all three cases to be detected, fail the build, and require an apiDump to update the files.

fzhinkin commented 1 month ago

@JakeWharton, thanks for reporting the issue!

With native targets (points 1 and 2), there's a bug related to how we mitigate failures related to Apple targets being unavailable on Linux & Windows. As a workaround, apiValidation.klib.strictValidation = true will fail the apiCheck, but with not a very meaningful message (missing targets need to be included there, in the message I mean).

fzhinkin commented 1 month ago

As of an Android target removal, the task responsible for dump comparison (jvmApiCheck) traverses a file tree rooted by the api directory until it finds a file with an appropriate name (for example, redwood-protocol-guest.api).

So it finds either of {android,jvm}/*.api files and validates the generated dump against it.

I don't think it was implemented that way intentionally.

fzhinkin commented 1 month ago

For Klibs, the logic of target filtration should be inverted to exclude all unsupported targets (among those defined in a project) instead of leaving all supported targets (which does not include targets removed since the golden dump was created).

JakeWharton commented 1 month ago

As a workaround, apiValidation.klib.strictValidation = true will fail the apiCheck, but with not a very meaningful message (missing targets need to be included there, in the message I mean).

Ah, nice. Thanks! For now I enabled this only on CI where we're always on a Mac. The message is not great, like you said, but it should be extremely rare (and it's probably only me making the changes).

Adding the actual missing targets to the message would be a nice improvement. And I'm not sure whether it's worth the code to disambiguate between targets that are unavailable on the host OS, or those which are available but have been removed, but that also would be nice.