Skip to content

Conversation

ajaychandran
Copy link
Contributor

@ajaychandran ajaychandran commented Jul 2, 2025

This PR rewrites importer implementations using a new architecture that is easy to maintain and extend.

Motivation

While working on adding support for cross projects, the build models used in the existing implementations turned out to be insufficient for the use-case.
This prompted work on an alternate architecture that culminated in this pull request.

Solution

New data models are introduced to define a build specification. This includes an ADT whose members have a one-to-one correspondence with module types defined within Mill. Importer implementations were updated to target the new models.

New and dropped features

  • Added support for converting cross-platform and cross version SBT projects.
  • Added support for configuring ErrorProneModule for Gradle and Maven projects.
  • Added support for configuring jvmId.
  • Dropped support for mapping resources.
  • Redundant command line options were removed.

Other improvements

  • Fixed mapping for PomSettings in Maven conversion.
  • Fixed mapping for artifactName in SBT conversion.
  • Publish is skipped for Maven modules where it is explicitly disabled. Example, testsuite-osgi module in netty project.
  • Prevent runtime failure by handling Maven ModelBuildingException.

Limitations

  • As before, build customizations are not supported. This includes custom sources, custom Gradle configurations, and Maven profiles.
  • SBT cross-platform support is limited to projects using sbt-crossproject plugin.

Layout changes

  • Added module libs.init.main.buildgen.api for the new data models. The module is cross built against Scala 2.12 for use within SBT.
  • Added libs.init.main.gradle.exportplugin to isolate the classpath containing the Gradle plugin.

Testing

  • Updated integration tests to use golden testing for checking settings.
  • Added integration.manual[migrating] module to develop and test the conversions against real world projects.

Summary of fixes/enhancements made to improve support for test projects.

  • Fixed init error in path handling for spring-ai.
  • Fixed init/configuration error by removing custom resources for byte-buddy/joda-beans.
  • Added spring-boot-dependencies BOM dependency for microconfig.
  • Generated ErrorPluginModule configurations for mockito.
  • Generated cross-platform/version configurations for Airstream, cats, enumeratum, fs2, nscala-time, refined, scala-logging, scopt, scrypto.

Future work

  • Add support for converting Kotlin/Android projects.
  • Add support for converting SBT projects that use the sbt-projectmatrix plugin.

@ajaychandran
Copy link
Contributor Author

@lihaoyi As per the Maven POM schema, the description, organization and url fields are not mandatory. Is there a reason why these are required in Mill PomSettings?

@lihaoyi
Copy link
Member

lihaoyi commented Jul 3, 2025

No reason I'm aware of, we can make them optional

@lefou
Copy link
Member

lefou commented Jul 3, 2025

I think the Maven Central validator rejects projects without these fields. Although it's specific to Maven Central, having these fields mandatory avoids some late pitfalls, when publishing artifacts.

We only use PomSettings for publishing/sharing, so having some minimal standards on meta-data is probably not bad.

@ajaychandran
Copy link
Contributor Author

ajaychandran commented Jul 3, 2025

We could allow null values and handle in the render method. This would satisfy the empty use-case without requiring changes in user code.

@ajaychandran
Copy link
Contributor Author

ajaychandran commented Jul 29, 2025

A new SBT importer implementation has been added that can import projects that use sbt-crossproject plugin for cross platform development. The implementation also generates cross modules for cross Scala versions.

This was tested against 12 projects and all but 2 were successfully imported.

  • scala/scala3: Multiple SBT projects use the same base directory.
  • scalapb/ScalaPB: Uses sbt-projectmatrix for cross development.

One of the tests can be run using the following command:

./mill standalone.migrate[sbt].testOnly mill.standalone.MillInitAirstreamTests

@ajaychandran
Copy link
Contributor Author

ajaychandran commented Jul 29, 2025

@lihaoyi

I am going to be working on adding support for some more settings such as jvmId and generating base trait feature.

What are your thoughts on supporting more plugins? We already have tests that require the following:

  • sbt-projectmatrix
  • sbt-play
  • ScalablyTypedConverterGenSourcePlugin

@lihaoyi
Copy link
Member

lihaoyi commented Jul 30, 2025

