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

Add binary-compatibility-validator and API dumps for desktop #803

Merged
merged 10 commits into from
Sep 13, 2023

Conversation

igordmn
Copy link
Collaborator

@igordmn igordmn commented Sep 8, 2023

https://github.com/Kotlin/binary-compatibility-validator

This plugin dumps all API for JVM modules to files (in the future - for other targets). If API changed, we should call:

./gradlew desktopApiDump

to commit a new API, otherwise CI will fail (./gradlew desktopCICheck will be called on CI`).

If you see that some strings are deleted in the API dump, most probably it means an incompatible API change (the plugin doesn't check itself, unfortunately).

See the official workflow guide to understand it more.

Example of error:

Execution failed for task ':compose:material3:material3:desktopApiCheck'.
> API check failed for project material3.
  --- D:\Work\compose-multiplatform-core\compose\material3\material3\api\desktop\material3.api
  +++ D:\Work\compose-multiplatform-core\out\androidx\compose\material3\material3\build\api\desktop\material3.api
  @@ -552,6 +552,11 @@
   public abstract interface annotation class androidx/compose/material3/ExperimentalMaterial3Api : java/lang/annotation/Annotation {
   }

  +public final class androidx/compose/material3/FF {
  +     public static final field $stable I
  +     public fun <init> ()V
  +}
  +
   public final class androidx/compose/material3/FabPosition {
        public static final field Companion Landroidx/compose/material3/FabPosition$Companion;
        public static final synthetic fun box-impl (I)Landroidx/compose/material3/FabPosition;

   You can run :material3:apiDump task to overwrite API declarations

One disadvantage that error shows apiDump instead of desktopApiDump. apiDump doesn't work because of the broken Android target, and to fix it properly we need to do a lot of changes. Just disabling the tasks doesn't work.

  • Desktop API is constructed from desktopMain + skikoMain + commonMain sourceSets

  • Android API is constructed from desktopMain + commonMain. It is disabled for now, because Android build is broken. The plan is to fix the Android target, and dump Android API as well. It will help us to track new API that we need to port more easily.

P.S. Jetpack Compose sources has its own API generator (Metalava), but it won't be applicable to other Kotlin targets in the future, but binary-compatibility-validator will

Test

  1. call desktopCICheck - it succeeds
  2. change/add API
  3. call desktopCICheck - it fails

Also remove `testDesktop`, `desktopTest` should be used instead.

This task will be run on CI, and we will add other tasks to it (such as API checks).
https://github.com/Kotlin/binary-compatibility-validator

This plugin dumps all API for JVM modules to files (in the future - for other targets). If API changed, we should call:
```
./gradlew desktopApiDump
```
to commit a new API, otherwise CI will fail (`./gradlew desktopCICheck` will be called on CI`).

