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

Redefines the Gradle build strategy #175

Merged
merged 26 commits into from
Jul 7, 2023
Merged

Redefines the Gradle build strategy #175

merged 26 commits into from
Jul 7, 2023

Conversation

juanpedromoreno
Copy link
Contributor

This PR could be considered a WIP, even if it's finally merged. Further iterations and optimizations will be required.

Initially, the objective of this PR is to decrease the build time. For that reason:

  • I'm creating a pipeline matrix with the different platforms: jvm, js, linuxX64, macosX64, mingwX64 (notice I've removed macosArm64 due to some incompatibilities).
  • I split the constructions and tests into multi-platform and single-platform (JVM). This could be done by creating the groups at the Gradle level, which could be the subject of further improvements.
  • Regarding JS, I'm not building for browser and node, which might be required for certain modules, but I left that out of this work.

Result

Hopefully, the build times should be reduced by more than 10 minutes.

Only js platform

Platform is not necessary for single-platform

Unifies gradlew commands

Removes macosarm64

Improves build commands

on pull_request
check the code formatting including a new step
@juanpedromoreno juanpedromoreno force-pushed the build-platform-matrix branch from 28adab1 to 06d331a Compare June 8, 2023 07:56
@juanpedromoreno juanpedromoreno force-pushed the build-platform-matrix branch from 40c2e20 to 1659cb9 Compare June 8, 2023 09:07
@juanpedromoreno juanpedromoreno force-pushed the build-platform-matrix branch from 1659cb9 to 4deb864 Compare June 8, 2023 09:10
@juanpedromoreno juanpedromoreno force-pushed the build-platform-matrix branch from 88d7d67 to 875a241 Compare June 8, 2023 13:41
@juanpedromoreno juanpedromoreno force-pushed the build-platform-matrix branch from 875a241 to 3458b92 Compare June 8, 2023 13:42
@juanpedromoreno juanpedromoreno marked this pull request as ready for review June 8, 2023 13:52
@juanpedromoreno juanpedromoreno force-pushed the build-platform-matrix branch from 4f9ce2c to 037233a Compare June 8, 2023 15:03
Copy link
Contributor

@Yawolf Yawolf left a comment

Choose a reason for hiding this comment

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

Is this still a WIP PR? It would be very beneficial to merge this asap 🙂

@diesalbla
Copy link
Contributor

@Yawolf Is this compatible with the #197 , or would that take it far apart?

@raulraja
Copy link
Contributor

raulraja commented Jul 4, 2023

@juanpedromoreno do we still need this, what can we do to get it merged?

@juanpedromoreno
Copy link
Contributor Author

I think @franciscodr and @nomisRev were testing a different approach. I don't want to make it conflict with theirs, but I agree with you; let's decide to speed up the builds and go for one. Happy to close this one if it doesn't make sense 👍

serras
serras previously approved these changes Jul 6, 2023
Copy link
Contributor

@serras serras left a comment

Choose a reason for hiding this comment

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

Nice trick to handle for multi- and single-platform targets!

@raulraja
Copy link
Contributor

raulraja commented Jul 6, 2023

@franciscodr @nomisRev can this be merged?
@juanpedromoreno I see a hardcoded list of modules there assumed to be multiplatform. Can we get this dynamically from the project build? When adding new modules, I see how this may not work as intended if they are not added to this list.

@juanpedromoreno
Copy link
Contributor Author

@Yawolf was going to look at this. Please, Yago, look also at that particular comment from @raulraja . Thanks

I see a hardcoded list of modules there assumed to be multiplatform. Can we get this dynamically from the project build?

@Yawolf
Copy link
Contributor

Yawolf commented Jul 6, 2023

@Yawolf was going to look at this. Please, Yago, look also at that particular comment from @raulraja . Thanks

I see a hardcoded list of modules there assumed to be multiplatform. Can we get this dynamically from the project build?

