Skip to content

Conversation

@lostiniceland
Copy link
Contributor

@lostiniceland lostiniceland commented Aug 26, 2025

I have been working on Groovy support since I would like to use Mill for projects where tests are written in Spock/Groovy.

It is basically a modified version of the Kotlin support.

Whats currently working:

  • Groovy compilation (tested with 4 & 5)
  • Maven project layout with an additional convenience option to configure Groovy for tests only
  • JUnit5 test execution
  • Spock tests
  • CompileStatic
  • Configure Bytecode Version and Preview

If anyone would like to provide feedback I would appreciate it.

Pull request: #5747

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.

Thanks for opening this PR and welcome to Mill.

I had a quick scan and left some comments where I spotted something. Overall, this looks reasonable, but there might be some more places copied from the Kotlin implementation which aren't adequate for Groovy.

@lihaoyi lihaoyi changed the title Intial Groovy setup Initial Groovy setup Aug 27, 2025
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.

I understand, that you copied another language module to start this implementation, but it would be helpful, if you could revise it and delete all tasks that aren't needed, like groovycOptions.

@lostiniceland lostiniceland force-pushed the groovylib branch 2 times, most recently from 21334e0 to 69cb09a Compare September 15, 2025 14:08
@lostiniceland
Copy link
Contributor Author

@lefou I've resolved most of the issues and fixed the Spock bug.

I've also extended the use-cases and backed them by tests to support static compilation as well as a 3-stage compilation for setups where Java depends on Groovy and vice-versa.

In the next weeks I will probably find some more time to see about what configuration options are needed and how to support them.

@lefou
Copy link
Member

lefou commented Sep 16, 2025

@lostiniceland Can you resolve the merge conflicts?

@lostiniceland
Copy link
Contributor Author

@lefou I would like to pull bomMvnDeps up to JavaModuleBase in order to setup BOMs within the TestModule.Spock utility (I guess other testframeworks provide BOMs as well).
Would this break compatibility? If not, are there other reasons why it is not there?

@lefou
Copy link
Member

lefou commented Sep 16, 2025

@lefou I would like to pull bomMvnDeps up to JavaModuleBase in order to setup BOMs within the TestModule.Spock utility (I guess other testframeworks provide BOMs as well). Would this break compatibility? If not, are there other reasons why it is not there?

You can try, but I think it will break bin-compat.

@lostiniceland
Copy link
Contributor Author

@lefou I would like to pull bomMvnDeps up to JavaModuleBase in order to setup BOMs within the TestModule.Spock utility (I guess other testframeworks provide BOMs as well). Would this break compatibility? If not, are there other reasons why it is not there?

You can try, but I think it will break bin-compat.

I gave it a try and no additional test broke. From SemVer perspective it also shouldn't break anything as long as the trait provides a default implementation. So I will keep this for now.

@lostiniceland
Copy link
Contributor Author

lostiniceland commented Sep 16, 2025

@lefou Have a look at the latest commit

I managed to manage (pun intended) the JUnit5/Spock dependencies. Since the BOMs are only available at certain versions this will be checked.
For JUnit starting from 5.12 this has the nice effect that the junit-bom manages the platform-launcher version. So there is actually no need to specify this (but removing the property will break compatibility).
So I've implemented it according to these rules:

  1. platform-launcher version set => add launcher with specific version
  2. no launcher version, but junit-bom available => add launcher without version (because managed)
  3. else => don't add launcher
    This should make the usage easier.

What do you think? Do you want to keep this, or should I remove it?

@lostiniceland lostiniceland force-pushed the groovylib branch 2 times, most recently from 32f8111 to bda67ee Compare September 23, 2025 13:44
@lostiniceland
Copy link
Contributor Author

lostiniceland commented Sep 23, 2025

Looks like I was wrong about binary compatibility though I am not quite sure what to make of this:
https://github.com/lostiniceland/mill/actions/runs/17948459255/job/51041447311

Lets say Groovylib is released with Mill 1.0.10 then I cannot use this with Mill 1.0.9. Fine. And plugins targeting 1.0.10 will also still be compatible since the new method has a default implementation.
Currently I don't really trust this test.

@lostiniceland
Copy link
Contributor Author

@lefou I think this PR is ready to get out of the draft status.

I've added examples and docs (though ./mill docs.fastPages didn't do anything), added most relevant compiler options, and I've been using it for Spock testing in a side project for some weeks now without problems.

There is this outstanding issue with the binary-compatibility caused by adding bomMvnDeps to the TestModule base (see previous message) which I think should actually work. Maybe @lihaoyi can provide insight. If this really breaks compat, then I will have to remove the feature that a BOM is automatically added for certain Test setups.

