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

Don't show code completion for non-existenst API in commonMain that fails on Android with NoSuchMethodException #1328

Merged
merged 1 commit into from
May 3, 2024

Conversation

igordmn
Copy link
Collaborator

@igordmn igordmn commented Apr 29, 2024

Fixes JetBrains/compose-multiplatform#3503
Fixes https://youtrack.jetbrains.com/issue/COMPOSE-481/skikoMain-API-is-available-in-commonMain

When we apply redirecting on androidx artifacts, we replace this tree of dependencies:

app -> jb-foundation
app -> jb-foundation-android -> jb-ui-android -> jb-ui-graphics-android

by this tree:

app -> jb-foundation
app -> androidx-foundation-> androidx-ui-> androidx-ui-graphics

Because we don't match trees in different dependencies, KMP plugins can't resolve the available API properly. To solve this, we add additional dependencies to the root JB artifacts (which is safe, as they will be redirected).

Instead of:

    {
      "name": "releaseApiElements-published",
      "attributes": {
        "org.gradle.category": "library",
        "org.gradle.usage": "java-api",
        "org.jetbrains.kotlin.platform.type": "androidJvm"
      },
      "dependencies": [
        {
          "group": "androidx.compose.ui",
          "module": "ui",
          "version": {
            "requires": "1.6.4"
          }
        }
      ]
    },

(file)
we use now:

    {
      "name": "releaseApiElements-published",
      "attributes": {
        "org.gradle.category": "library",
        "org.gradle.usage": "java-api",
        "org.jetbrains.kotlin.platform.type": "androidJvm"
      },
      "dependencies": [
        {
          "group": "androidx.annotation",
          "module": "annotation",
          "version": {
            "requires": "1.5.0"
          }
        },
        {
          "group": "androidx.compose.ui",
          "module": "ui",
          "version": {
            "requires": "1.6.6"
          }
        },
        {
          "group": "org.jetbrains.compose.runtime",
          "module": "runtime-saveable",
          "version": {
            "requires": "0.0.0-igor.demin-fix-skiko-api-in-common-dev1610"
          }
        },
        {
          "group": "org.jetbrains.compose.ui",
          "module": "ui-geometry",
          "version": {
            "requires": "0.0.0-igor.demin-fix-skiko-api-in-common-dev1610"
          }
        },
        {
          "group": "org.jetbrains.compose.ui",
          "module": "ui-graphics",
          "version": {
            "requires": "0.0.0-igor.demin-fix-skiko-api-in-common-dev1610"
          }
        },
        {
          "group": "org.jetbrains.compose.ui",
          "module": "ui-text",
          "version": {
            "requires": "0.0.0-igor.demin-fix-skiko-api-in-common-dev1610"
          }
        },
        {
          "group": "org.jetbrains.compose.ui",
          "module": "ui-unit",
          "version": {
            "requires": "0.0.0-igor.demin-fix-skiko-api-in-common-dev1610"
          }
        },
        {
          "group": "org.jetbrains.compose.ui",
          "module": "ui-util",
          "version": {
            "requires": "0.0.0-igor.demin-fix-skiko-api-in-common-dev1610"
          }
        }
      ]
    },

(file)

Testing

The project from https://kmp.jetbrains.com works on different targets with implementation(compose.ui) commented (bug is reproduce only this way)

  • use 0.0.0-igor.demin-fix-skiko-api-in-common-dev1610 built from this PR
  • check that Android runs
  • check that desktop runs
  • check that skikoMain-only API doesn't appear in commonMain: Modifier.onPointerEvent
  • but appears in desktopMain

Release Notes

Fixes - Multiple Platforms

  • Don't show code completion for non-existenst API in commonMain that fails on Android with NoSuchMethodException

@igordmn
Copy link
Collaborator Author

igordmn commented Apr 29, 2024

Will be included in 1.7.0

@igordmn igordmn requested a review from eymar April 29, 2024 11:57
@eymar
Copy link
Member

eymar commented May 3, 2024

with implementation(compose.ui) commented (bug is reproduce only this way)

I reproduced the issue without commenting implementation(compose.ui) on CM 1.6.2.

Then with your version, IDE doesn't resolve skiko symbols and doesn't list them in completion suggestsions in commonMain.

So the fix works as expected. (Not sure why there was a need to comment implementation(compose.ui))

@igordmn
Copy link
Collaborator Author

igordmn commented May 3, 2024

I reproduced the issue without commenting implementation(compose.ui) on CM 1.6.2.

The bug was - if there is no explicit dependency on the module, then we see skikoMain API of this module

