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

Redo Build Logic #744

Closed
jGleitz opened this issue Dec 23, 2020 · 14 comments
Closed

Redo Build Logic #744

jGleitz opened this issue Dec 23, 2020 · 14 comments
Assignees
Milestone

Comments

@jGleitz
Copy link
Collaborator

jGleitz commented Dec 23, 2020

This project’s build logic is, to put it mildly, involved. I think it could be improved upon.

I have two major criticisms:

  • The build logic relies a lot on the magic Gradle allows with Groovy (for example: setting properties on project.ext that are then read by other projects). Even though I’ve read through the build files a lot by now, I still don’t understand how it all works together. This is worsened by the fact that no help by the IDE is possible and that the logic relies on the robstoll Gradle plugins, which do a lot of magic and pre-configuration under the hood.
  • Not all task dependencies are set up properly, (which, from my analysis is why Build Improvements #743 fails)

I can’t promise I’ll find the time, but if I do, I’d like to improve the build logic of this project. I created this ticket so that we can agree on what a better build setup should look like.

I propose to:

  • Rewrite the build logic in Kotlin. This gives us a lot more help from the IDE.
  • Use standard plugins wherever possible. Try to remove the robstoll plugins. (This last point will probably not sound good to you. I am not suggesting to duplicate logic all over Atrium. I am, however, suggesting that configuration (as opposed to functionality) should be contained entirely in this project. We have seen in the past that it is difficult to change the robstoll Gradle plugins because multiple projects rely on it. From my point of view, duplicating some configuration over some projects is better than having a build logic that is both hard to understand and, even worse, hard to evolve).
  • At a later point: Make the logic generator a Gradle plugin on its own that lives in a different repository. This allows third parties to use it as well.

What’s your opinion on this?

@jGleitz
Copy link
Collaborator Author

jGleitz commented Dec 23, 2020

One example of what I think can be improved: The main build file declares certain types of projects:

def commonProjects = getCommonProjects()
def jsProjects = getJsProjects()
def jvmProjects = getJvmProjects()
def multiplatformProjects = commonProjects + jsProjects + jvmProjects

The get... methods come from the tuttell plugin. So no easy way to find out how they work (they work based on naming conventions).

Next, we have:

configureCommonProjects()
configureAndroidProjects()
configureJsProjects()
configureJvmProjects()

These functions, once again, jump into the tutelli plugins. Afterwards, we do some more configuration in the root build file.

Now imagine being in a subproject and trying to figure out how this project is built. One would first have to find out that the root build file does most of the configuration, then find out as which type the project gets classified and then collect all the bits where this type gets configured – including code from another repository.

I think this could be improved by having reusable bits of configuration (for example from a buildSrc project, maybe even from an external project) that are explicitly included by every project. This will, of course, lead to somewhat longer files and some duplication. However, instead of making maintenance worse, it would make maintenance easier because

a) we can see directly where the configuration is coming from.
b) if we name the configuration functions well, we can see directly, without reading the functions themselves, what is being configured.

@robstoll
Copy link
Owner

I planned to migrate to the new kotlin MPP plugin in either v0.16.0 or v0.17.0 and would therefore be happy if you could help out. I guess the examples in your second comment will then no longer be a problem.

Use standard plugins wherever possible. Try to remove the robstoll plugins.

Surprise... good with me, I actually planned to do it as well but... I also planned to factor out some logic into an own plugin (or buildSrc fine with me as well) at a later point. In any case, I would keep the following plugins for now

  • tutteli-gradle-kotlin-module-info (unless Kotlin supports module-info.java somehow by now)
  • tutteli-gradle-settings (IMO totally fine to have settings.gradle and not settings.gradle.kts)

Might be I I am going to re-use tutteli-gradle-project-utils after your changes but we will see.

Rewrite the build logic in Kotlin. This gives us a lot more help from the IDE.

I would tackle this in a separate step but if you feel like it you can also do everything at once

At a later point: Make the logic generator a Gradle plugin on its own that lives in a different repository. This allows third parties to use it as well.

I would also do this at a later stage. The generator works only due to some conventions and I don't want to expose it currently (as it would become public API).

@jGleitz
Copy link
Collaborator Author

jGleitz commented Dec 23, 2020