@lihaoyi
Copy link
Member

lihaoyi commented Sep 24, 2025

I haven't reviewed this in detail, but let's put this in contrib/ rather than libs/. That will send the right signal that it is new and experimental, and if it becomes popular we can move it into libs/ with the Java/Scala/Kotlin toolchains

@lostiniceland
Copy link
Contributor Author

I haven't reviewed this in detail, but let's put this in contrib/ rather than libs/. That will send the right signal that it is new and experimental, and if it becomes popular we can move it into libs/ with the Java/Scala/Kotlin toolchains

Do I have to change the package from mill.groovylib to mill.contrib.groovylib? It would be the correct way, but it just creates more work and I am quite confident that the module is in a good shape. I'd rather spent my time on adding tests for the recently added compiler options (currently only tested manually)

@lefou
Copy link
Member

lefou commented Sep 26, 2025

Since we also have experimental pythonolib and javascriptlib not in contrib, I think it's ok to have a groovylib, as long as it is marked as experimental.

We already have the example tooling in place for <lang>lib modules, which is quite different to contrib modules.

@lihaoyi
Copy link
Member

lihaoyi commented Sep 26, 2025

Since we also have experimental pythonolib and javascriptlib not in contrib, I think it's ok to have a groovylib, as long as it is marked as experimental.

We already have the example tooling in place for <lang>lib modules, which is quite different to contrib modules.

Sounds good to me!

@lefou lefou marked this pull request as ready for review September 26, 2025 10:49
Comment on lines 17 to 27
/**
* In a mixed setup this will compile the Groovy sources to Java stubs.
*/
def compileGroovyStubs(
sourceFiles: Seq[os.Path],
classpath: Seq[os.Path],
outputDir: os.Path
)(implicit
ctx: TaskCtx
)
: Result[CompilationResult]
Copy link
Member

Choose a reason for hiding this comment

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

What is the output of this? .class files or .java files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.java though they are cleaned up after the last groovy compilation stage.

Copy link
Member

Choose a reason for hiding this comment

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

I think using CompilationResult in the return isn't appropriate here.

Copy link
Member

@lefou lefou Sep 27, 2025

Choose a reason for hiding this comment

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

Is this stub-handling somewhere documented? Or can you outline the process and what you mean with "cleaned up after the last compilation stage"?

It looks to me, this groovy stubs should reside in it's own cacheable task, since it should not need to be regenerated when just Java files changed.

Copy link
Contributor Author

@lostiniceland lostiniceland Sep 27, 2025

Choose a reason for hiding this comment

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

I will document it.

In order for Java to compile with dependencies to Groovy, It only needs the method signatures (headers). This is what the Groovy Stubs are. When you have dependencies Groovy -> Java AND Java -> Groovy you need a 3 stage compile

  1. Groovy compiles to Java stubs (*.java files in the output)
  2. Java compiles with the Stubs available
    2.1 Cleanup stubs after Java compile (I was wrong before, it happens here)
  3. Compile Groovy (now all Java dependencies are also available on the compilepath).

It should actually be very similar in the other toolchains (scala, kotlin) but maybe the external compiler handles this already internally.

Copy link
Contributor Author

@lostiniceland lostiniceland Sep 29, 2025

Choose a reason for hiding this comment

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

Good point about Mill caching. I was using GMavenPlus (Groovy Maven Plugin) as a reference, and maybe there are some things that we could do differently. For instance, I could test if the Groovy compiler can read Java files so we might be able to ditch the stubs.

Edit: The Groovy compiler is actually able to receive Groovy and Java sources and compile but it will not consider anything passed in javacOptions. Thats why I choose to split the stages.

Copy link
Member

@lefou lefou Sep 29, 2025

Choose a reason for hiding this comment

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

Technically, we could model the three steps as independent cacheable tasks, as in the following pseude-code:

def groovyCompileJavaStubs: Task[PathRef]
def groovyCompileJava: Task[PathRef] // depends on groovyCompileJavaStubs
def groovyComileGroovy: Task[PathRef] // depends on groovyCompileJava

Unfortunately, this produces more than one out directory with final classfiles with is not what (current) Mill expects, so we need to merge-copy the final classes-directories together.

def compile: Task[CompilationResult] {
  Seq(groovyCompileGroovy(), groovyCompileJava())
    .map(pr = os.copy(pr.path, Task.dest, mergeFolders = true))
  CompileResult(..., PathRef(Task.dest))
}

Copy link
Contributor Author

@lostiniceland lostiniceland Sep 29, 2025