@raulraja I changed that by doing this:
https://github.com/xebia-functional/xef/pull/175/files#diff-c0dfa6bc7a8685217f70a860145fbdf416d449eaff052fa28352c5cec1a98c06R18

Let me know if that makes sense to you 🙂

So far, ./gradlew build takes around 7m:

BUILD SUCCESSFUL in 7m 36s
275 actionable tasks: 263 executed, 12 up-to-date

The main branch, in my local, takes more than 9 minutes

BUILD SUCCESSFUL in 9m 32s
264 actionable tasks: 254 executed, 10 up-to-date

@juanpedromoreno
Copy link
Contributor Author

@Yawolf Shall we filter for those that are multi-platform somehow?

@raulraja
Copy link
Contributor

raulraja commented Jul 6, 2023

@raulraja I changed that by doing this: https://github.com/xebia-functional/xef/pull/175/files#diff-c0dfa6bc7a8685217f70a860145fbdf416d449eaff052fa28352c5cec1a98c06R18

Yes, that makes sense but I think that not all subprojects returned are actually multiplatform, that returned list needs to be partitioned somehow based on that.

@juanpedromoreno
Copy link
Contributor Author

Right,

Maybe something like this @Yawolf ?

fun Project.isMultiplatformModule(): Boolean {
    val kotlinPluginId = "org.jetbrains.kotlin.multiplatform"
    return plugins.hasPlugin(kotlinPluginId)
}

val multiPlatformModules = project.subprojects.filter { it.isMultiplatformModule() }

@Yawolf
Copy link
Contributor

Yawolf commented Jul 6, 2023

Right,

Maybe something like this @Yawolf ?

fun Project.isMultiplatformModule(): Boolean {
    val kotlinPluginId = "org.jetbrains.kotlin.multiplatform"
    return plugins.hasPlugin(kotlinPluginId)
}

val multiPlatformModules = project.subprojects.filter { it.isMultiplatformModule() }

Yeah! Something like that! Thanks! 😄

@Yawolf
Copy link
Contributor

Yawolf commented Jul 6, 2023

Right,
Maybe something like this @Yawolf ?

fun Project.isMultiplatformModule(): Boolean {
    val kotlinPluginId = "org.jetbrains.kotlin.multiplatform"
    return plugins.hasPlugin(kotlinPluginId)
}

val multiPlatformModules = project.subprojects.filter { it.isMultiplatformModule() }

Yeah! Something like that! Thanks! 😄

@juanpedromoreno for some reason I don't understand, project.plugins returns an empty list. I've tried with project.project.plugins, and many others. At the end I end up doing this:

fun isMultiplatformModule(project: Project): Boolean {
  val kotlinPluginId = "libs.plugins.kotlin.multiplatform"
  return project.buildFile.readText().contains(kotlinPluginId)
}

And that returns this:

[xef-core, xef-filesystem, xef-gpt4all, xef-kotlin, xef-openai, xef-tokenizer]

Which is correct, if I'm not wrong.

@juanpedromoreno
Copy link
Contributor Author

👍 Great, that list looks correct, yes.

@Yawolf
Copy link
Contributor

Yawolf commented Jul 7, 2023

In my local, with these changes, it takes:

BUILD SUCCESSFUL in 6m 39s
264 actionable tasks: 250 executed, 14 up-to-date

While the main branch takes:

BUILD SUCCESSFUL in 6m 44s
264 actionable tasks: 250 executed, 14 up-to-date

In GitHub actions, this PR took 7m 36s to build, while PR #130 took 8m 59s. So there's a slight performance improvement in these changes.

@raulraja @juanpedromoreno

@juanpedromoreno
Copy link
Contributor Author

Sounds good, thanks @Yawolf

@Yawolf Yawolf merged commit 79979f4 into main Jul 7, 2023
@Yawolf Yawolf deleted the build-platform-matrix branch July 7, 2023 11:55
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.

6 participants