@igordmn igordmn merged commit 3dc1df1 into jb-main May 3, 2024
6 checks passed
@igordmn igordmn deleted the igor.demin/fix-skiko-api-in-common branch May 3, 2024 11:00
igordmn added a commit that referenced this pull request May 8, 2024
…in` that fails on Android with `NoSuchMethodException` (#1328)"

This reverts commit 3dc1df1.
igordmn added a commit to JetBrains/compose-multiplatform that referenced this pull request May 22, 2024
AGP 7 isn't supported by Jetpack Compose 1.7 (which we'll use in CMP
1.7) and by Lifecycle 2.8.

After this CI is merged, I will:
- revert the revert of [the skiko API
fix](JetBrains/compose-multiplatform-core#1328)
in jb-main
- raise the AGP version on TeamCity

### Details

It was discovered after merging [the skiko API
fix](JetBrains/compose-multiplatform-core#1328).
We had `D8: java.lang.NullPointerException` error on TeamCity. After
investigating dependencies, it appears that previous the dependencies
were incorrect:
```
org.jetbrains.compose.material:material -> androidx.compose.material:material:1.6.7 -> androidx.lifecycle:lifecycle-livedata-core:2.6.1
```
And the new are correct, but aren't supported by AGP 7:
```
org.jetbrains.compose.material:material -> org.jetbrains.compose.ui -> org.jetbrains.androidx.lifecycle:lifecycle-common -> androidx.lifecycle:lifecycle-common:2.8.0 -> androidx.lifecycle:lifecycle-livedata-core:2.8.0
```
The dependency chains was printed by this command on the
`commonResources` test project:
```
./gradlew dependencyInsight --configuration demoDebugRuntimeClasspath --dependency androidx.lifecycle:lifecycle-livedata-core
```

## Testing

1. Create a pure Jetpack Compose project via Android Studio Hedgehog |
2023.1.1 Patch 2
2. Set Jetpack Compose  to 1.7.0-beta01
3. Downgrade AGP to 7.4.0
4. Run
5. See the error:
```
ERROR:C:\Users\igord\.gradle\caches\transforms-4\7fcc63c0892ce9266300d6463c3c751d\transformed\lifecycle-livedata-core-2.8.0-runtime.jar: D8: java.lang.NullPointerException: Cannot invoke "String.length()" because "<parameter1>" is null
Execution failed for task ':app:mergeExtDexDebug'.
> Could not resolve all files for configuration ':app:debugRuntimeClasspath'.
   > Failed to transform lifecycle-livedata-core-2.8.0.aar (androidx.lifecycle:lifecycle-livedata-core:2.8.0) to match attributes {artifactType=android-dex, asm-transformed-variant=NONE, dexing-enable-desugaring=true, dexing-enable-jacoco-instrumentation=false, dexing-is-debuggable=true, dexing-min-sdk=24, org.gradle.category=library, org.gradle.dependency.bundling=external, org.gradle.libraryelements=aar, org.gradle.status=release, org.gradle.usage=java-runtime}.
      > Execution failed for DexingNoClasspathTransform: C:\Users\igord\.gradle\caches\transforms-3\4aae1223695d47acd0b149bb5811ca5f\transformed\lifecycle-livedata-core-2.8.0-runtime.jar.
         > Error while dexing.