@ajaychandran update the PR title and description to summarize the motivation, major changes, how it integrates with the existing codebase, and testing strategy. It's a large PR and that would make it easier to review

Comment on lines 88 to 91
ps.println()
ps.print("def repositories = super.repositories() ++ ")
ps.print("Seq(")
ps.print(literalize(repositories.head))
Copy link
Member

@lihaoyi lihaoyi Jul 30, 2025

Choose a reason for hiding this comment

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

Please use string interpolation for this and all similar code snippets so it's not so verbose

import mill.constants.OutFiles.{millDaemon, millNoDaemon, out}
import os.{Path, ProcessInput, ProcessOutput, Shellable}

class StandaloneTester(
Copy link
Member

@lihaoyi lihaoyi Jul 30, 2025

Choose a reason for hiding this comment

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

Why does this need to be different from normal IntegrationTester? We should make these normal integration tests using the normal IntegrationTester utilities

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 doesn't have the same requirements as IntegrationTester. The workspace creation and initialization happens outside the tester.
Given that this PR proposal will take some time, I chose to isolate this to avoid any conflicts when re-basing.

Copy link
Member

Choose a reason for hiding this comment

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

What workspace creation and initialization do we expect with IntegrationTester? If run on an empty folder, the workspace starts empty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IntegrationTester clears the workspace and copies resources. I suppose this can be circumvented with an empty resource folder.

I am not opposed to the idea of sharing existing code. I just kept all new code isolated initially. I will merge the testkit classes and move the tests to integration/manual.

@@ -0,0 +1,33 @@
package build.standalone
Copy link
Member

Choose a reason for hiding this comment

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

Let's put this in the integration/manual/, indicating that these are tests meant to be run manually but won't be run in CI, but are otherwise normal integration tests

@@ -0,0 +1,108 @@
package mill.init.migrate
Copy link
Member

@lihaoyi lihaoyi Jul 30, 2025

Choose a reason for hiding this comment

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

How does this file related to the code we already have in libs/init/buildgen/src/mill/main/buildgen/ir.scala? Does it replace it entirely, or does it re-use parts of the existing model?

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 is meant to replace it.
In essence, it is a refactoring of the existing model that organizes settings into groups that correspond to actual module types.
Also, the IR types with the rendered code are not required and do not exist in the new model. This was primarily introduced, by me, for file merging. The new implementation achieves the same in a sane way.

"org.testng" -> "TestModule.TestNg",
"org.scalatest" -> "TestModule.ScalaTest",
"org.specs2" -> "TestModule.Specs2",
"com.lihaoyi" -> "TestModule.Utest",
Copy link
Member

Choose a reason for hiding this comment

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

This seems a bit imprecise. For example a lot of people use com.lihaoyi::os-lib but don't use utest. Could we match on a whitelist of artifact names (or prefixes, ignoring the _{scala-version}) as well?

Copy link
Member

Choose a reason for hiding this comment

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

Also, this seems to duplicate logic we already have in BuildgenUtil. We should consolidate it and avoid copy-pasting code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is the same as the list in BuildGenUtil.
Given that this is a rewrite of the core design, I have kept the new implementation separate. If the proposal is approved, this can be consolidated.


import upickle.default.{ReadWriter, macroRW}

case class Tree[+A](root: A, children: Seq[Tree[A]] = Nil) {
Copy link
Member

Choose a reason for hiding this comment

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

How is this different from libs/init/buildgen/src/mill/main/buildgen/Tree.scala?

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's in Scala 2 and the unused features are removed.
But basically it is the same and only exists because the new implementation is isolated.

}

def renderSbtPlatformModule =
s"""trait SbtPlatformModule extends SbtModule { outer =>
Copy link
Member

@lihaoyi lihaoyi Jul 30, 2025

Choose a reason for hiding this comment

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

How is this different from SbtModule with PlatformScalaModule? If it's necessary, we should put it (and CrossSbtPlatformModule) in libs/scalalib/src/mill/scalalib/ rather than codegen-ing it every import

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SbtModule resets sources

  override def sources = Task.Sources("src/main/scala", "src/main/java")

and PlatformModuleBase operates on sourceFolders.

  override def sourcesFolders: Seq[os.SubPath] = super.sourcesFolders.flatMap {
    source => Seq(source, source / os.up / s"${source.last}-${platformCrossSuffix}")
  }

Are these compatible in any scenario?

This is being used to replicate the layout used by sbt-crossproject plugin. But now we also have sbt-projectmatrix for cross development which seems to use a completely different structure. Is it okay to add a trait to scalalib per plugin?

Copy link
Member

Choose a reason for hiding this comment

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

We should make SbtModule use sourceFolders as well, just for consistency with the rest.

I think having traits in scalalib for each of them is fine as long as they're meaningful and well documented .

@@ -0,0 +1,225 @@
package mill.init.migrate.sbt
Copy link
Member

Choose a reason for hiding this comment

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

How does this relate to the existing libs/init/sbt/exportplugin/src/mill/main/sbt/ExportBuildPlugin.scala? Can we consolidate them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Functionally they are the same.
But the script offers certain advantages such as not requiring any extra build files to add a plugin. The idea was borrowed from IDEA :)

import scala.collection.mutable
import scala.util.Using

opaque type PackageTree = Tree[PackageRepr]
Copy link
Member

@lihaoyi lihaoyi Jul 30, 2025

Choose a reason for hiding this comment

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

Let's just use a normal case class here with normal methods, rather than this opaque type with extensions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A PackageTree is supposed to have the following invariants:

  • The root package is located at the workspace root.
  • The tree structure represents the filesystem structure.
    An opaque type seemed the "right" choice.
    The extension methods can be defined as normal methods.

Could you provide the reason behind the comment? This is just for my learning.

Copy link
Member

Choose a reason for hiding this comment

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

In general, we aim to use simpler language features over more powerful ones, unless there is a strong reason otherwise. In this case, there is no strong reason to use an opaque type, so we use case class which people are more familiar with

@lihaoyi
Copy link
Member

lihaoyi commented Jul 30, 2025

@ajaychandran Overall this looks decent as a first pass, but what is missing is how this interacts with the existing libs/init/sbt implementation

  • Does the new implementation pass all the same tests? I saw some tests failing on your fork's CI
  • Does the new implementation replace the old one? If so can we delete the old one entirely?
  • Can we share code between the old and new implementations? I see a bunch of duplicate logic that can be consolidated

@lihaoyi
Copy link
Member

lihaoyi commented Jul 30, 2025

@lihaoyi

I am going to be working on adding support for some more settings such as jvmId and generating base trait feature.

What are your thoughts on supporting more plugins? We already have tests that require the following:

sbt-projectmatrix
sbt-play
ScalablyTypedConverterGenSourcePlugin

@ajaychandran Let's focus on getting this PR merged first before we start looking at follow ups, I think there's enough work here to keep you occupied for at least a few more days, and I don't want us to get distracted and end up leaving work half finished

import java.io.PrintStream
import scala.collection.immutable.SortedSet

trait PackageWriter {
Copy link
Member

@lihaoyi lihaoyi Jul 30, 2025

Choose a reason for hiding this comment

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

We should re-use as much of BuildGenUtil as possible here, which already has logic for rendering def mvnDeps, def moduleDeps, etc. We should not need to re-implement it from scratch and end up maintaining two different implementations. We should only need to implement that parts that BuildGenutil does not already implement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was hoping to port the other importers to the new design. This might eliminate BuildGenUtil entirely.
Can we discuss this?

val platform = crossVersion match {
case v: librarymanagement.Full if v.prefix.nonEmpty => "::"
case v: librarymanagement.Binary if v.prefix.nonEmpty => "::"
case v: librarymanagement.For2_13Use3 if v.prefix.nonEmpty => "_3" + "::"
Copy link
Member

Choose a reason for hiding this comment

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

Why not just "_3::"? Same for the "_2.13" + "::" below

@ajaychandran ajaychandran changed the title WIP: Improve support for importing projects to Mill from SBT, Gradle, Maven WIP: Proposal for new SBT importer implementation supporting cross projects using a revised core design Jul 30, 2025
@ajaychandran
Copy link
Contributor Author

The PR title and description have been updated.

@lihaoyi
Copy link
Member

lihaoyi commented Jul 30, 2025

@ajaychandran Thanks for updating the PR description. Given this is a cleanroom implementation that aims to completely replace the old one, the PR description will need to be a lot more detailed to explain what the differences between the old implementation and new one are, to justify why it needs to be replaced and why the new implementation is better.

As mentioned over email, this will need to be integrated into the existing utils and Maven/Gradle implementation before merging. While it's fine for you to experiment with a new design in a separate module, it needs to be fully integrated into the existing code before merging, so we can see the results of your new "better" architecture in a realistic scenarios with the same requirements. The old SBT implementation should be fully removed, and whatever code that can be shared between the new SBT implementation and the Maven/Gradle implementations should be consolidated

import mill.javalib._
import mill.javalib.publish._

trait `9.0.0BaseModule` extends MavenModule with PublishModule {
Copy link
Member

Choose a reason for hiding this comment

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

Let's avoid backticks, maybe cal it Gradle900BaseModule or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.
Using "Project" prefix since this is common to all conversions. The directory name is ignored in this case.

@@ -0,0 +1,183 @@
#!/usr/bin/env sh
Copy link
Member

Choose a reason for hiding this comment

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

Are these gradlew, gradlew.bat, and gradle-wrapper.jar files available for download anywhere? If so we should download them as part of the build in libs/init/package.mill rather than vendoring them in our codebase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The files were removed since they are not used by the conversion.

|""".stripMargin
}

def renderRepositories(values: Seq[String], crossValues: Seq[(String, Seq[String])]) =
Copy link
Member

Choose a reason for hiding this comment

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

This long list of render* methods that just forward to renderTask/renderTaskSeq don't seem to do anything useful, especially since they're only called in one place each. Let's just inline them at the callsite

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@lihaoyi
Copy link
Member

lihaoyi commented Oct 13, 2025

@lefou would you like to take a review pass on this as well? I've reviewed it, but it's a big PR and could use another set of eyes

@ajaychandran
Copy link
Contributor Author

@lihaoyi @lefou

I will be pushing in some updates shortly. Please hold on to the review for now.

@ajaychandran ajaychandran marked this pull request as ready for review October 13, 2025 17:35
@ajaychandran
Copy link
Contributor Author

@lihaoyi @lefou
This is ready for review.

The new core datatypes were refactored and simplified.

  • ModuleRepr and TestModuleRepr were unified into ModuleSpec. This recursive type is a specification for a module and its nested modules.
  • Added PackageSpec to define the path for a root module.
  • BuildRepr was renamed to BuildSpec.
  • The generic Tree type was removed.

The PR description has been updated.

@ajaychandran
Copy link
Contributor Author

ajaychandran commented Oct 13, 2025

Also, the SBT script was changed to a plugin.
The classpath for plugins is isolated from scripts. This prevented the use of a reflection based technique to read plugin settings.