I also planned to factor out some logic into an own plugin

Factoring out logic is always good! Factoring out configuration is bad, from my point of view.

tutteli-gradle-kotlin-module-info

This seems to be mainly about logic (how to build module-info.java files). So as long as there is no other de-facto-standard plugin for this, I am also in favour of using it!

tutteli-gradle-settings

I’d have to see how the files would look like without it to have an opinion on this.

I would tackle this in a separate step but if you feel like it you can also do everything at once.

For me, it will be easier to do it at once. I don’t know the Gradle API by hand, so IDE completion is very welcome for me.


It seems that we generally agree on what improvements to make. Great! I’ll tackle this if I find time (no promises, sorry!).

@robstoll
Copy link
Owner

In case you have time, then it probably makes sense if you do #641 as well (there is a closed PR which you could resurrect)

@jGleitz
Copy link
Collaborator Author

jGleitz commented Dec 23, 2020

Why is atrium-scala2 not part of the atrium build?

@robstoll
Copy link
Owner

robstoll commented Dec 24, 2020

Because:

  • intellij bug, cannot build scala if part of Kotlin multi-project build
  • some people use vscode + metals for scala and vscode does not support Kotlin
  • as info, there is a composite build in misc/tools, so it is still part of Atrium's CI pipeline

@robstoll
Copy link
Owner

robstoll commented Jan 7, 2021

Since I am going to refactor bc-tests and this will affect settings.gradle as well, I am going to rewrite it to setttings.gradle.kts

@jGleitz
Copy link
Collaborator Author

jGleitz commented Jan 7, 2021

Cool thing! If this works well, maybe we can address this issue one step at a time.

@robstoll
Copy link
Owner

robstoll commented Jan 8, 2021

Doesn't work well at all. Intellij is constantly throwing errors (its basically this issue: https://youtrack.jetbrains.com/issue/KT-42769) until it stops monitoring fatal errors. The effect of it is, that syntax highlighting does not work well or is simply wrong. In this sense it is not much better than groovy in the end. Yet, I hope this is going to be fixed soon and thus I am still moving to .kts

@robstoll
Copy link
Owner

robstoll commented Jan 8, 2021

It looks more and more like only a big bang transition to the new MPP plugin will work (shame on Jetbrains, that's really a migration path I don't want for Atrium. That many issues I already stumbled over and had to find workarounds 😞). Hitting the following issue now: https://youtrack.jetbrains.com/issue/KT-32811

Edit maybe we are lucky and we can do the transition in multiple steps nonetheless by not yet setting up bc-tests for js

@jGleitz
Copy link
Collaborator Author

jGleitz commented Jan 8, 2021

This is unfortunate. A big bang redo of the build logic would surely be a big improvement, but is also difficult and involved to pull off.

@robstoll
Copy link
Owner

robstoll commented Apr 19, 2021

@Sxtanna I have pathed the way so that you can continue with the migration. I have migrated atrium-core to the new MPP plugin, thought without the JS target as this one can only be done in a big-bang. See f21b2a0#diff-e129df9e5134b2df3a6dd7417b8e0d6890a16212bbf423505ceeecfc80e62d5d to have an overview what I have done.

There are a few specialities which you need to be aware of:

  • you need to add the module name which you want to migrate to newMultiplatformProjectNames in /build.gradle (many things will be already configured this way) before you start with the migration
  • please use build.gradle.kts in the new modules and no longer build.gradle
  • you should not move sources to avoid merge conflicts, I re-configured src and resources dirs to still use the same location as the old MPP structure. We will change this once we have migrated everything
  • you don't need to prepare the JS target, we will do this once we have migrated all common/jvm targets
  • point your PR against migrate-to-new-mpp and not against master (also create your branch from this upstream branch)

Moreover I suggest:

  • migrate bottom up the module dependency graph. This will cause the least problems. i.e. in the following order:
    • translations
    • logic
    • verbs
    • verbs-internal
    • apis
    • bundles
    • specs

@robstoll
Copy link
Owner

Good news, it looks like JetBrains has a workaround for the JS problem:
https://youtrack.jetbrains.com/issue/KT-40226

@robstoll
Copy link
Owner

was able to re-introduce the js target. last point is publishing, going to close this issue already

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants