-
-
Notifications
You must be signed in to change notification settings - Fork 375
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
Added support for 'mill init' from existing Gradle project #4363
Conversation
Thanks for the PR @ajaychandran ! To help simplify the review, could you flesh out the PR description with a summary of the code changes and any things worth noting? That would help me understand the PR much more quickly and accurately vs reverse engineering the intent from the diff Also, could you write an architecture summary of the entire
Now that it's getting more complex, such a summary would help understand this code going forward |
} | ||
|
||
def compact(tree: BuildTree): BuildTree = { | ||
println("compacting Mill build tree") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe leave a code comment here explaining what "compacting" means and why we want to do it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the logic here is kind of hard too read. I suggest either refactoring it to using a functional approach, or at least using Option.None
instead of null
s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what the does basically is to serve the merge
flag in the config classes: when combining the builds of all modules into a single "build.mill", the imports, the companion objects, and the inner
s (which are the Module
object contents) will be merged with their duplicate fields removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think refactoring this function to a functional programming approach will make it easier to read and maintain, aka getting rid of the var
s and foreach
s and using map
s,flatMap
s, distinct
s, toSet
s, etc. instead. See a related comment in Maven's BuildGen
for an example.
@@ -0,0 +1,125 @@ | |||
package mill.main.gradle; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did these model files come from? Were they copy-pasted from somethird-party code, or did you come up with them from scratch? If you came up with them, did you reference any documentation or code when writing them? We should leave a comment so people can see where these model files came from
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the models were written from scratch.
- Gradle requires every model to be a
Serializable
interface (or a runtime error is thrown). - The relevant plugins/settings were identified by searching the Gradle source repository.
- Tooling API usage was adapted from an old demo project.
- Latest Gradle API was obtained after posting on the forum.
I have added some docs and links but there isn't much information on the internals of the Gradle API.
@ShreckYe Did you find any documentation that could be useful here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess linking to the Javadocs of gradle.api.Project
, org.gradle.api.artifacts.Configuration
, org.gradle.api.artifacts.Dependency
, etc. should be enough. I can't find other types of tutorials or more elaborate documentation either.
* @param module build module | ||
*/ | ||
@mill.api.experimental | ||
case class Node[Module](dirs: Seq[String], module: Module) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would I be correct to say that we generally construct a Node[Something]
from the Maven/Gradle build, then transform that into a Node[BuildObject]
representing the Mill build, and then we serialize that to build.mill
/package.mill
files on disk?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
@lihaoyi |
@ajaychandran maybe lets move everything into the |
@lihaoyi wrote
But I would suggest to stick with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I have performed a full and rigorous code review as requested by Haoyi in #3962 (comment). First I'd like to say it's a job well done and I see there is a great effort put into this. Given the amount of code, I've left numerous comments. Please note:
-
In all these comments, I have proposed different alternative implementations, some of which may contradict each other. Please go through all the comments first and make decisions before taking a suggestion and start working on one.
-
Some suggestions reflect my personal coding style and may not be universally applicable. They may be unnecessary. If you have disagreements with these suggestions, please discuss and seek @lihaoyi's final advice.
@lihaoyi, it would be very helpful if you could provide your initial feedback on these comments when you have a chance.
There are 3 areas I'd like to highlight:
-
Scala is a language that prefers functional programming and Mill is a tool that favors immutability. Some imperative logic and logic using
null
s can be adapted into functional to be more readable and maintainable. -
More common code in both
BuildGen
clases can be extracted.Especially, please check out my "intermediate model" suggestion from the comments in "main/gradle/src/mill/main/gradle/BuildGen.scala" and consider its potential benefits.
-
In Scala, it's generally considered more idiomatic to use full words rather than abbreviated forms. I think these 3 abbreviations "interp", "cfg", "reps", especially, are better to be replaced by their full forms. If you accept this, please search and replace all their occurrences in the affected files.
@@ -202,6 +202,7 @@ object Deps { | |||
val mavenResolverTransportWagon = | |||
ivy"org.apache.maven.resolver:maven-resolver-transport-wagon:$mavenResolverVersion" | |||
val coursierJvmIndexVersion = "0.0.4-70-51469f" | |||
val gradleApi = ivy"dev.gradleplugins:gradle-api:8.11.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest is 8.12 but this is not important anyway.
I get it now that you are using "dev.gradleplugins:gradle-api" whose latest is 8.11.1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, the version is conventionally extracted into a constant:
val gradleApi = ivy"dev.gradleplugins:gradle-api:8.11.1" | |
val gradleApiVersion = "8.11.1" | |
val gradleApi = ivy"dev.gradleplugins:gradle-api:$gradleApiVersion" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, the version is conventionally extracted into a constant:
Looks premature to me.
** JUnit 4 | ||
** JUnit 5 | ||
** TestNG | ||
* configures multiple, compile and test, resource directories | ||
|
||
=== Command line arguments | ||
.name of generated base module trait defining project metadata settings | ||
|
||
The conversion and it's output (the generated Mill build files) can be customized using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conversion and it's output (the generated Mill build files) can be customized using | |
The conversion and its output (the generated Mill build files) can be customized using |
[#arguments] | ||
=== Command line arguments | ||
|
||
The conversion and it's output (the generated Mill build files) can be customized using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conversion and it's output (the generated Mill build files) can be customized using | |
The conversion and its output (the generated Mill build files) can be customized using |
/** Usage | ||
|
||
> rm build.mill # remove any existing build file | ||
|
||
> git init . | ||
> git remote add -f origin https://github.com/komamitsu/fluency.git | ||
> git checkout 2.7.3 # multi-module Java project that requires Java 16+ | ||
|
||
> ./mill init --base-module FluencyModule --jvm-id 16 | ||
converting Gradle build | ||
writing Mill build file to fluency-aws-s3/package.mill | ||
writing Mill build file to fluency-core/package.mill | ||
writing Mill build file to fluency-fluentd-ext/package.mill | ||
writing Mill build file to fluency-fluentd/package.mill | ||
writing Mill build file to fluency-treasuredata/package.mill | ||
writing Mill build file to build.mill | ||
init completed, run "mill resolve _" to list available tasks | ||
|
||
> ./mill __.compile | ||
compiling 9 Java sources to ...out/fluency-aws-s3/compile.dest/classes ... | ||
compiling 6 Java sources to ...out/fluency-aws-s3/test/compile.dest/classes ... | ||
compiling 27 Java sources to ...out/fluency-core/compile.dest/classes ... | ||
compiling 8 Java sources to ...out/fluency-core/test/compile.dest/classes ... | ||
|
||
> ./mill fluency-core.test # running all tests takes too long | ||
Test org.komamitsu.fluency.FluencyTest finished, ... | ||
Test org.komamitsu.fluency.validation.ValidatableTest finished, ... | ||
Test org.komamitsu.fluency.buffer.BufferTest finished, ... | ||
Test org.komamitsu.fluency.buffer.BufferPoolTest finished, ... | ||
Test org.komamitsu.fluency.flusher.FlusherTest finished, ... | ||
Test org.komamitsu.fluency.recordformat.MessagePackRecordFormatterTest finished, ... | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these files be converted into READMEs since they are just commands and results in comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These commands are automatically used as integration tests and the (expected) output will be asserted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Thanks for pointing out.
val url = escape(license.getUrl) | ||
val isOsiApproved = false | ||
val isFsfLibre = false | ||
val distribution = "\"repo\"" | ||
|
||
s"License($id, $name, $url, $isOsiApproved, $isFsfLibre, $distribution)" | ||
}.mkString("Seq(", ", ", ")") | ||
val versionControl = Option(model.getScm).fold(Seq.empty[String]) { scm => | ||
val repo = escapeOption(scm.getUrl) | ||
val conn = escapeOption(scm.getConnection) | ||
val devConn = escapeOption(scm.getDeveloperConnection) | ||
val tag = escapeOption(scm.getTag) | ||
|
||
Seq(repo, conn, devConn, tag) | ||
}.mkString("VersionControl(", ", ", ")") | ||
val developers = model.getDevelopers.iterator().asScala.map { dev => | ||
val id = escape(dev.getId) | ||
val name = escape(dev.getName) | ||
val url = escape(dev.getUrl) | ||
val org = escapeOption(dev.getOrganization) | ||
val orgUrl = escapeOption(dev.getOrganizationUrl) | ||
|
||
s"Developer($id, $name, $url, $org, $orgUrl)" | ||
}.mkString("Seq(", ", ", ")") | ||
val publishVersion = escape(model.getVersion) | ||
val publishProperties = | ||
if (cfg.publishProperties.value) { | ||
val props = model.getProperties | ||
props.stringPropertyNames().iterator().asScala | ||
.map(key => s"(\"$key\", ${escape(props.getProperty(key))})") | ||
} else Seq.empty | ||
|
||
val pomSettings = | ||
s"override def pomSettings = PomSettings($description, $organization, $url, $licenses, $versionControl, $developers)" | ||
val publishVersionSetting = | ||
s"override def publishVersion = $publishVersion" | ||
val publishPropertiesSetting = | ||
optional( | ||
"override def publishProperties = super.publishProperties() ++ Map", | ||
publishProperties | ||
) | ||
|
||
s"""$pomSettings | ||
| | ||
|$publishVersionSetting | ||
| | ||
|$publishPropertiesSetting""".stripMargin | ||
} | ||
|
||
private def escape(value: String): String = | ||
pprint.Util.literalize(if (value == null) "" else value) | ||
|
||
private def escapeOption(value: String): String = | ||
if (null == value) "None" else s"Some(${escape(value)})" | ||
|
||
private def optional(start: String, value: String): String = | ||
if (null == value) "" | ||
else s"$start$value" | ||
|
||
private def optional(construct: String, args: IterableOnce[String]): String = | ||
optional(construct + "(", args, ",", ")") | ||
|
||
private def optional( | ||
start: String, | ||
vals: IterableOnce[String], | ||
sep: String, | ||
end: String | ||
): String = { | ||
val itr = vals.iterator | ||
if (itr.isEmpty) "" | ||
else itr.mkString(start, sep, end) | ||
} | ||
|
||
private def resources(relPaths: IterableOnce[os.RelPath]): String = { | ||
val itr = relPaths.iterator | ||
if (itr.isEmpty) "" | ||
else | ||
itr | ||
.map(rel => s"PathRef(millSourcePath / \"$rel\")") | ||
.mkString(s"override def resources = Task.Sources { super.resources() ++ Seq(", ", ", ") }") | ||
val companions = cfg.depsObject.fold(SortedMap.empty[String, BuildObject.Constants])(name => | ||
SortedMap((name, SortedMap(namedIvyDeps.result() *))) | ||
) | ||
|
||
( | ||
companions, | ||
mainBomIvyDeps.result(), | ||
mainIvyDeps.result(), | ||
mainCompileModuleDeps.result(), | ||
mainCompileIvyDeps.result(), | ||
mainModuleDeps.result(), | ||
mainRunIvyDeps.result(), | ||
mainRunModuleDeps.result(), | ||
testModule, | ||
testBomIvyDeps.result(), | ||
testIvyDeps.result(), | ||
testModuleDeps.result() | ||
) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, this is another part that I think a functional approach would be more readable and maintainable. I did adapt this part based on your old implmentation in my implementation, and feel free to borrow the code. Also, I extracted a GenericBuildModels.AllDependencies
so we have more abstraction and code reuse.
baseModule: Option[String] = None, | ||
@arg(doc = "name of generated nested test module") | ||
@arg(doc = "version of custom JVM to configure in --base-module", short = 'j') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, "distribution and version".
os.exists(os.pwd / "build.gradle") || | ||
os.exists(os.pwd / "build.gradle.kts") || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can ignore the build scripts here as they are just for subprojects, like Mill's "package.mill". Just detecting settings scripts should be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Project JCommander does not have a settings script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Thanks for pointing out.
main/package.mill
Outdated
def moduleDeps = Seq(buildgen) | ||
def ivyDeps = Agg( | ||
build.Deps.gradleApi, | ||
build.Deps.logback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logback is runner
's dependency already, but you can leave it if you want this to be explicit.
val testIvyDeps = SortedSet.newBuilder[String] | ||
val testModuleDeps = SortedSet.newBuilder[String] | ||
|
||
val hasTest = os.exists(os.Path(model.getProjectDirectory) / "src/test") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above in Gradle's, this value is used only once deeply inside the if-blocks
* `--base-module` (`-b`): name of generated base module trait defining shared settings | ||
[source,sh] | ||
---- | ||
./mill init --base-module MyModule | ||
---- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example how to keep the source block in the list item.
https://docs.asciidoctor.org/asciidoc/latest/lists/continuation/#list-continuation
* `--base-module` (`-b`): name of generated base module trait defining shared settings | |
[source,sh] | |
---- | |
./mill init --base-module MyModule | |
---- | |
* `--base-module` (`-b`): name of generated base module trait defining shared settings | |
+ | |
[source,sh] | |
---- | |
./mill init --base-module MyModule | |
---- |
The refactoring looks good. Thanks @lihaoyi I think I addressed the remaining comments. Please let me know if anything is pending. |
packaging = null, | ||
pomParentArtifact = null, | ||
resources = Nil, | ||
testResources = Nil, | ||
publishProperties = Nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ajaychandran here as well, we should either leave a comment as to why packaging
/pomParentArtifact
/resources
/testResources
/publishProperties
are null/Nil? Or fill them out if they do not need to be so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added support for packaging
/ publishProperties
.
Added comments for
pomParentArtifact
: This is not directly available in the Gradle API.resources
/testResources
: These were skipped to avoid usingJavaPluginExtension.getSourceSets
; this would limit support to Gradle version7.1+
.
- Use MavenModel.Pom.packaging - Added comments for skipped settings - Removed unused MavenModel.Pom.organization() - Fix compiler warning
CI failures seem to be unrelated.
|
Thanks @ajaychandran , I gave CI a few kicks so hopefully it's just flakiness. |
This looks good to me for now I think, thanks again @ajaychandran for your contribution and @ShreckYe for the review. We can continue to improve this over time but I think this is complete enough to satisfy the original ticket @ajaychandran I will transfer the bounty using the same details we used before. @ShreckYe please email me (haoyi.sg@gmail.com) your international bank transfer details and I will make the payment |
This is from reviewing #4363. I couldn't comment on this because it was not part of the changes.
…4405) As commented in #4363 (comment). The PR got merged before I replied to this.
…yi#4363) Resolves com-lihaoyi#3962 by extending `mill init` with the ability to import a Gradle project. * handles deeply nested modules * configures dependencies for configurations: * implementation / api * compileOnly / compileOnlyApi * runtimeOnly * testImplementation * testCompileOnly * configures testing frameworks * supports Gradle plugins * [java](https://docs.gradle.org/current/userguide/java_plugin.html) * [maven-publish](https://docs.gradle.org/current/userguide/publishing_maven.html) Additionally, Maven support added in com-lihaoyi#3756 was improved. * shared logic moved out to module `buildgen` * added support for BOM dependencies * eliminated creation of redundant test modules * fixed `millSourcePath` for test module --------- Co-authored-by: Li Haoyi <haoyi.sg@gmail.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
This is from reviewing com-lihaoyi#4363. I couldn't comment on this because it was not part of the changes.
…om-lihaoyi#4405) As commented in com-lihaoyi#4363 (comment). The PR got merged before I replied to this.
Resolves #3962 by extending
mill init
with the ability to import a Gradle project.Additionally, Maven support added in #3756 was improved.
buildgen
millSourcePath
for test module