@lihaoyi
Copy link
Member

lihaoyi commented Oct 14, 2025

For Testing, could you list out in the PR description what repos fail to import with the previous implementation, that now are able to import successfully? And for each one, what feature was missing before that this PR adds to make it succeed

@ajaychandran bump on this

@ajaychandran
Copy link
Contributor Author

For Testing, could you list out in the PR description what repos fail to import with the previous implementation, that now are able to import successfully? And for each one, what feature was missing before that this PR adds to make it succeed

Failure could be

  • 'init` command fails
  • compilation fails for generated build files
  • imported configuration is invalid

Which one are you referring to here?

Should cross Scala projects be included?

@ajaychandran
Copy link
Contributor Author

@lihaoyi
Added 2 entries for fixes for init failures in the "Testing" section.
Also, added an entry in "Other improvements" section for skipping publish.

@lihaoyi
Copy link
Member

lihaoyi commented Oct 14, 2025

Failure could be

  • 'init` command fails
  • compilation fails for generated build files
  • imported configuration is invalid

Which one are you referring to here?

Should cross Scala projects be included?

Any of them, what I want in the PR description is to know which of the features added in this PR is the one that made each new project's import succeed

@ajaychandran
Copy link
Contributor Author

Updated PR description with summary for test projects.

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.

3 participants