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

Add Kotlin integration #3514

Merged
merged 13 commits into from
Sep 14, 2024
Merged

Add Kotlin integration #3514

merged 13 commits into from
Sep 14, 2024

Conversation

0xnm
Copy link
Contributor

@0xnm 0xnm commented Sep 10, 2024

This PR solves Part 1 from #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):

  • Kotlin stdlib is added by overriding ivyDeps, meaning that if the user of KotlinModule wants to add its own deps and forget to call super.ivyDeps() ++ <their deps> and just does def 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.
  • No Kotlin Reflection library is added automatically to the compile classpath, meaning that if code to compile contains any Kotlin Reflection classes, compilation will fail (because -nostdlib flag is used for compilation)

@lihaoyi Feel free to give your feedback while this is in a Draft state if you want.

@lihaoyi
Copy link
Member

lihaoyi commented Sep 11, 2024

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

@0xnm
Copy link
Contributor Author

0xnm commented Sep 12, 2024

@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.

@lihaoyi
Copy link
Member

lihaoyi commented Sep 13, 2024

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

@lefou
Copy link
Member

lefou commented Sep 13, 2024

Don't forget to address copyright requirements. mill-kotlin is not MIT licensed.

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'd expect some refactorings, since the original codebase was cross-built against multiple Mill versions. Some compromises had to be made to accomplish that, which are no longer necessary here. E.g. the KotlinModulePlatform is superfluous and should be merged with KotlinModule to keep things simple and understandable.

Beside the requirements of the original Apache License 2.0, I'd expect at least some traces of history in the documentation and/or Scaladocs. Not doing so not only pisses the original contributors of the code off (which is mostly me), but it also doesn't guide the user and developer in understanding the code and which relationship it has to the pre-existing mill-kotlin project. I don't think, it's acceptable for an Open Source Software project, to dishonor others people work.

@0xnm
Copy link
Contributor Author

0xnm commented Sep 13, 2024

@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 85c1f30. 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 3385f94, 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.

@0xnm 0xnm marked this pull request as ready for review September 13, 2024 20:08
@0xnm
Copy link
Contributor Author

0xnm commented Sep 13, 2024

@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).

@lihaoyi
Copy link
Member

lihaoyi commented Sep 13, 2024

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

@lihaoyi lihaoyi merged commit 756dd37 into com-lihaoyi:main Sep 14, 2024
24 checks passed
@lefou
Copy link
Member

lefou commented Sep 15, 2024

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.

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