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

Support mill init from an existing sbt project, and improve related Maven and Gradle support #4586

Open
wants to merge 113 commits into
base: main
Choose a base branch
from

Conversation

ShreckYe
Copy link
Contributor

@ShreckYe ShreckYe commented Feb 18, 2025

Resolves #3450.

Feature summary

The conversion

  • handles deeply nested modules
  • captures publish settings
  • configures dependencies for configurations:
    • no configuration
    • Compile
    • Test
    • Runtime
    • Provided
    • Optional
  • configures testing frameworks:
    • Java:
      • JUnit 4
      • JUnit 5
      • TestNG
    • Scala:
      • ScalaTest
      • Specs2
      • µTest
      • MUnit
      • Weaver
      • ZIOTest

The conversion does not support:

  • custom dependency configurations
  • custom settings including custom tasks
  • sources other than Scala on JVM and Java, such as Scala.js and Scala Native
  • cross builds

Implementation summary

To resolve the issue, I use sbt's addPluginSbtFile command to add a plugin jar assembled from the sbt-mill-init-export-build sbt project, which adds an sbt task millInitExportBuild to extract the settings for a build and serialize it with uPickle, and then import the build with the rest of the code in SbtBuildGenMain. The sbt-mill-init-export-build sbt plugin project is built with sbt, so it's wired up with Mill's build logic, with some cross build setups for the common models as sbt depends on Scala 2.12.

Some questions and next steps

  • I left some TODO comments in the code related to alternative implementations, unimplemented features I don't know whether are necessary at the moment and may involve some work, and refactoring/renaming suggestions. Please check them out and let me know what to do next.
  • Conventionally in an sbt project the common settings, corresponding to the base module, are set in ThisBuild. However, not all projects seem to follow this convention, which can lead to very cumbersome build files with long duplicate settings, such as javacOptions. I can add the similar baseProject config arg from Gradle's implementation to resolve this if this is necessary.

…sting SBT project from its corresponding Gradle support

`SbtBuildGenMain.scala` and `BuildGenTests.scala` are not fully adapted yet, and the unadapted code is temporarily commented out.

An initial SBT plugin `ProjectTreePlugin` is added but not fully implemented yet. Only a task that writes some content to a file is added to test whether this approach works.

Exception in `./mill main.init.sbt.compile`:
```text
[1486] java.io.IOException: Scala signature package has wrong version
[1486]  expected: 5.0
[1486]  found: 5.2 in package.class
```

Corresponding code for Maven and Gradle is slightly improved BTW.
…s Scala 2.13 while SBT uses Scala 2.12

`./mill main.init.sbt.test` passes now.
…-project-tree` plugin to an SBT build and writing and reading an empty `ProjectTree` model

`./mill main.init.sbt.test` and `./mill main.init.sbt.sbt-mill-init-generate-project-tree.compile` run successfully. Other than these 2, the changes are not fully tested yet.
…ee` sbt task in `SbtBuildGenMain`, create a "scala-seed-project" test project in test resources with `sbt new`, and update `ProjectTreePlugin`

`./mill main.init.sbt.test` fails.

In the sbt launched under the directory "main/init/sbt/test/resources/scala-seed-project" with `sbt -addPluginSbtFile=/.../mill-init.sbt` with the following content, the `tasks` sbt command output doesn't include an added `millInitGenerateProjectTree` task:

```sbt
addSbtPlugin("com.lihaoyi" % "mill-main-init-sbt-sbt-mill-init-generate-project-tree" % "0.1.0-SNAPSHOT" from "file:/.../mill/out/main/init/sbt/sbtPluginResources.dest/sbt-mill-init-generate-project-tree.jar")
```

The following content of "mill-init.sbt" yields the following result:

```sbt
import mill.main.sbt.ProjectTreePlugin

addSbtPlugin("com.lihaoyi" % "mill-main-init-sbt-sbt-mill-init-generate-project-tree" % "0.1.0-SNAPSHOT" from "file:/.../mill/out/main/init/sbt/sbtPluginResources.dest/sbt-mill-init-generate-project-tree.jar")

enablePlugins(ProjectTreePlugin)

lazy val root = (project in file("."))
  .enablePlugins(ProjectTreePlugin)

```

```text
/Users/shreckye/Projects/mill/mill-init.sbt:1: error: not found: object mill
import mill.main.sbt.ProjectTreePlugin
       ^
/Users/shreckye/Projects/mill/mill-init.sbt:8: error: not found: value ProjectTreePlugin
  .enablePlugins(ProjectTreePlugin)
                 ^
sbt.compiler.EvalException: Type error in expression
```

Reviewing the contents of https://repo1.maven.org/maven2/com/eed3si9n/sbt-assembly_2.12_1.0/2.3.1/sbt-assembly_2.12_1.0-2.3.1.jar, I see there is an "sbt/sbt.autoplugins" file which has the content:

```
sbtassembly.AssemblyPlugin

```

This probably means that such a file has to be included for an sbt plugin and a plain Mill module doesn't support this.
…ala-seed-project" and run `./mill main.init.sbt.test`

The exception:

```text
java.nio.file.NoSuchFileException: /Users/shreckye/Projects/mill/out/main/init/sbt/test/test.dest/sandbox/scala-seed-project/target/mill-init-project-tree.json
```
…n `sbt new sbt/sbt-autoplugin.g8` in "main/init/sbt"
…t to the testing frameworks BTW in the Scaladoc of `SbtBuildGenMain`
…gic that jumps between Mill and sbt

The issues in commit 3c9f155 are resolved and `./mill main.init.sbt.test` now passes.
…project "sbt-mill-init-export-build", and generate build files for "scala-seed-project" in the test resources

The generated "scala-seed-project" "build.mill" file is not completely correct yet.

Changes:

* All kinds of necessary model case classes are added in "Models.scala" in the "models" module.
* A `MillPublishCrossScalaModule` is extracted in the Mill build files.
* The sbt plugin project "sbt-mill-init-generate-project-tree" is renamed to "sbt-mill-init-export-build" as the plugin doesn't just export a tree of projects.
* An `sbtPluginProjectSource` `Source` task is added in the `sbt` module's Mill build configuration to react to changes in the sbt Plugin project.
* `BuildGenBase` is updated with a new type parameter `I` for `input`, as the `IrBaseInfo` is not retrieved from a module in sbt but from `ThisBuild`, and thus `BuildExport.defaultBuildInfo` needs to be passed.
  * A `BaseInfoFromSubproject` subtype trait is added.
  * A `BasicConfig` is extracted from `Config` for sbt.
* `MavenBuildGenMain` and `GradleBuildGenMain` are updated accordingly and slightly improved.
* Changes in `BuildGenUtil`:
  * `scalacOptions` is added similar to `javacOptions`.
  * More Scala test frameworks are added in `testModulesByGroup`.
…evious commit, adjust the format of some code, and inline an sbt task
…`ExportBuildPlugin` because its members are always needed when there is no `baseModule`

* Add a `takeIrPomIfNeeded` function to also set a module's `PomSettings` when it's different from a base module's `PomSettings`.
* Regenerate and update "test/resources/expected/scala-seed-project/build.mill".
…ix the tests `./mill main.init.maven.test` and `./mill main.init.gradle.test` which were broken
…t to the test resources

Both `./mill main.init.sbt.test` and running `sbt` in the project directory fail with:

```
java.lang.UnsupportedOperationException: The Security Manager is deprecated and will be removed in a future release
```

This is likely because I am using JDK 21.
…mprove

1. Bump the sbt version in "sbt-multi-project-example" to v1.10.7
1. Print an info message to remind the user when the sbt command to run the `millInitExportBuild` sbt task fails.
1. Drop dependencies of unknown configurations.
1. Generate the expected snapshots for "sbt-multi-project-example" and review.
   1. Comment the Local Maven Repository as it's machine-dependent.
…y are in the `BaseModule` in the generated build files
…t the conversion, regenerate the snapshots, and fix a bug that `organizationHomepage` is passed as `IrPom.url` instead of `homepage` and regenerate again

Code related to `organizationHomepage` is commented out as it's actually unnecessary.
…un them, review their results, and fix some bugs revealed

- Fix compiling issues in `MavenBuildGenMain.scala` and `GradleBuildGenMain.scala` caused by `scalaVersion` introduced.
- Copy the sbt plugin "sbt-mill-init-export-build-assembly.jar" to a temp directory to be consumed so it works in integration tests when Mill is packaged.
- Improve a message in `ExportBuildPlugin`.
- Make the generated `package` object always extends the `BaseModule` or `SbtModule` trait.
- Fix a bug in `renderIvy` that the double colon `::` is placed at the wrong place.
- Update the expected build file snapshots accordingly in test resources.
- Update docs.
@ShreckYe
Copy link
Contributor Author

