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

Support KMP & Jetbrains compose multiplatform #110

Conversation

mr3y-the-programmer
Copy link
Contributor

@mr3y-the-programmer mr3y-the-programmer commented Jun 15, 2024

this PR expands the gradle plugin's support to make it work with compose multiplatform (at the very least android and jvm desktop targets should be supported now).

There are some breaking changes though, As usages of java File API in the core module have been migrated to okio, and java File API is exposed from some APIs' public surface, Therefore, those are affected by this change.

@mr3y-the-programmer
Copy link
Contributor Author

mr3y-the-programmer commented Jun 15, 2024

I'm having some issues testing those changes locally by publishing the gradle plugin to mavenLocal() repository.
Here is what I've tried so far:

- run .\gradlew publishToMavenLocal task
- add mavenLocal() repository to pluginManagement {} block in settings.gradle.kts
- add some sample multiplatform project with the following configuration in its build.gradle.kts file:

plugins {
 kotlin("multiplatform")
 id("org.jetbrains.compose") version libs.versions.compose.get()
 id("dev.shreyaspatil.compose-compiler-report-generator") version "1.3.1"
}

kotlin {
 jvm()
}

but I'm getting errors like these and I'm not sure how to deal with them:

An exception occurred applying plugin request [id: 'dev.shreyaspatil.compose-compiler-report-generator', version: '1.3.1']
> Failed to apply plugin 'dev.shreyaspatil.compose-compiler-report-generator'.
   > Could not create plugin of type 'ReportGenPlugin'.
      > Could not generate a decorated class for type ReportGenPlugin.
         > com/android/build/api/variant/AndroidComponentsExtension

maybe you can help me with this, as I'm not very familiar with testing gradle plugins locally?

@mr3y-the-programmer
Copy link
Contributor Author

mr3y-the-programmer commented Jun 16, 2024

UPDATE: I managed to get samples working and test the plugin locally. I found a small issue which is about registering gradle tasks for jvm & multiplatform at the right time when their target info is made available(we're doing it correctly for android modules using onVariants{} callback), Currently tasks are registered way earlier than that, and as a result, a runtime exception is thrown while applying the plugin in a project. I'm still working on it.

@mr3y-the-programmer
Copy link
Contributor Author

mr3y-the-programmer commented Jun 16, 2024

Found no issues so far after fixing those timing bugs.

I'm also thinking about pushing samples(multiplatform, android & desktop) to the repo to make it easier to test any changes right now or in the future & provide a rapid feedback loop but I'll probably do it in another PR to make changes easier to review and to keep this PR focused on one single topic, WDYT?

@PatilShreyas
Copy link
Owner

@mr3y-the-programmer That's great. I've not yet looked into the PR changes. Let me get back to you after taking a look. I won't be able to spend much time here, so please expect a delay

Copy link
Owner

@PatilShreyas PatilShreyas left a comment

Choose a reason for hiding this comment

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

@mr3y-the-programmer Did first round of review. Have some comments, can you please look into them? Plugin changes LGTM 👍🏻 Nice work.

build.gradle.kts Outdated Show resolved Hide resolved
cli/build.gradle.kts Outdated Show resolved Hide resolved
core/build.gradle.kts Outdated Show resolved Hide resolved
core/build.gradle.kts Outdated Show resolved Hide resolved
@mr3y-the-programmer
Copy link
Contributor Author

Did the required modifications based on the recent comments, take your time and see if you've any other questions

Copy link
Owner

@PatilShreyas PatilShreyas left a comment

Choose a reason for hiding this comment

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

This changeset actually looks good to me. Nice work here and I appreciate your contribution 😃. I've few more comments here.

build.gradle.kts Outdated Show resolved Hide resolved
gradle-plugin/build.gradle.kts Show resolved Hide resolved
val composeMultiplatform =
runCatching {
target.extensions.getByType(ComposeExtension::class.java)
}.getOrNull()
Copy link
Owner

Choose a reason for hiding this comment

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

I'll get back here with something if there's anything else by which we can write this in better way. I've did something like this in my another plugin: https://github.com/PatilShreyas/bytemask/blob/v1.0.0-alpha01/gradle-plugin/src/main/java/dev/shreyaspatil/bytemask/plugin/BytemaskPlugin.kt#L40-L52

Can we do similar here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh, This is basically the way I'm doing it in other plugins too but I tried to follow the same conventions of your plugin, That being said, I will try to rewrite those using pluginManager.withPlugin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

@mr3y-the-programmer
Copy link
Contributor Author

made the required changes and tested the plugin locally, I saw no regressions 🥳

@PatilShreyas
Copy link
Owner

made the required changes and tested the plugin locally, I saw no regressions 🥳

Cool, now change looks good @mr3y-the-programmer. Thanks for contributing and it's definitely gonna help multi-platform devs. Kudos 🎉

I'll merge it in a separate branch and will do some cleanup, doc update and will plan release in few days.

@PatilShreyas PatilShreyas changed the base branch from main to multiplatform June 20, 2024 12:06
@PatilShreyas PatilShreyas merged commit df6f61e into PatilShreyas:multiplatform Jun 20, 2024
2 checks passed
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

2 participants