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

feat: allow for a non-local Java Compiler to be used #1988

Merged
merged 2 commits into from
Aug 12, 2022

Conversation

ckipp01
Copy link
Collaborator

@ckipp01 ckipp01 commented Aug 12, 2022

Currently if you want to pass in some javacOptions that start with
-J the Java compiler will reject them since the local java compiler
won't accept -J flags. You can see this in an example on JDK 17 where
you need some -J flags to get the java semantidb compiler plugin to
work:

object javaModule extends JavaModule {

  def ivyDeps = Agg(ivy"com.sourcegraph:semanticdb-javac:0.8.2")

  def javacOptions = Seq(
    "-J--add-exports",
    "-Jjdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED",
    "-J--add-exports",
    "-Jjdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED",
    "-J--add-exports",
    "-Jjdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED",
    "-J--add-exports",
    "-Jjdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED",
    "-J--add-exports",
    "-Jjdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED",
    s"-Xplugin:semanticdb -sourceroot:${T.workspace} -targetroot:${T.dest}"
  )

}

Which will result in:

❯ mill javaModule.compile
[19/19] javaModule.compile
[info] compiling 1 Java source to /Users/ckipp/Documents/scala-workspace/minimal/out/javaModule/compile.dest/classes ...
[warn] Javac is running in 'local' mode. These flags have been removed:
[warn]  -J--add-exports, -Jjdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED, -J--add-exports, -Jjdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED, -J--add-exports, -Jjdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED, -J--add-exports, -Jjdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED, -J--add-exports, -Jjdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED
[error] An exception has occurred in the compiler (17.0.4). Please file a bug against the Java compiler via the Java bug reporting page (http://bugreport.java.com) after checking the Bug Database (http://bugs.java.com) for duplicates. Include your program, the following diagnostic, and the parameters passed to the Java compiler in your report. Thank you.
[error] java.lang.IllegalAccessError: class com.sourcegraph.semanticdb_javac.SemanticdbPlugin (in unnamed module @0x4e59433c) cannot access class com.sun.tools.javac.api.BasicJavacTask (in module jdk.compiler) because module jdk.compiler does not export com.sun.tools.javac.api to unnamed module @0x4e59433c
...

This change looks at the javacOptions when creating the java comipler
and if detected makes sure you get a non-local compiler. If none are
detected, then the behavior basically is the same as it was before where
a local instance is used.

We also cache this to ensure it's not created over and over. However if
your javacOption change, then we do give a new instance.

Fixes #1983

Currently if you want to pass in some `javacOptions` that start with
`-J` the Java compiler will reject them since the local java compiler
won't accept `-J` flags. You can see this in an example on JDK 17 where
you need some `-J` flags to get the java semantidb compiler plugin to
work:

```scala
object javaModule extends JavaModule {

  def ivyDeps = Agg(ivy"com.sourcegraph:semanticdb-javac:0.8.2")

  def javacOptions = Seq(
    "-J--add-exports",
    "-Jjdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED",
    "-J--add-exports",
    "-Jjdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED",
    "-J--add-exports",
    "-Jjdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED",
    "-J--add-exports",
    "-Jjdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED",
    "-J--add-exports",
    "-Jjdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED",
    s"-Xplugin:semanticdb -sourceroot:${T.workspace} -targetroot:${T.dest}"
  )

}
```

Which will result in:

```
❯ mill javaModule.compile
[19/19] javaModule.compile
[info] compiling 1 Java source to /Users/ckipp/Documents/scala-workspace/minimal/out/javaModule/compile.dest/classes ...
[warn] Javac is running in 'local' mode. These flags have been removed:
[warn]  -J--add-exports, -Jjdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED, -J--add-exports, -Jjdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED, -J--add-exports, -Jjdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED, -J--add-exports, -Jjdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED, -J--add-exports, -Jjdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED
[error] An exception has occurred in the compiler (17.0.4). Please file a bug against the Java compiler via the Java bug reporting page (http://bugreport.java.com) after checking the Bug Database (http://bugs.java.com) for duplicates. Include your program, the following diagnostic, and the parameters passed to the Java compiler in your report. Thank you.
[error] java.lang.IllegalAccessError: class com.sourcegraph.semanticdb_javac.SemanticdbPlugin (in unnamed module @0x4e59433c) cannot access class com.sun.tools.javac.api.BasicJavacTask (in module jdk.compiler) because module jdk.compiler does not export com.sun.tools.javac.api to unnamed module @0x4e59433c
...
```

This change looks at the `javacOptions` when creating the java comipler
and if detected makes sure you get a non-local compiler. If none are
detected, then the behavior basically is the same as it was before where
a local instance is used.

We also cache this to ensure it's not created over and over. However if
your `javacOption` change, then we do give a new instance.

Fixes com-lihaoyi#1983
@lefou lefou self-requested a review August 12, 2022 10:18
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.

This change looks reasonable. I think we should limit the newly introduced cache though, as it will grow indefinitely when the user frequently changes options. Either, we introduce a limit, which we probably also want to control via some settings, or we use a SoftReference[Compilers].

@ckipp01
Copy link
Collaborator Author

ckipp01 commented Aug 12, 2022

I think we should limit the newly introduced cache though, as it will grow indefinitely when the user frequently changes options

I was actually thinking that this would be super rare in the lifetime of this? Even if a user changes this 10 times, that's not a huge deal right? What would too large be in your opinion, and in what scenario do you think a user would be constantly updating these? I might be missing that use case in my mind.

@lefou
Copy link
Member

lefou commented Aug 12, 2022

I think we should limit the newly introduced cache though, as it will grow indefinitely when the user frequently changes options

I was actually thinking that this would be super rare in the lifetime of this? Even if a user changes this 10 times, that's not a huge deal right? What would too large be in your opinion, and in what scenario do you think a user would be constantly updating these? I might be missing that use case in my mind.

If we don't expect the cache to be large, there is no reason to let it grow to exactly that. Even if this is not a critical spot in Mill, out of experience, sometimes, this will blow up. Imagine an unnoticed bug in Compilers which consumes a lot of memory, causing various other spurious issues in Mill.

Mill server is expected to run in the background and this can be very long. Also, BSP is starting an instance and keeps it for the full IDE session. If one plays around with some javacOptions which affect lots of modules, boom. I'm also to lazy to come up with a good default value, instead, I propose to just use a SoftReference, as we do in other places in Mill, e.g. val classLoaderCache in the same file. It's use is rather simple, but we prevent a whole class of issues.

@ckipp01
Copy link
Collaborator Author

ckipp01 commented Aug 12, 2022

Alright, refactored to use a SoftReference. I've actually never used that before, so it was interesting reading up on it.

@ckipp01 ckipp01 marked this pull request as ready for review August 12, 2022 12:37
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. Thank you!

@lefou lefou merged commit 080d704 into com-lihaoyi:main Aug 12, 2022
@lefou lefou added this to the 0.10.6 milestone Aug 12, 2022
@ckipp01 ckipp01 deleted the nonLocalJava branch August 12, 2022 12:42
ckipp01 added a commit to ckipp01/mill-scip that referenced this pull request Aug 25, 2022
Now that com-lihaoyi/mill#1988 is in a stable
release we can start supporting pure javaModules when the user is on
Java 17 on Mill >= 0.10.6.
lefou added a commit that referenced this pull request Jan 4, 2023
…2231)

We need this feature to support SemanticDB generation 
(#2227) via a Java
compiler plugin in mixed Scala/Java projects.

We already introduced non-local Java compilers in 
#1988. This change now
also allows the use of a non-local Java compiler in mixed Java/Scala
modules, if any `-J` option is given.

As compilers are cached, I added the `hashCode` of the relevant runtime
`javacOptions` to the cache key.

I took the opportunity to remove non-relevant options from the existing
Java compiler cache, to reduce the amount of cached compiler instances.

Pull request: #2231
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.

[feature request] Add support for setting javaHome in javaModule
2 participants