```
6. Change AGP to 8.1.0
7. The run is successful.

## Release Notes
### Breaking changes - Android
- Minimal supported AGP raised to 8.1.0
igordmn added a commit that referenced this pull request Sep 16, 2024
Fixes https://youtrack.jetbrains.com/issue/CMP-5864/Compose-UI-test-error-on-android-No-static-method-forceEnableAppTracing

Regression after #1367

During redirection we added dependencies from "androidMain.dependencies" to the Android target, which contain a lot of prerelease versions. Also, we usually don't update `androidMain` during merges.

Instead of `androidMain` we should use `commonMain`. `androidMain` dependencies are added transitively from `androidx` redirection.

## Testing

1.
```
./gradlew :mpp:publishComposeJbToMavenLocal
```
`~/.m2/repository/org/jetbrains/compose/ui/ui-test/0.0.0-SNAPSHOT/ui-test-0.0.0-SNAPSHOT.module`, search `monitor`. It doesn't exist anymore

2. Retest #1328

## Release Notes
### Fixes - Multiplatform
- _(prerelease fix)_ Fix "Compose UI test error on android: No static method forceEnableAppTracing"
- _(prerelease fix)_ Fix "Android target depends on prerelease versions"
igordmn added a commit that referenced this pull request Sep 16, 2024
Fixes https://youtrack.jetbrains.com/issue/CMP-5864/Compose-UI-test-error-on-android-No-static-method-forceEnableAppTracing

Regression after #1367

During redirection we added dependencies from "androidMain.dependencies" to the Android target, which contain a lot of prerelease versions. Also, we usually don't update `androidMain` during merges.

Instead of `androidMain` we should use `commonMain`. `androidMain` dependencies are added transitively from `androidx` redirection.

## Testing

1.
```
./gradlew :mpp:publishComposeJbToMavenLocal
```
`~/.m2/repository/org/jetbrains/compose/ui/ui-test/0.0.0-SNAPSHOT/ui-test-0.0.0-SNAPSHOT.module`, search `monitor`. It doesn't exist anymore

2. Retest #1328

## Release Notes
### Fixes - Multiplatform
- _(prerelease fix)_ Fix "Compose UI test error on android: No static method forceEnableAppTracing"
- _(prerelease fix)_ Fix "Android target depends on prerelease versions"
igordmn added a commit that referenced this pull request Sep 16, 2024
Fixes https://youtrack.jetbrains.com/issue/CMP-5864/Compose-UI-test-error-on-android-No-static-method-forceEnableAppTracing

Regression after #1367

During redirection we added dependencies from "androidMain.dependencies" to the Android target, which contain a lot of prerelease versions. Also, we usually don't update `androidMain` during merges.

Instead of `androidMain` we should use `commonMain`. `androidMain` dependencies are added transitively from `androidx` redirection.

## Testing

1.
```
./gradlew :mpp:publishComposeJbToMavenLocal
```
`~/.m2/repository/org/jetbrains/compose/ui/ui-test/0.0.0-SNAPSHOT/ui-test-0.0.0-SNAPSHOT.module`, search `monitor`. It doesn't exist anymore

2. Retest #1328

## Release Notes
### Fixes - Multiplatform
- _(prerelease fix)_ Fix "Compose UI test error on android: No static method forceEnableAppTracing"
- _(prerelease fix)_ Fix "Android target depends on prerelease versions"
igordmn added a commit that referenced this pull request Sep 16, 2024
Fixes https://youtrack.jetbrains.com/issue/CMP-5864/Compose-UI-test-error-on-android-No-static-method-forceEnableAppTracing

Regression after #1367

During redirection we added dependencies from "androidMain.dependencies" to the Android target, which contain a lot of prerelease versions. Also, we usually don't update `androidMain` during merges.

Instead of `androidMain` we should use `commonMain`. `androidMain` dependencies are added transitively from `androidx` redirection.

## Testing

1.
```
./gradlew :mpp:publishComposeJbToMavenLocal
```
`~/.m2/repository/org/jetbrains/compose/ui/ui-test/0.0.0-SNAPSHOT/ui-test-0.0.0-SNAPSHOT.module`, search `monitor`. It doesn't exist anymore

2. Retest #1328

## Release Notes
### Fixes - Multiplatform
- _(prerelease fix)_ Fix "Compose UI test error on android: No static method forceEnableAppTracing"
- _(prerelease fix)_ Fix "Android target depends on prerelease versions"
igordmn added a commit that referenced this pull request Sep 16, 2024
Fixes
https://youtrack.jetbrains.com/issue/CMP-5864/Compose-UI-test-error-on-android-No-static-method-forceEnableAppTracing

Regression after
#1367

During redirection we added dependencies from `androidMain.dependencies`
to the Android target, which contains a lot of prerelease versions.
Also, we usually don't update `androidMain` during merges.

Instead of `androidMain` we should use `commonMain`. Pure `androidMain`
dependencies are added transitively from `androidx` redirection.

## Testing

1.
```
./gradlew :mpp:publishComposeJbToMavenLocal
```

`~/.m2/repository/org/jetbrains/compose/ui/ui-test/0.0.0-SNAPSHOT/ui-test-0.0.0-SNAPSHOT.module`,
search `monitor`. It doesn't exist anymore

2. Retest
#1328

This should be tested by QA.

## Release Notes
### Fixes - Multiplatform
- _(prerelease fix)_ Fix "Compose UI test error on android: No static
method forceEnableAppTracing"
- _(prerelease fix)_ Fix "Android target depends on prerelease versions"
igordmn added a commit that referenced this pull request Sep 16, 2024
Fixes
https://youtrack.jetbrains.com/issue/CMP-5864/Compose-UI-test-error-on-android-No-static-method-forceEnableAppTracing

Regression after
#1367

During redirection we added dependencies from `androidMain.dependencies`
to the Android target, which contains a lot of prerelease versions.
Also, we usually don't update `androidMain` during merges.

Instead of `androidMain` we should use `commonMain`. Pure `androidMain`
dependencies are added transitively from `androidx` redirection.

## Testing

1.
```
./gradlew :mpp:publishComposeJbToMavenLocal
```

`~/.m2/repository/org/jetbrains/compose/ui/ui-test/0.0.0-SNAPSHOT/ui-test-0.0.0-SNAPSHOT.module`,
search `monitor`. It doesn't exist anymore

2. Retest
#1328

This should be tested by QA.

## Release Notes
### Fixes - Multiplatform
- _(prerelease fix)_ Fix "Compose UI test error on android: No static
method forceEnableAppTracing"
- _(prerelease fix)_ Fix "Android target depends on prerelease versions"
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.

skikoMain API are shown as available in commonMain
2 participants