If you see that some strings are deleted in the API dump, most probably it means an incompatible API change (the plugin doesn't check itself, unfortunately).

- For Desktop API is constructed from desktopMain + skikoMain + commonMain sourceSets

- For Android API is constructed from desktopMain + commonMain (but it is disabled for now, because Android is in a broke state). The plan is to fix the Android target, and dump Android API as well. It will help us to track new API that we need to port more easily.

[The workflow](https://github.com/Kotlin/binary-compatibility-validator#workflow)

Example of error:
```
Execution failed for task ':compose:material3:material3:desktopApiCheck'.
> API check failed for project material3.
  --- D:\Work\compose-multiplatform-core\compose\material3\material3\api\desktop\material3.api
  +++ D:\Work\compose-multiplatform-core\out\androidx\compose\material3\material3\build\api\desktop\material3.api
  @@ -552,6 +552,11 @@
   public abstract interface annotation class androidx/compose/material3/ExperimentalMaterial3Api : java/lang/annotation/Annotation {
   }

  +public final class androidx/compose/material3/FF {
  +     public static final field $stable I
  +     public fun <init> ()V
  +}
  +
   public final class androidx/compose/material3/FabPosition {
        public static final field Companion Landroidx/compose/material3/FabPosition$Companion;
        public static final synthetic fun box-impl (I)Landroidx/compose/material3/FabPosition;

   You can run :material3:apiDump task to overwrite API declarations
```

One disadvantage that error shows `apiDump` instead of `desktopApiDump`. `apiDump` doesn't work because of the broken Android target, and to fix it properly we need to do a lot of changes. Just disabling the tasks doesn't work.

P.S. Jetpack Compose sources has its own API generator (Metalava), but it won't be applicable to other Kotlin targets in the future, but binary-compatibility-validator [will](Kotlin/binary-compatibility-validator#149)

## Test
1. call desktopCICheck - it succeeds
2. change/add API
3. call desktopCICheck - it fails
@igordmn igordmn force-pushed the igor.demin/make-api-file branch from 05740dd to 518a8f2 Compare September 8, 2023 18:16
@igordmn igordmn changed the base branch from jb-main to igor.demin/ci-task September 8, 2023 18:17
@igordmn igordmn marked this pull request as ready for review September 8, 2023 19:56

public final class androidx/compose/material3/AlertDialog_skikoKt {
public static final fun AlertDialog (Lkotlin/jvm/functions/Function0;Landroidx/compose/ui/Modifier;Landroidx/compose/ui/window/DialogProperties;Lkotlin/jvm/functions/Function2;Landroidx/compose/runtime/Composer;II)V
public static final fun AlertDialog-Oix01E0 (Lkotlin/jvm/functions/Function0;Lkotlin/jvm/functions/Function2;Landroidx/compose/ui/Modifier;Lkotlin/jvm/functions/Function2;Lkotlin/jvm/functions/Function2;Lkotlin/jvm/functions/Function2;Lkotlin/jvm/functions/Function2;Landroidx/compose/ui/graphics/Shape;JJJJFLandroidx/compose/ui/window/DialogProperties;Landroidx/compose/runtime/Composer;III)V
Copy link
Member

Choose a reason for hiding this comment

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

  1. What -Oix01E0 mean here?
  2. It looks like this validator doesn't handle values of default parameters, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. It is how Kotlin compiles a fun with value classes (Color, for example):
    image

  2. Kotlin compiles such functions to Foo$default:

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. And it seems, yes, we can't track default values changes. But it is a behaviour change, not API change

@igordmn igordmn mentioned this pull request Sep 12, 2023
Base automatically changed from igor.demin/ci-task to jb-main September 12, 2023 13:28
@igordmn igordmn requested a review from MatkovIvan September 12, 2023 13:40
@igordmn
Copy link
Collaborator Author

igordmn commented Sep 12, 2023

Added a readme

MULTIPLATFORM.md Outdated
@@ -26,6 +26,38 @@ Run tests for UIKit:
./gradlew :mpp:testUIKit
```

### API checks
Compose Multiplatform stores all public API in *.api files. If any API is added/changed, `./gradlew desktopCICheck` will fail with an error that API is changed (it runs on CI). Example:
Copy link
Collaborator

Choose a reason for hiding this comment

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

desktopCICheck -> checkDesktop

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

@igordmn igordmn merged commit 494405d into jb-main Sep 13, 2023
@igordmn igordmn deleted the igor.demin/make-api-file branch September 13, 2023 14:43
igordmn added a commit that referenced this pull request Jan 30, 2024
https://github.com/Kotlin/binary-compatibility-validator

This plugin dumps all API for JVM modules to files (in the future - for
other targets). If API changed, we should call:
```
./gradlew desktopApiDump
```
to commit a new API, otherwise CI will fail (`./gradlew desktopCICheck`
will be called on CI`).

If you see that some strings are deleted in the API dump, most probably
it means an incompatible API change (the plugin doesn't check itself,
unfortunately).

See [the official workflow
guide](https://github.com/Kotlin/binary-compatibility-validator#workflow)
to understand it more.

Example of error:
```
Execution failed for task ':compose:material3:material3:desktopApiCheck'.
> API check failed for project material3.
  --- D:\Work\compose-multiplatform-core\compose\material3\material3\api\desktop\material3.api
  +++ D:\Work\compose-multiplatform-core\out\androidx\compose\material3\material3\build\api\desktop\material3.api
  @@ -552,6 +552,11 @@
   public abstract interface annotation class androidx/compose/material3/ExperimentalMaterial3Api : java/lang/annotation/Annotation {
   }

  +public final class androidx/compose/material3/FF {
  +     public static final field $stable I
  +     public fun <init> ()V
  +}
  +
   public final class androidx/compose/material3/FabPosition {
        public static final field Companion Landroidx/compose/material3/FabPosition$Companion;
        public static final synthetic fun box-impl (I)Landroidx/compose/material3/FabPosition;

   You can run :material3:apiDump task to overwrite API declarations
```

One disadvantage that error shows `apiDump` instead of `desktopApiDump`.
`apiDump` doesn't work because of the broken Android target, and to fix
it properly we need to do a lot of changes. Just disabling the tasks
doesn't work.

- Desktop API is constructed from desktopMain + skikoMain + commonMain
sourceSets

- Android API is constructed from desktopMain + commonMain. It is
disabled for now, because Android build is broken. The plan is to fix
the Android target, and dump Android API as well. It will help us to
track new API that we need to port more easily.

P.S. Jetpack Compose sources has its own API generator (Metalava), but
it won't be applicable to other Kotlin targets in the future, but
binary-compatibility-validator
[will](Kotlin/binary-compatibility-validator#149)

## Test
1. call desktopCICheck - it succeeds
2. change/add API
3. call desktopCICheck - it fails

---------

Co-authored-by: Ivan Matkov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants