Skip to content
This repository has been archived by the owner on Jan 2, 2022. It is now read-only.

feat: modules separation #2

Merged
merged 20 commits into from
Aug 28, 2020
Merged

Conversation

programadorthi
Copy link
Contributor

Sometimes we need apply some configurations to subprojects checking for project type (Application, Android Library or Java Library). For instance, when we have modules Java and Android inside same project, the only way to run both unit tests is calling test or check task. The problem here is that if your Android module has many BuildTypes or combinations with ProductFlavors, test or check task will run for each BuildType/ProductFlavor.

BuildTypes: debug and release
Tasks will be executed:

testDebugUnitTest 
testReleaseUnitTest

Things become worst if we have ProductFlavors: development, sandbox, production
Tasks will be executed:

testDevelopmentDebugUnitTest 
testDevelopmentReleaseUnitTest
... and so on for each flavor

So, this pull request is not to solve test or check task only. But to deliver a new way to solve other things related to similar contexts.

  1. Libraries.kt or broken in JavaModules.kt and AndroidModules.kt and now we know what projects are java or android only;
  2. Instead of have inner paths and modules together in an unique constant, we have now inner objects to create a logic between project path. For instance the path: :commons:utils generate:
// Before
object Libraries {
    const val COMMONS_UTILS = ":commons:utils"
    // allAvailable
}
// Now
object JavaLibraries {
    object Commons {
        const val UTILS = ":commons:utils"
        // allAvailable
    }
}

Each depth in the path, except last, generate a inner object.

  1. allAvailable now is a HashSet instead of List to avoid O(N²) in operatios that requires check contains.
  2. deepSearch was replaced with walkTopDown from FileTreeWalk. The main reasons are that we can use the maxDepth function to avoid walk until the last file in depth and onEnter to avoid enter in folders that doesn't contains build.gradle(.kts) files. MaxDepth can improve perfomance when we have a fixed structure.
maxDepth | rootFolder
.
1        ├── build.gradle
         ├── settings.gradle
         ├── app
2        │   ├── build.gradle.kts
         │   └── src
         ├── buildSrc
2        │   ├── build.gradle.kts
         │   └── src
         ├── common
         │   ├── core
3        │   │   ├── build.gradle
         │   │   └── src
         │   └── utils
3        │       ├── build.gradle.kts
         │       └── src
         ├── features
         │   ├── home
3        │   │   ├── build.gradle
         │   │   └── src
         │   └── login
3        │       ├── build.gradle
         │       └── src

So, the max depth to find a build.gradle is 3. Passing this value to the magic-modules extension will avoid down more deep.

ktlint fixes
created a data class to avoid mistakes and help with maintenance
created a data class to avoid mistakes and help with maintenance
replaced deep search with walkTopDown function and added max depth extension field to improve looking for build scripts

inner folders/paths now are separated by inner objects instead a constant using underscore
Copy link
Member

@ubiratansoares ubiratansoares left a comment

Choose a reason for hiding this comment

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

Thanks for pushing this contribution 🙂

I have some questions for you :

[...] So, this pull request is not to solve test or check task only. But to deliver a new way to solve other things related to similar contexts.

Could you elaborate more around this use case? To be quite honest here, I don't see the correlation between constants auto-mapped at buildSrc and Gradle task names

allAvailable now is a HashSet instead of List to avoid O(N²) in operations that requires check contains.

Why would be this optimisation needed at all? If you have a multi-hundred module build - ie, an already HUGE project - most likely the current version of this plugin parses the project structure in a few dozen milliseconds, which is pretty much doable in the overall build time for most of project tasks. In the other* hand, I felt the implementation driven by such HashSet looks harder to read and understand compared to the actual one ...

Do you have some concrete use cases and/or benchmarks that would make it clear why this optimisation is needed?

deepSearch was replaced with walkTopDown from FileTreeWalk. The main reasons are that we can use the maxDepth function to avoid walk until the last file in depth and onEnter to avoid enter in folders that doesn't contains build.gradle(.kts)

While this optimisation is quite handy, it is also possible to be achieved by fine tuning the current DSF, right? 😄

In addition to the question above, I'm curious to learn why did you remove yourself from the contributors for this project... It is my pleasure to have you listed here, given your previous PR 🙂

Last, but not least : seems the latest commit you pushed is missed a lost Github Check from Codebeat, could you please have a look on that (maybe pushing more commits, or squashing the existing ones)?

var rawLibraryPlugins: List<String> = listOf(
"com.android.library", "kotlin", "com.android.dynamic-feature", "jvm"
"com.android.library", "com.android.dynamic-feature"
Copy link
Member

Choose a reason for hiding this comment

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

This is a very good addition 💯

override fun hashCode(): Int {
return value.hashCode()
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This logic looks like a bit convoluted, specially for equals

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was generated by IDE. I agree about convolution. This is a generated code that I avoid to use. It was generated while I was testing the write Tree. I'll change.

@programadorthi
Copy link
Contributor Author

programadorthi commented Aug 22, 2020

I knew that will generate some doubts. 😄

Could you elaborate more around this use case? To be quite honest here, I don't see the correlation between constants auto-mapped at buildSrc and Gradle task names

The correlation here is when we need link projects to tasks in some manner. Mainly when we need create custom or link Gradle tasks that depends other modules grade task.
As we know to run Gradle task using terminal, we need the full project path that is the same used in the settings include function and that has become a constant inside buildSrc.
So, we can now do something like following scenarios:

  1. Some modules need to be published:
val projectsToPublish = listOf(
    JavaModules.PathA.MODULEA, // :patha:modulea
    JavaModules.PathA.MODULEB, // :patha:moduleb
    LibraryModules.PathB.MODULEA, // :pathb:modulea
    LibraryModules.PathB.MODULEC, // :pathb:modulec
)

// This tasks publish all modules associated using dependsOn
tasks.register("publishLibraries") {
    dependsOn(
        *projectsToPublish
            .map { modulePath -> "$modulePath:publish" }
            .toTypedArray()
    )
    doLast {
        println(">> All libraries published <<")
    }
}
  1. When run Android (Library|Dynamic-Feature) variant unit test, should run java module unit test too:
    This scenario is common on CI when DevOps use ./gradlew test or ./gradlew check in the test step. They are overkill tasks because will run many things that just increase the CI flow time.
    Solving with one task only: ./gradlew runUnitTests
val javaModules = listOf(
    JavaModules.PathA.MODULEA, // :patha:modulea
    JavaModules.PathA.MODULEB, // :patha:moduleb
)

val librariesModules = listOf(
    LibraryModules.PathB.MODULEA, // :pathb:modulea
    LibraryModules.PathB.MODULEC, // :pathb:modulec
)

// This task runs just specific unit test tasks
tasks.register("runUnitTests") {
    fun mapPathWithoutBuildType(projectPath: String) = "$projectPath:test"

    fun mapPathWithBuildType(projectPath: String) = "$projectPath:testReleaseUnitTest"

    dependsOn(
        *(
            javaModules.map(::mapPathWithoutBuildType) +
                librariesModules.map(::mapPathWithBuildType)
            ).toTypedArray()
    )

    doLast {
        println(">> All Unit test passed <<")
    }
}

And so on...

Do you have some concrete use cases and/or benchmarks that would make it clear why this optimisation is needed?

Well, about HashSet, I confess that is a specific context. But I think that could happen with anyone.
I need to configure some subprojects and I need to know when it is a Java or Android project. So I do:

subprojects {
    // HashSet helps here using contains insteand of other List operations
    if (JavaModules.allAvailable.contains(this.path) || LibraryModules.PathA.allAvailable.contains(this.path)) {
        // apply project configurations
    }
}

While this optimisation is quite handy, it is also possible to be achieved by fine tuning the current DSF, right? smile

Yes, I totally agree. But walkTopDown is a built-in function and offer other functions like onEnter that help us to improve search. I was using it before find your project 😄

In addition to the question above, I'm curious to learn why did you remove yourself from the contributors for this project... It is my pleasure to have you listed here, given your previous PR slightly_smiling_face

This was a mistake. I'll revert 😄

Last, but not least : seems the latest commit you pushed is missed a lost Github Check from Codebeat, could you please have a look on that (maybe pushing more commits, or squashing the existing ones)?

I'll check what happened.

revert README contribution section
@ubiratansoares
Copy link
Member

@programadorthi Thanks for the explanations. My comments about

About the correlation between module names and custom Gradle tasks

The examples you provided are quite clear and the use case is quite legit. I think this change is quite welcome then.

About Hashset implementation to store Module names

Unfortunately, I don't see this a strong use case here. Looks really handy for your specific needs, but too specific for the purpose of this plugin and for the overall semantics it provides.

About walkTopDown optimisation

Imho, such optimisation is not related with the main use case you are trying to tackle here; but you can keep it as you wish.

@ubiratansoares
Copy link
Member

Btw, I just removed the Codebeat integration with this project for now, since it looks quite unstable regarding the CI checks

@programadorthi
Copy link
Contributor Author

programadorthi commented Aug 25, 2020

Unfortunately, I don't see this a strong use case here. Looks really handy for your specific needs, but too specific for the purpose of this plugin and for the overall semantics it provides.

I understand. I'm was analyzing and I think that HashSet is a specific case. I'll revert to List.

Imho, such optimisation is not related with the main use case you are trying to tackle here; but you can keep it as you wish.

About this case, I made a mistake. I forgot to move time measurement from apply function to settingsEvaluated because heavy operations was moved to wait extension parameters. This way, we are computing a time that is almost 0 milliseconds. The time measurement should be moved to settingsEvaluated body that is an async function.

I'll keep for now because in a project with 50 modules, when all modules is organized in the root fold, without deepSearch was better than with it. First sync was 100 milliseconds with walkTopDown instead of 2090 milliseconds with deepSearch. Of course, deepSearch can be improved and measured time can variate between hardware configurations.

Btw, I just removed the Codebeat integration with this project for now, since it looks quite unstable regarding the CI checks

Well, I was looking for the reason that has stopped codebeat validation, but I think the reason was not my PR.

@ubiratansoares
Copy link
Member

@programadorthi Looks great! Please, make sure your fork is up-to-date with current master branch (I've added one commit yesterday).

@ubiratansoares ubiratansoares merged commit 9f1ea7b into dotanuki-labs:master Aug 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants