Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
JakeWharton opened this issue Jun 4, 2024 · 4 comments · Fixed by #236
Closed

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

JakeWharton opened this issue Jun 4, 2024 · 4 comments · Fixed by #236
Assignees
Labels
bug Something isn't working klib

Comments

@JakeWharton
Copy link

I removed a bunch of multiplatform targets in cashapp/redwood#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 cashapp/redwood#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 fzhinkin added bug Something isn't working klib labels Jun 4, 2024
@fzhinkin
Copy link
Collaborator

fzhinkin commented Jun 4, 2024

@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
Copy link
Collaborator

fzhinkin commented Jun 4, 2024

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 fzhinkin self-assigned this Jun 4, 2024
@fzhinkin
Copy link
Collaborator

fzhinkin commented Jun 4, 2024

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).

@fzhinkin fzhinkin added this to the klib/0.15.0-Beta.3 milestone Jun 4, 2024
@JakeWharton
Copy link
Author

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.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working klib
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants