Skip to content

Pt. 1: Migrate :workflow-core and :workflow-runtime to kmp gradle plugin #793

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 23, 2022
Merged

Pt. 1: Migrate :workflow-core and :workflow-runtime to kmp gradle plugin #793

merged 1 commit into from
May 23, 2022

Conversation

bnvinay92
Copy link
Contributor

@bnvinay92 bnvinay92 commented May 10, 2022 •
edited
Loading

Also migrates from the jmh gradle plugin to kotlinx.benchmark (used in :workflow-runtime).
Running ./gradlew publishToMavenLocal publishes both workflow-core and workflow-core-jvm.
Screen Shot 2022-05-11 at 2 10 04 AM

@bnvinay92 bnvinay92 requested review from a team and zach-klippenstein as code owners May 10, 2022 22:01
@bnvinay92 bnvinay92 changed the title Migrate :workflow-core and :workflow-runtime to kmp gradle plugin Pt. 1: Migrate :workflow-core and :workflow-runtime to kmp gradle plugin May 10, 2022
bnvinay92
bnvinay92 commented May 10, 2022
View reviewed changes
@bnvinay92 bnvinay92 force-pushed the vn/p1-gradle branch 3 times, most recently from ceae652 to e13cd5d Compare May 10, 2022 22:52
bnvinay92
bnvinay92 commented May 10, 2022
View reviewed changes
dependencies/classpath.txt
@@ -164,7 +166,7 @@ org.jlleitschuh.gradle:ktlint-gradle:10.3.0
org.json:json:20180813
org.jsoup:jsoup:1.13.1
org.jvnet.staxex:stax-ex:1.8.1
org.openjdk.jmh:jmh-core:1.27
org.openjdk.jmh:jmh-core:1.21
Copy link
Contributor Author

@bnvinay92 bnvinay92 May 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Though the plugin lets you override this version and its 1.34. Not sure if this is accurate.

steve-the-edwards
steve-the-edwards reviewed May 11, 2022
View reviewed changes
Copy link
Contributor

@steve-the-edwards steve-the-edwards left a comment

Choose a reason for hiding this comment

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

LGTM. I will connect with @RBusarow about it too.

Maybe @rjrjr (though he is away this week so it will be next week before he can loop back in).

dependencies/classpath.txt
@@ -73,6 +73,7 @@ com.squareup.retrofit2:converter-moshi:2.9.0
com.squareup.retrofit2:retrofit:2.9.0
com.squareup:javapoet:1.10.0
com.squareup:javawriter:2.5.0
com.squareup:kotlinpoet:1.3.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know where this was introduced?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's coming from the kotlinx-benchmark plugin. (build scan)

It shouldn't really matter, since this is the build classpath.

RBusarow
RBusarow reviewed May 13, 2022
View reviewed changes
Copy link
Contributor

@RBusarow RBusarow left a comment

Choose a reason for hiding this comment

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

This seems pretty straightforward, though I'm not sure what magic is creating those *-jvm artifacts.

I'm making a snapshot build and testing against our main project. I'll report back after all the CI finishes.

dependencies/classpath.txt
@@ -73,6 +73,7 @@ com.squareup.retrofit2:converter-moshi:2.9.0
com.squareup.retrofit2:retrofit:2.9.0
com.squareup:javapoet:1.10.0
com.squareup:javawriter:2.5.0
com.squareup:kotlinpoet:1.3.0
Copy link
Contributor

Choose a reason for hiding this comment

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

It's coming from the kotlinx-benchmark plugin. (build scan)

It shouldn't really matter, since this is the build classpath.

@RBusarow
Copy link
Contributor

So @bnvinay92, our CI is green and we should be good to go, except we're missing our common Kotlin settings in these KMP modules. This is why the .api files were changed, and why we're now getting some compiler warnings in those modules.
These settings were applied by the kotlin-jvm convention plugin.

I've made a PR to introduce a sibling kotlin-multiplatform plugin (#794).

@bnvinay92 bnvinay92 closed this May 17, 2022
@bnvinay92 bnvinay92 reopened this May 17, 2022
@bnvinay92
Copy link
Contributor Author

bnvinay92 commented May 19, 2022 •
edited
Loading

This seems pretty straightforward, though I'm not sure what magic is creating those *-jvm artifacts.

The multiplatform plugin creates *-target artifacts per target. Funnily I expected it to create artifacts under workflow-core-jvm-jvm but instead just overwrote workflow-core-jvm with Kotlin artifacts until the artifact id was renamed to drop the jvm.

RBusarow
RBusarow approved these changes May 19, 2022
View reviewed changes
Copy link
Contributor

@RBusarow RBusarow left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for doing this!

@rjrjr rjrjr merged commit b5f7837 into square:main May 23, 2022
@bnvinay92 bnvinay92 deleted the vn/p1-gradle branch June 2, 2022 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RBusarow RBusarow RBusarow approved these changes

@zach-klippenstein zach-klippenstein Awaiting requested review from zach-klippenstein zach-klippenstein is a code owner

@steve-the-edwards steve-the-edwards Awaiting requested review from steve-the-edwards

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bnvinay92 @RBusarow @steve-the-edwards @rjrjr