The CI seems to fail because there is no sbt set up. (See "main.init.sbt.sbtPluginJarResources failed" in https://github.com/ShreckYe/mill/actions/runs/13398010494/job/37429413361.) Shall I update the CI to set it up? Or shall I pull and add a script from sbt-extras to use instead?

@lihaoyi
Copy link
Member

lihaoyi commented Feb 18, 2025

@ShreckYe yes please update the CI scripts

@ShreckYe
Copy link
Contributor Author

ShreckYe commented Mar 5, 2025

Updated.

One of the 2 CI test on Linux with JDK 11 failed randomly and the other somehow got cancelled. They should work after some reruns.

@ShreckYe ShreckYe requested review from lefou and lihaoyi March 6, 2025 09:48
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Looks good to me. I didn't test it manually though.

@lihaoyi
Copy link
Member

lihaoyi commented Mar 7, 2025

@ShreckYe so I did some manual testing, and hit an issue trying to import https://github.com/ruippeixotog/scala-scraper. It seems the dependency from the modules/config module on the core module (defined by SBT here) is being dropped, and modules/config/package.mill is generating a dependency on the (empty) root module instead def moduleDeps = super.moduleDeps ++ Seq(build). Do you have any idea why this might be happening?

@ShreckYe
Copy link
Contributor Author

ShreckYe commented Mar 7, 2025

@ShreckYe so I did some manual testing, and hit an issue trying to import https://github.com/ruippeixotog/scala-scraper. It seems the dependency from the modules/config module on the core module (defined by SBT here) is being dropped, and modules/config/package.mill is generating a dependency on the (empty) root module instead def moduleDeps = super.moduleDeps ++ Seq(build). Do you have any idea why this might be happening?

I don't see the reason at a glance. I will try to debug and find out.

@lihaoyi
Copy link
Member

lihaoyi commented Mar 7, 2025

Also tried this on the https://github.com/foundweekends/giter8 and it works great. Some manual fixes needed (launcher module needed to be renamed to avoid conflicting with the task, org.scala-sbt:sbt dependency is added by a plugin so needed to be added manually, plugin.compile fails due to some weird magic) but otherwise most things compile and all tests pass (or at least pass as much as they do with SBT).

@lihaoyi
Copy link
Member

lihaoyi commented Mar 7, 2025

I tried this on https://github.com/nscala-time/nscala-time and it doesn't seem to be picking up the test module. Seems like it's generating a trait test extends SbtTests rather than an object. Is it because we don't have Scalacheck in the list of TestModules? Could we add it?

@ShreckYe
Copy link
Contributor Author

ShreckYe commented Mar 7, 2025

I tried this on https://github.com/nscala-time/nscala-time and it doesn't seem to be picking up the test module. Seems like it's generating a trait test extends SbtTests rather than an object. Is it because we don't have Scalacheck in the list of TestModules? Could we add it?

I remember seeing this too at some point but didn't reproduce it later after some iterations. I will try to find out why it happens.

@ShreckYe
Copy link
Contributor Author

ShreckYe commented Mar 7, 2025

@lihaoyi Shall I add these 3 libraries to the integration tests too or just experiment with them manually?

@lihaoyi
Copy link
Member

lihaoyi commented Mar 7, 2025

@ShreckYe manual testing is fine; integration tests are pretty expensive since they run every time and slow down CI, so we want to minimize them even at the cost of test coverage

@ShreckYe
Copy link
Contributor Author

ShreckYe commented Mar 7, 2025

@ShreckYe so I did some manual testing, and hit an issue trying to import https://github.com/ruippeixotog/scala-scraper. It seems the dependency from the modules/config module on the core module (defined by SBT here) is being dropped, and modules/config/package.mill is generating a dependency on the (empty) root module instead def moduleDeps = super.moduleDeps ++ Seq(build). Do you have any idea why this might be happening?

This is caused by the core project with a setting name := "scala-scraper" causing it to have the same GAV ID as the root project.

In its build.sbt there is a workaround of not publishing the root project to make it work with sbt:

// do not publish the root project
publish / skip := true

I don't see an easy and general way to fix and support this though: we can't just ignore projects with publish / skip := true; and I don't think either project (the root project or core) takes precedence over the other. I think I can update the code to print a warning in such cases though.

@lihaoyi
Copy link
Member

lihaoyi commented Mar 7, 2025

What about if there is a conflict between two modules and one has publish / script := true and the other doesn't, we ignore the one that is skipped and use the other one?

@lihaoyi
Copy link
Member

lihaoyi commented Mar 7, 2025

Perhaps another question: looking at that repo, only the core module has name := "scala-scraper", and the root module has no name assigned. How can root have the same GAV as core when one has a name and the other doesn't?

@ShreckYe
Copy link
Contributor Author

ShreckYe commented Mar 7, 2025

Perhaps another question: looking at that repo, only the core module has name := "scala-scraper", and the root module has no name assigned. How can root have the same GAV as core when one has a name and the other doesn't?

If I understand correctly, in sbt a root project is added by default, and each project takes the name of its directory if it's not set explicitly. So in this case the root project takes the directory name scala-scraper, which is the same as the core project's, and both of them then have the same GAV net.ruippeixotog:scala-scraper:version.

@ShreckYe
Copy link
Contributor Author

ShreckYe commented Mar 7, 2025

What about if there is a conflict between two modules and one has publish / script := true and the other doesn't, we ignore the one that is skipped and use the other one?

I can try to implement this approach but I think it's a bit niche and not straighforward to the user, as publish is only one of the publish tasks - it's not the SSOT for all the publish settings as there are others such as publishLocal and publishM2. This is also the reason I just let all the converted modules extend PublishModule instead of judging from publish / skip.

@ShreckYe
Copy link
Contributor Author

ShreckYe commented Mar 7, 2025

I just checked again and I was wrong about this, publishLocal / skip depends on publish / skip: it follows the value of publish / skip if not set explicitly. And publish / skip is also the documented way to avoid publishing a project.

@ShreckYe
Copy link
Contributor Author

ShreckYe commented Mar 7, 2025

I have been going through the related sbt settings, and a better approach as it occurs to me now, might be to use buildDependencies and get ProjectRefs instead of ModuleIDs for the inter-project dependencies, instead of retrieving the ModuleIds and getting the corresponding projects from a GAV-project map. Some related definitions copied here:

  val buildDependencies = settingKey[BuildDependencies]("Definitive source of inter-project dependencies for compilation and dependency management.\n\tThis is populated by default by the dependencies declared on Project instances, but may be modified.\n\tThe main restriction is that new builds may not be introduced.").withRank(DSetting)
final class BuildDependencies private (
    val classpath: DependencyMap[ClasspathDep[ProjectRef]],
    val aggregate: DependencyMap[ProjectRef]
)

In this way we also solve the potential issue when multiple subprojects have the same name from their directories and therefore the same GAV/ID, which is likely when the build is not a published library thus not configured further. This is also what I suggested in the Gradle PR review as Gradle has the same issue (I remember running into this working on one of my projects years ago).

This refactor will involve some work though. For example, BuildGenBase.getPackage will be only needed in the Maven module then, and related code will need to be refactored.

@ShreckYe ShreckYe changed the title Support mill init from an existing sbt project Support mill init from an existing sbt project, and improve related Maven and Gradle support Mar 7, 2025
@lihaoyi
Copy link
Member

lihaoyi commented Mar 7, 2025

@ShreckYe FYI I have pushed fixed for test-mill-bootstrap.sh, pull it down before you make further changes so you preserve them

I have been going through the related sbt settings, and a better approach as it occurs to me now, might be to use buildDependencies and get ProjectRefs instead of ModuleIDs for the inter-project dependencies, instead of retrieving the ModuleIds and getting the corresponding projects from a GAV-project map.

Let's try implementing that in this PR and see if we can get it to work. It does indeed seem like a better way of doing things, since unlike Maven, SBT and Gradle don't always publish all modules as artifacts by default, so it seems likely to get conflicts of the sort I hit in scala-scraper

@ShreckYe
Copy link
Contributor Author

ShreckYe commented Mar 7, 2025

OK no problem. Shall I do this just for sbt here in this PR or for both Gradle and sbt?

@lihaoyi
Copy link
Member

lihaoyi commented Mar 7, 2025

Try doing it for both; hopefully a lot of the code is very similar, and you've reviewed the gradle PR so you should be somewhat familiar. But if you can't get gradle to work then at least let's do it for SBT

@sake92
Copy link
Contributor

sake92 commented Mar 8, 2025

I think I used that approach in my PR #4575
A bit ugly with mutable variables.. might be useful

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.

Support mill init from an existing SBT project (2000USD Bounty)
5 participants