badoo / MVIKotlin

Extendable MVI framework for Kotlin Multiplatform with powerful debugging tools (logging and time travel), inspired by Badoo MVICore library
https://arkivanov.github.io/MVIKotlin
Apache License 2.0
828 stars 64 forks source link

Setup Kotlin hierarchical project structure #278

Closed sellmair closed 3 years ago

arkivanov commented 3 years ago

@sellmair Thank you for the PR! The build is failing on CI on configuration phase, where it is executed with MPP targets separation per host. It can be locally reproduced by running the following command: ./gradlew build -Dtargets=IOS,MACOS_X64. You can find the build logs here.

Here is the beginning part of the error message:

Could not determine the dependencies of task ':keepers:compileKotlinMetadata'.
> Could not resolve all task dependencies for configuration ':keepers:metadataCompileClasspath'.
   > Could not resolve project :utils-internal.
     Required by:
         project :keepers
      > The consumer was configured to find a usage of 'kotlin-api' of a component, as well as attribute 'org.jetbrains.kotlin.platform.type' with value 'common'. However we cannot choose between the following variants of project :utils-internal:
          - commonMainMetadataElements
          - iosArm64ApiElements
          - iosArm64Default
          - iosArm64RuntimeOnly
          - iosX64ApiElements
          - iosX64Default
          - iosX64RuntimeOnly
          - macosX64ApiElements
          - macosX64Default
          - macosX64RuntimeOnly
          - metadataDefault

I tried your branch, and the error is caused by the following flag: kotlin.mpp.enableGranularSourceSetsMetadata=true.

More context. I am not sure yet what is the real cause of the error. On CI I run builds on two hosts: Ubuntu and macOS. For each host there is a Job, which runs ./gradlew build -Ptargets=A,B,C. The targets parameter is used in build scripts to dynamically enable only required targets. This reduces build times, and also the cost, because macOS host is 10 times more expensive than Linux.

arkivanov commented 3 years ago

@sellmair Seems like there is a bug in HMPP. The build fails when (for example) only ios and macosX64 targets are enabled. Attached a minimal reproducer project.

HMPP_issue.zip

sellmair commented 3 years ago

Added a second commit: This happens because of the compatibility metadata variant not supporting the case when "commonMain" suddenly becomes a "shared native" source set.

I think generally disabling this variant, except when publishing metadata should work for you!

arkivanov commented 3 years ago

@sellmair Thanks for the fix! Now there are compilation errors.

2021-08-18T12:25:11.6725090Z > Task :mvikotlin:compileCommonMainKotlinMetadata FAILED
2021-08-18T12:25:11.6756340Z e: /Users/runner/work/MVIKotlin/MVIKotlin/mvikotlin/src/commonMain/kotlin/com/arkivanov/mvikotlin/core/binder/BinderExt.kt: (3, 22): Unresolved reference: essenty
2021-08-18T12:25:11.6758690Z Build cache key for task ':mvikotlin:compileCommonMainKotlinMetadata' is a1ac53bd4cfbc93834e55b05d8e4391f
2021-08-18T12:25:11.6764630Z e: /Users/runner/work/MVIKotlin/MVIKotlin/mvikotlin/src/commonMain/kotlin/com/arkivanov/mvikotlin/core/binder/BinderExt.kt: (4, 22): Unresolved reference: essenty
2021-08-18T12:25:11.6767180Z Task ':mvikotlin:compileCommonMainKotlinMetadata' is not up-to-date because:
2021-08-18T12:25:11.6768540Z e: /Users/runner/work/MVIKotlin/MVIKotlin/mvikotlin/src/commonMain/kotlin/com/arkivanov/mvikotlin/core/binder/BinderExt.kt: (15, 32): Unresolved reference: Lifecycle
2021-08-18T12:25:11.6773670Z   No history is available.
2021-08-18T12:25:11.6783260Z e: /Users/runner/work/MVIKotlin/MVIKotlin/mvikotlin/src/commonMain/kotlin/com/arkivanov/mvikotlin/core/instancekeeper/InstanceKeeperExt.kt: (3, 22): Unresolved reference: essenty
2021-08-18T12:25:11.6785720Z Run in-process tool "konanc"
2021-08-18T12:25:11.6787940Z e: /Users/runner/work/MVIKotlin/MVIKotlin/mvikotlin/src/commonMain/kotlin/com/arkivanov/mvikotlin/core/instancekeeper/InstanceKeeperExt.kt: (4, 22): Unresolved reference: essenty
2021-08-18T12:25:11.6791320Z e: /Users/runner/work/MVIKotlin/MVIKotlin/mvikotlin/src/commonMain/kotlin/com/arkivanov/mvikotlin/core/instancekeeper/InstanceKeeperExt.kt: (7, 26): Unresolved reference: InstanceKeeper
2021-08-18T12:25:11.6800560Z e: /Users/runner/work/MVIKotlin/MVIKotlin/mvikotlin/src/commonMain/kotlin/com/arkivanov/mvikotlin/core/instancekeeper/InstanceKeeperExt.kt: (12, 41): Unresolved reference: InstanceKeeper
2021-08-18T12:25:11.6802850Z e: /Users/runner/work/MVIKotlin/MVIKotlin/mvikotlin/src/commonMain/kotlin/com/arkivanov/mvikotlin/core/instancekeeper/InstanceKeeperExt.kt: (17, 5): Unresolved reference: InstanceKeeper
2021-08-18T12:25:11.6806140Z e: /Users/runner/work/MVIKotlin/MVIKotlin/mvikotlin/src/commonMain/kotlin/com/arkivanov/mvikotlin/core/instancekeeper/InstanceKeeperExt.kt: (18, 5): 'onDestroy' overrides nothing
sellmair commented 3 years ago

Oooooh! I see: This happens because due to the CI optimization of just registering certain targets, the 'commonMain' source set is considered "shared native" as it only shares code across native targets. When trying to compile metadata for such a shared native source set, the Kotlin/Native compiler is used over the old "metadata compiler". This new compiler cannot the old *.kotlin_metadata format produced by the non-hmpp library.

I see the following options:

Third option is advised by me. However, this whole story starts making me think: Maybe the Kotlin Multiplatform plugin should offer some umbrella tasks for specific targets like buildLinux or buildLinuxX64 , etc.

With such umbrella tasks automatically created, the CI config would be pretty simple 🤔

arkivanov commented 3 years ago

@sellmair I see! I used the top-level build task because I usually forget to add new modules to the corresponding GitHub jobs. And also any new checks (e.g. when I added detekt) should be added when introduced. Another possible issue here is that various tools add various tasks to the build, and the set of tasks changes over the versions. It will be hard to keep everything in sync.

So ideally I would like to have those buildXxx tasks. This would even simply Gradle configs, and scalability would not be affected.

arkivanov commented 3 years ago

@sellmair I forgot to mention that publication is also split by targets, plus additional metadata publication on Linux. So if there will be separate buildXxx tasks, it won't allow me to get rid of the target split for publication. Having publishXxx and publishXxxToMavenLocal tasks would help here as well, perhaps. Not 100% sure what would be a best way here

sellmair commented 3 years ago

Okay, with this in mind, I think disabling the corresponding tasks will get as somewhere:

macosX64().compilations.all {
            if(!isTargetEnabled(Target.MACOS_X64)) {
                compileKotlinTask.enabled = false
            }
        }

I added an example commit for this. Would you mind checking if something like this would work for you?