cbeust / kobalt

A Kotlin-based build system for the JVM.
Apache License 2.0
433 stars 60 forks source link

Handle comments in Build.kt better #332

Closed cbeust closed 7 years ago

cbeust commented 7 years ago

From @dmitry-zhuravlev:

Another problem that comments doesn't properly handled in Build.kt: Try specify this:

/*val build = buildScript{
    repos("dl.bintray.com/cbeust/maven")
    plugins("com.beust:kobalt-line-count:0.5")
}*/

val bs = buildScript {
    repos("bintray.com/kotlin/kotlin-eap-1.1")
    plugins("com.beust.kobalt:kobalt-line-count:0.18")
}

In 'tmp/kobaltXXXXXX/Build.kt' will be something like:

import com.beust.kobalt.*
import com.beust.kobalt.api.*
}

val bs = buildScript {
    repos("bintray.com/kotlin/kotlin-eap-1.1")
    plugins("com.beust.kobalt:kobalt-line-count:0.18")
}

and this will fail with:

\Build.kt:3:1 Expecting a top level declaration
com.beust.kobalt.KobaltException: Couldn't compile file: }
pabl0rg commented 7 years ago

@cbeust you probably got the idea for the buildScript, repos and plugins directives from Gradle which also has this magic in the build file.

Polyglot Maven handles this differently. It has a separate XML where plugins are defined. This means they don't have to pre-parse build files (written in Groovy, Scala, YAML, etc). It may also be more clear for users what is going on.

Would you consider changing how Kobalt adds repos/plugins? Is that possible after 1.0?

Would such a change allow you to completely eliminate pre-parsing of Build.kt?

It might also allow setting the Kotlin compiler version somewhere other than the user's ~/.config/kobalt/settings.xml which is not possible to change on a CI server.

cbeust commented 7 years ago

I think having all the information in the build file is convenient for the users instead of having to look at two different files (another reason why I don't like that Gradle requires settings.gradle for multiple projects) so I'm not sure I like this idea.

The only downside of buildScript is two phases of compilation, but 1) recompiling the build file is pretty rare and 2) that extra time is slowly going down with the efforts that JetBrains is making to optimize kotlinc (and soon we'll have a daemon in Kobalt too).

So... no, I don't think it's a good idea, but do you have other arguments in favor of that idea?

pabl0rg commented 7 years ago

My main arguments are 1. it's odd to parse the build file "manually" before compiling it and 2. it's counterintuitive that one can import symbols defined by a plugin that the IDE doesn't know about until after syncing.

What prompted me to comment on this issue was an error I got when trying to build your fork of u2020 with kobalt.

*****
***** ERROR Couldn't compile file: import com.beust.kobalt.plugin.android.*
/private/var/folders/gk/ml0b8m817tg8wg0my3819tnc0000gn/T/kobalt1697658814156817287/Build.kt:7:32 Unresolved reference: android
*****

I didn't know that having everything in a single file was a priority of Kobalt's. I have no strong opinion on the matter, but had gotten the opposite impression since Kobalt creates a who module for the build and adds several folders and files to a project (kobalt/src, kobalt/wrapper, .kobalt)

Thanks!

cbeust commented 7 years ago

It's not really a priority, I just find it convenient (and I think Gradle users like it too). I think the alternative you're proposing would result in a worse user experience.

dmitry-zhuravlev commented 7 years ago

@cbeust @pabl0rg Actually I see another problem with pre-processing Build.kt file. Every modification which add/delete any lines inside Build.kt file will break debug ability. So debug will partly work in some breakpoints maded in IDE in "original" Build.kt because "original" Build.kt not the "real" one.

cbeust commented 7 years ago

What's the problem exactly: the file can't be found, the lines do not match, something else?

dmitry-zhuravlev commented 7 years ago

Lines do not match...