Closed 0xnm closed 3 weeks ago
I think not supporting kotlin-reflect is fine for this initial PR for Part 1 of the bounty. Part 2 of the bounty covers a lot more scenarios, so we can look at figuring out supporting kotlin-reflect and other fanciness (compiler plugins?) then, as the counterpart to example/javalib/module/6-annotation-processors
and example/scalalib/module/6-scala-compiler-plugins
@lihaoyi Code-wise this PR is done, I think. I addressed your suggestions. The only remaining thing is to add Kotlin samples to the generated docs.
The code looks good I think. The example tests just need to be tweaked I think to match the stdout format of the kotest library. You can run them locally via ./mill -i 'example.kotlinlib.basic[1-simple].local'
and adjust them accordingly, and once they're green I'll merge this and wire the example tests into the docsite myself
- It in-sources https://github.com/lefou/mill-kotlin with minimal changes
Don't forget to address copyright requirements. mill-kotlin
is not MIT licensed.
@lefou Yes, sure, it was on my to-do list before marking PR as final, after code changes are complete.
I added the copyright now in https://github.com/com-lihaoyi/mill/pull/3514/commits/85c1f30650682a68e2bd609eca49566e93b7b020. However, since https://github.com/lefou/mill-kotlin has no explicit copyright statement neither in the source files, nor in the license file, I had to come up with the following (since I traced back the first commit to 2020 and there are 2 contributors):
/*
* Copyright 2020-Present Original lefou/mill-kotlin repository contributors.
*/
Feel free to suggest another one if needed.
Regarding the refactoring: I merged KotlinModulePlatform
in KotlinModule
in https://github.com/com-lihaoyi/mill/pull/3514/commits/3385f948245ebd30e7c0f16e63e6f1ee41e5e2d1, but I would refrain from the further changes, because I think it is better to keep the focus of this PR on the integration part and keep the original files as-is for that. Refactoring can be done in the further PRs if needed.
@lihaoyi I fixed Kotlin tests, but there is one job ('integration.{feature,failure}[_].fork.testCached
) which failed and I guess it is not related to this PR. Probably there is a flaky test there, but I have no rights to re-run the job (without pushing empty commit).
I re-ran the CI job, looks good. I'll go ahead and merge it and add a landing page for it, and will include a reference to the original repo as @lefou requested. @0xnm you can email me your bank transfer details and I'll pay out the first half of the bounty
What I actually meant to request is to respect the original license, which is Apache License 2. Although it allows you to do almost everything, it also demands to point it out and to include the license text somewhere in the project, if you use or derive the work.
The fact that the actual code you included is from me is rather secondary here. My point is about free and open source work in general. It's only free if you comply to the license, whether you like it or not. And since this is missing in this pull request, I had rather not accepted this PR as-is.
This PR solves Part 1 from https://github.com/com-lihaoyi/mill/issues/3451:
I still need to finalize it (especially automated test for example
4-builtin-commands
fails right now), but most of the work is done.Things I noticed in the original implementation (https://github.com/lefou/mill-kotlin):
ivyDeps
, meaning that if the user ofKotlinModule
wants to add its own deps and forget to callsuper.ivyDeps() ++ <their deps>
and just doesdef ivyDeps = <their deps>
, then no stdlib will be in the classpath during the compilation and it will fail with cryptic error. Such approach is thus error-prone.-nostdlib
flag is used for compilation)@lihaoyi Feel free to give your feedback while this is in a Draft state if you want.