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

Refactor implementation to use public scip-java API #100

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

olafurpg
Copy link

Previously, mill-scip referenced internal APIs from scip-java, which would have breaking changes between patch versions. This commit refactors the implementation to replace usages of unstable internal APIs with more stable APIs.

Previously, mill-scip referenced internal APIs from scip-java, which
would have breaking changes between patch versions. This commit
refactors the implementation to replace usages of unstable internal APIs
with more stable APIs.
@olafurpg
Copy link
Author

@ckipp01 what is the best way to test a change like this?

@ckipp01
Copy link
Owner

ckipp01 commented Jul 21, 2023

Ahhh, I didn't realize these were all meant to be internal 😬. Thanks so much for the PR!

@ckipp01 what is the best way to test a change like this?

./mill itest.[_].test

Should run the integration tests for both the 10.x and 11.x series of Mill.

)

ScipSemanticdb.run(options)
val exit = ScipJava.app.run(List("index-semanticdb", "--cwd", workspace.toString, "--output", scipFile.toString, dest.toString))
Copy link
Owner

Choose a reason for hiding this comment

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

It looks like the classpath entries aren't being included here?

Copy link
Author

Choose a reason for hiding this comment

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

The classpath is automatically inferred from javacopts.txt, which is written to the targetroot

@olafurpg
Copy link
Author

I opened a PR in scip-java to move internal APIs under the com.sourcegraph.internal package https://github.com/sourcegraph/scip-java/pull/617/files

@olafurpg
Copy link
Author

The integration tests are failing with a pprint bincompat issue

io.kipp.mill.scip.Scip.generate java.lang.NoSuchMethodError: 'pprint.TPrint pprint.TPrint$.lambda(scala.Function1)'
    com.sourcegraph.scip_java.commands.IndexSemanticdbCommand$.classDefinition$lzycompute$1(IndexSemanticdbCommand.scala:151)
    com.sourcegraph.scip_java.commands.IndexSemanticdbCommand$.com$sourcegraph$scip_java$commands$IndexSemanticdbCommand$$classDefinition$1(IndexSemanticdbCommand.scala:151)
    com.sourcegraph.scip_java.commands.IndexSemanticdbCommand$.<clinit>(IndexSemanticdbCommand.scala:151)
    com.sourcegraph.scip_java.commands.IndexCommand$.apply$default$17(IndexCommand.scala:88)
    com.sourcegraph.scip_java.commands.IndexCommand$.<clinit>(IndexCommand.scala:244)
    com.sourcegraph.scip_java.ScipJava$.<clinit>(ScipJava.scala:23)
    io.kipp.mill.scip.Scip$.createScip(Scip.scala:269)
    io.kipp.mill.scip.Scip$.$anonfun$generate$2(Scip.scala:

@ckipp01
Copy link
Owner

ckipp01 commented Jul 21, 2023

The integration tests are failing with a pprint bincompat issue

io.kipp.mill.scip.Scip.generate java.lang.NoSuchMethodError: 'pprint.TPrint pprint.TPrint$.lambda(scala.Function1)'
    com.sourcegraph.scip_java.commands.IndexSemanticdbCommand$.classDefinition$lzycompute$1(IndexSemanticdbCommand.scala:151)
    com.sourcegraph.scip_java.commands.IndexSemanticdbCommand$.com$sourcegraph$scip_java$commands$IndexSemanticdbCommand$$classDefinition$1(IndexSemanticdbCommand.scala:151)
    com.sourcegraph.scip_java.commands.IndexSemanticdbCommand$.<clinit>(IndexSemanticdbCommand.scala:151)
    com.sourcegraph.scip_java.commands.IndexCommand$.apply$default$17(IndexCommand.scala:88)
    com.sourcegraph.scip_java.commands.IndexCommand$.<clinit>(IndexCommand.scala:244)
    com.sourcegraph.scip_java.ScipJava$.<clinit>(ScipJava.scala:23)
    io.kipp.mill.scip.Scip$.createScip(Scip.scala:269)
    io.kipp.mill.scip.Scip$.$anonfun$generate$2(Scip.scala:

I'll try to figure this out. I know a while back I looked at updating the com.lihaoyi stuff in scip-java since it's pretty old, but then I hit on issues with it using moped because the deps over there are even older. It might be hard to get this right assuming that Mill is using the latest while the much older stuff is coming in through scip-java.

@olafurpg
Copy link
Author

I found local uncommitted diff in moped that updates multiple dependencies together. IIRC, I stopped after failing to resolve breaking changes in upickle APIs. It's probably solvable, I didn't spend much time on it. The diff is here

https://github.com/scalameta/moped/compare/main...olafurpg:moped:olafurpg/update-everything?expand=1

If we can get moped updated, then it should be trivial to update scip-java since the pprint/upickle stuff is just implementation details of moped.

@olafurpg
Copy link
Author

One possible solution is for mill-scip to load scip-java in an isolated classloader. scip-java is intentionally designed as a cli application, not a library APIs. By focusing on being an application, scip-java allows itself to more liberally add dependencies that would be good to avoid if it was designed as a library.

@olafurpg
Copy link
Author

I'm open to adding a scip-java-interfaces module that would allow you to invoke it via Java reflection similar to how we do it for scalafmt/scalafix/mdoc. In the meantime, you should be able to invoke the main function instead with the caveat that it exists the JVM process on failure (not on success). We can add a separate int mainAPI(String[] args, OutputStream stdout, OutputStream stderr) endpoint that returns the exit code rather than exiting the process.

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.

2 participants