Choose a reason for hiding this comment

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

@lefou Thanks for fixing the mima setup.

EDIT: I've rebased and updated the compilation to use a dedicated (cached) folder for the stubs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I missed your post and tried another solution. What's your take on the latest commit?
I am not sure if we split the compile into 3 public tasks: since they are implicitly dependent on another, it can easily break if the user overwrites only one.

Copy link
Member

Choose a reason for hiding this comment

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

I haven't a look at your latest changes, yet. About my suggestions: You always could hide implementation details by making task private, without loosing all the benefits. Besides, I think there is only a minimal risk of breaking anything by overriding, since each task has exactly one purpose and clear in- and outputs. Also, overriding selected task is exactly the way to customize the integration further. There is also the benefit of inspect-ability from the CLI.

@lefou
Copy link
Member

lefou commented Sep 29, 2025

Your latest git rebase looks slightly gone wrong. Some commit should not be part of this PR. Better avoid git rebease and use git merge for newer changes, esp. since we are in the middle of a review and multiple users have committed to the branch.

@lostiniceland
Copy link
Contributor Author

lostiniceland commented Dec 12, 2025

@lefou I also worked on the website restructuring yesterday (merging right now). I will run more tests over the weekend as well. And Spock 2.4 for Groovy 5 was released yesterday, so I would like to add a cross-test to the current test-suite.
I am also going over every example from the docs again to make sure all is correct.

@lefou
Copy link
Member

lefou commented Dec 12, 2025

I think we can commit to merge this before we tag 1.1.0. The only issue I'm currently aware of is the fact that the declarative example doesn't work when Mill runs on Java 17, but we can't yet select the groovyCompileTargetBytecode, which we can fix independently, given this is still experimental and it works on Java 21.

@lefou lefou added this to the 1.1.0 milestone Dec 12, 2025
@lihaoyi
Copy link
Member

lihaoyi commented Dec 12, 2025

@lihaoyi Any idea, how to deal with this error message?

[error] build.mill.yaml:7:30
8714]   groovyCompileTargetBytecode: '11'
8714]                                ^
8714]   groovyCompileTargetBytecode Failed de-serializing config override: expected unit got string

The task is defined as def groovyCompileTargetBytecode: T[Option[String]].

I think I've seen this before as a type inference issue for None. Can you try replacing it with Option.empty[T] at the definition of this method in the code

@lefou
Copy link
Member

lefou commented Dec 12, 2025

@lihaoyi Any idea, how to deal with this error message?

[error] build.mill.yaml:7:30
8714]   groovyCompileTargetBytecode: '11'
8714]                                ^
8714]   groovyCompileTargetBytecode Failed de-serializing config override: expected unit got string

The task is defined as def groovyCompileTargetBytecode: T[Option[String]].

I think I've seen this before as a type inference issue for None. Can you try replacing it with Option.empty[T] at the definition of this method in the code

Changed it.

-  def groovyCompileTargetBytecode: T[Option[String]] = None
+  def groovyCompileTargetBytecode: T[Option[String]] = Option.empty

Now the error message is

8714]   [error] build.mill.yaml:7:30
8714]   groovyCompileTargetBytecode: '11'
8714]                                ^
8714]   groovyCompileTargetBytecode Failed de-serializing config override: java.lang.Enum.<init>(java.lang.String)

@lihaoyi
Copy link
Member

lihaoyi commented Dec 12, 2025

How about Option.empty[String]?

@lefou
Copy link
Member

lefou commented Dec 12, 2025

How about Option.empty[String]?

That's not supported. But I tried something that works.

-  def groovyCompileTargetBytecode: T[Option[String]] = None
+  def groovyCompileTargetBytecode: T[Option[String]] = Task.apply[Option[String]] { Option.empty }

@lefou
Copy link
Member

lefou commented Dec 12, 2025

Correction: Compiler hickup, Option.empty[String] also works.

@lefou
Copy link
Member

lefou commented Dec 12, 2025

@lostiniceland I think I fixed everything except the test you're still working on. I leave it to you to finish your documentation changes. Please ping me, if you're done.

@lostiniceland
Copy link
Contributor Author

Test for Spock under Groovy 5 has been updated and documentation verified so far.

I will have a look tomorrow on the last failing CI Test.

@lostiniceland
Copy link
Contributor Author

@lefou finally a green build. Thanks for the support.

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.

Thank you for this contribution!

@lefou lefou merged commit c79a19c into com-lihaoyi:main Dec 13, 2025
35 checks passed
@lostiniceland lostiniceland deleted the groovylib branch December 13, 2025 16:23
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