search

davidB / scala-maven-plugin

The scala-maven-plugin (previously maven-scala-plugin) is used for compiling/testing/running/documenting scala code in maven.
https://davidb.github.io/scala-maven-plugin/
The Unlicense
560 stars 151 forks source link

Run incremental compiler in a forked process when jvmArgs are defined, close #507 #659

Closed slandelle closed 1 year ago

slandelle commented 1 year ago

Motivation:

The incremental mode which is now the default one doesn't handle jvmArgs as it can't run in a forked process. A user might need this for example in order to increase the Xss to compile complex code with lots of method chaining and type inference.

Modification:

Result:

The default incremental compiler now properly handles jvmArgs.

slandelle commented 1 year ago

@davidB Green at last! Note: I tried to upgrade the Scala versions used for the tests but the test_scaladoc_compiler_plugins test fails. Upgrading to Scala 2.13.10 means upgrading kind-projector to 0.13.2 and this is what fails. I'm really not familiar with kind-projector and don't intend to be any time soon...

davidB commented 1 year ago

Thanks for your hard work, your nights look short.

slandelle commented 1 year ago

Lot of refactor Hard (for me) to understand everything just by read (change due to code extraction are mixed with the rest)

Yeah, sorry about this. That's the kind of huge task that you don't start with a plan. Ideally, I would start over and isolate the changes, but honestly, I don't have time for that. I already spend more than 2 full days on this. If you insist, I can try and spend 2 hours at most.

Why do you introduce the booter jar ?

see https://github.com/davidB/scala-maven-plugin/pull/659/files#diff-6b9f0f5726657db850ef7d0509c82e1e1a9d8a734bf3e01a79eb8434b4747a5fR22-R24

On Windows, the command length is way more limited than on Linux and MacOS. What breaks here is the classpath, because all the libraries are scattered in the maven cache, each file with a super long absolute path. The workaround is to create a booter jar where the classpath is defined as an attribute in the MANIFEST.MF. We've been doing this successfully for a decade in Gatling (we found this workaround in the eclipse code).

slandelle commented 1 year ago

Thanks for your hard work, your nights look short.

Plane + jetlag. Yes they are 😂

davidB commented 1 year ago

No worry , I don't ask for any changes. I fully agree/understand, when we start changes, made refactor it's easy (with IDE,...) when we are on the flow (also when we do the change on our "spare time". My comment was more to say "I reviewed the code, but I might miss some points and issue".

slandelle commented 1 year ago

Thanks for your understanding. Also, another refactor I did along the way was to start using File for files and not String. So much better to have typed parameters! I considered using Path which semantically would be better when you just need the location of the file on the filesystem, but you end up jumping back to File all the time(eg, is it a directory?). It sadly feels like this Path API is only meant for NIO2 usage.

slandelle commented 1 year ago

@davidB Fine if I release 4.8.0?

davidB commented 1 year ago

Yes go for a release.

I switch from File to Path (I forgot some String from what I saw) when it was added to the JDK, and expected to be able to remove some toFile, but same conclusion as you :-(

davidB commented 1 year ago

Thanks to maintain the project.

[OT] Since 3y now, my contact with JVM is limited (few kotlin code for poc on Android, and sometimes review on oss project like this one). When I code for work (or free time) it's now in Rust.