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 a warning for a user who sets compose.kotlinCompilerPlugin to androidx.compose.compiler.compiler #3313

Merged
merged 5 commits into from Jul 6, 2023

Conversation

eymar
Copy link
Collaborator

@eymar eymar commented Jul 5, 2023

To help users figure out the reason why their compose app doesn't compile for ios or web, let's show a warning when they use androidx.compose.compiler:compiler

The warning will show up only when a multiplatform project contains at least one of the non-jvm targets: k/js, k/native or k/wasm

…android.compose.compiler.compiler`

The warning will show up only when a multiplatform project contains at least one of the non-jvm targets: k/js, k/native or k/wasm
@@ -25,6 +26,25 @@ class ComposeCompilerKotlinSupportPlugin : KotlinCompilerPluginSupportPlugin {
ComposeCompilerCompatibility.compilerVersionFor(target.getKotlinPluginVersion())
}
}
target.afterEvaluate {
warnAboutJetpackComposeCompilerUsageForNonJvm(it)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will be shown multiple times, as for the error (#2866).

The fix is probably using some global AtomicBoolean, but I am not sure where to place it (Gradle can cache confiugurations).

So, i just mentioned it to not forget to fix it in the future.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@eymar @igordmn

If there is a cache entry for a particular set of scripts, plugins, build arguments and environment variables, configuration phase will be skipped (including afterEvaluate and all code under Plugin.apply)

One way to avoid that and show a warning just once is to use a build service.

import org.gradle.api.Plugin
import org.gradle.api.Project
import org.gradle.api.logging.Logging
import org.gradle.api.provider.Provider
import org.gradle.api.provider.SetProperty
import org.gradle.api.services.BuildService
import org.gradle.api.services.BuildServiceParameters
import org.gradle.api.services.BuildServiceRegistry
import org.gradle.build.event.BuildEventsListenerRegistry
import org.gradle.tooling.events.FinishEvent
import org.gradle.tooling.events.OperationCompletionListener
import javax.inject.Inject

// The service implements OperationCompletionListener just so we Gradle would use the service
// even if the service is not used by any task or transformation
abstract class ComposeMultiplatformBuildService : BuildService<ComposeMultiplatformBuildService.Parameters>, OperationCompletionListener, AutoCloseable {
    interface Parameters : BuildServiceParameters {
        val postBuildWarnings: SetProperty<String>
    }

    private val log = Logging.getLogger(this.javaClass)

    override fun close() {
        parameters.postBuildWarnings.get().forEach {
            log.error(it)
        }
    }

    override fun onFinish(event: FinishEvent) {}

    companion object {
        fun configure(project: Project, fn: Parameters.() -> Unit): Provider<ComposeMultiplatformBuildService> =
            project.gradle.sharedServices.registerOrConfigure<Parameters, ComposeMultiplatformBuildService> {
                fn()
            }
    }
}

inline fun <reified P : BuildServiceParameters, reified S : BuildService<P>> BuildServiceRegistry.registerOrConfigure(
    crossinline fn: P.() -> Unit
): Provider<S> {
    val serviceClass = S::class.java
    val serviceFqName = serviceClass.canonicalName
    val existingService = registrations.findByName(serviceFqName)
        ?.apply { (parameters as? P)?.fn() }
        ?.service
    return (existingService as? Provider<S>)
        ?: registerIfAbsent(serviceFqName, serviceClass) {
            parameters.fn()
        }
}

The service first be registered like this:

abstract class ComposePlugin : Plugin<Project> {
    @Inject
    abstract fun getEventsListenerRegistry(): BuildEventsListenerRegistry

    override fun apply(project: Project) {
        val service = ComposeMultiplatformBuildService.configure(project) {}
        // we add the service to the BuildEventsListenerRegistry, so
        // Gradle uses it even if the service is not used by tasks
        getEventsListenerRegistry().onTaskCompletion(service)
    }
}

Then, a warning can be added like this:

ComposeMultiplatformBuildService.configure(project) {
    postBuildWarnings.add("A warning that will be shown once per build!")
}   

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Refactored with ComposeMultiplatformBuildService. Please take another look


private fun createWarningAboutNonCompatibleCompiler(currentCompilerPluginGroupId: String): String {
return """
| WARNING: You are using the '$currentCompilerPluginGroupId' compiler plugin in your Kotlin multiplatform project.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest to keep the message as concise as possible, so users are less likely to ignore it.
Especially if the warning would be shown for each affected module,
in which case a more verbose warning becomes visual noise.

My attempt:

WARNING: Using Custom Compose Compiler plugin ('$currentCompilerPluginGroupId') 
with non-JVM targets targets (Kotlin/Native, Kotlin/JS, Kotlin/WASM) is not supported.
More information: $COMPOSE_COMPILER_COMPATIBILITY_LINK

Alternatively, you could ask in #proofread-me Slack channel.

At the very least, multiplatform, jvm, and dekstop (target) should start with an upper case character.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ack

Copy link
Collaborator

@AlexeyTsvetkov AlexeyTsvetkov left a comment

Choose a reason for hiding this comment

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

Generally LGTM, but I suggest considering using a build service to show a warning even if configuration is loaded from the cache, and to show warning only once https://github.com/JetBrains/compose-multiplatform/pull/3313/files#r1253668708

@eymar eymar requested a review from AlexeyTsvetkov July 6, 2023 13:57
@eymar eymar merged commit f563664 into master Jul 6, 2023
3 checks passed
@eymar eymar deleted the add_warning_about_jc_compiler branch July 6, 2023 18:35
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.

None yet

3 participants