feat: add support for the Mill build tool by ckipp01 · Pull Request #477 · sourcegraph/scip-java

ckipp01 · 2022-08-02T15:47:53Z

This PR adds in support for the Mill build tool by utilizing
https://github.com/ckipp01/mill-scip. It's a pretty thin wrapper. You
can find more details about mill-scip in the repo but the general
workflow is:

It's an external Mill plugin that gathers everything needed to do a
compile via Mill, but then adds some extra plugins/scalacOptions to
ensure semanticDB gets produced
Once produced it uses scip-java as a library to actually produce the
scip file.

The plugin isn't actually a dependency here which ensures we don't have
any cyclical dependency on anything, but rather just utilizes the
--import functionality of Mill and external plugins. I'll leave a
couple more comments on various parts of the PR.

NOTE This only currently supports Scala and Scala 3 projects. There
are some issues with Java projects which will need some upstream work.
You can track that in com-lihaoyi/mill#1983.

EDIT: So about Java Modules, I actually fixed this in com-lihaoyi/mill#1988, so 0.10.6 should support this. However right now I say anything that starts with a 0.1 is supported, but I may need to change that to be > 0.10.6 or above.

Refs #306

Test plan

There is a test for the Mill integration

tests/buildTools/src/test/scala/tests/MillBuildToolSuite.scala

scip-java/src/main/scala/com/sourcegraph/scip_java/buildtools/MillBuildTool.scala

This PR adds in support for the Mill build tool by utilizing https://github.com/ckipp01/mill-scip. It's a pretty thin wrapper. You can find more details about mill-scip in the repo but the general workflow is: - It's an external Mill plugin that gathers everything needed to do a compile via Mill, but then adds some extra plugins/scalacOptions to ensure semanticDB gets produced - Once produced it uses `scip-java` as a library to actually produce the scip file. The plugin isn't actually a dependency here which ensures we don't have any cyclical dependency on anything, but rather just utilizes the `--import` functionality of Mill and external plugins. I'll leave a couple more comments on various parts of the PR. _NOTE_ This only currently supports Scala and Scala 3 projects. There are some issues with Java projects which will need some upstream work. You can track that in com-lihaoyi/mill#1983. Refs sourcegraph#306

ckipp01 · 2022-08-16T06:46:56Z

Gentle ping on this @olafurpg in case you're not watching the PRs. I'm assuming you're the one that will review this.

olafurpg

This looks great! Thanks for the ping. I've been on vacation and I didn't see this PR until now 😅

This reverts commit fe2c71c. This change was not needed since the working directory of the process is already `index.app.env.workingDirectory`

olafurpg · 2022-08-16T08:33:08Z

The CI is failing because mill is not installed on the system. Ideally, I'd prefer if the test case included a bootstrapping mill script in the root of the generated repo so that users can sbt test without having to pre-install mill. @ckipp01 could you look into this? I recall that Mill has some bootstrapping script but I'm not familiar with how to install it.

You can BTW run arbitrary code to copy the bootstrap script through the initCommand = parameter of the checkBuild(...) method. See GradleBuildToolSuite for how it's used to create a Gradle wrapper script.

ckipp01 · 2022-08-16T10:42:27Z

The CI is failing because mill is not installed on the system. Ideally, I'd prefer if the test case included a bootstrapping mill script in the root of the generated repo so that users can sbt test without having to pre-install mill. @ckipp01 could you look into this? I recall that Mill has some bootstrapping script but I'm not familiar with how to install it.

You can BTW run arbitrary code to copy the bootstrap script through the initCommand = parameter of the checkBuild(...) method. See GradleBuildToolSuite for how it's used to create a Gradle wrapper script.

Ahh yea of course, I didn't think of that. I went ahead and added the mill script in test resources and will now copy it over. However the tests will still fail. From my comment up above:

So this doesn't pass right now because to my understanding the tests rely on being able to pass in targetRoot which mill-scip doesn't support. I didn't really see a reason to when creating it, but I guess I could add it if it's a must to be used here. However I notice the sbt test sufffers from the same thing I think. Let me know what you think.

So in the test output you can see it actually finds the SemanticDB and creates the index, but the tests end up looking in the wrong place for it.

This looks great! Thanks for the ping. I've been on vacation and I didn't see this PR until now 😅

Also, welcome back 😄. I hope you had a good holiday!

From the CI logs

Staring to create a scip index from 2 SemanticDB documents.
[1/1] io.kipp.mill.scip.Scip.generate | [0/2] processing semanticdb
ill.scip.Scip.generate | [2/2] Finished creating your scip index

This is what I was referring to since I don't take into account --targetroot. So I can either change the test setup to maybe look through a fallback directory, or I'd need to add in the ability to handle targetroot handed in to Mill. However, I just wasn't really sure in what scenario that'd be used and wanted to avoid adding it until there was a use case. wdyt?

Nevermind, ignore that and just look at the updated comment down below.

ckipp01 · 2022-08-16T11:11:32Z

Actually, I was just able to add in a targetRoot to checkBuild. Now we'll default to what we were doing before unless it's set, and then we'll look there. We sort of cheat this in Mill not because Mill cares that we're passing it it, but instead the test knows where to look for the packages and semanticdb.

This is a bit opinionated, so no worries if you'd rather not, but when working on sourcegraph#477 I had a pretty hard time with Bloop, but when using sbt things seems to go much smoother. This pr does a couple things: - Updates the contributing docs from "don't use <insert-editor>" -> Feel free to use Metals with lesser support than IntelliJ will give you. It also then recommends using sbt as your build server. - Remove the explicit dependency on `sbt-bloop`. This both gets rid of some complexity in the build by removing the Bloop-explicit stuff which I assume most non-Scala devs aren't familiar with. - Removes the `BuildInfo.bloopVersion` which didn't seem to be used anywhere.

olafurpg · 2022-08-17T08:47:04Z

Released scip-java v0.8.4 yesterday and was able to scip-java index the coursier/coursier repo thanks to this contribution! Very cool 😄

ckipp01 mentioned this pull request Aug 2, 2022

feat: add support for the Mill build tool #476

Closed

ckipp01 commented Aug 2, 2022

View reviewed changes

tests/buildTools/src/test/scala/tests/MillBuildToolSuite.scala Show resolved Hide resolved

scip-java/src/main/scala/com/sourcegraph/scip_java/buildtools/MillBuildTool.scala Show resolved Hide resolved

ckipp01 force-pushed the mill branch from 0aa8e8f to c0fa032 Compare August 12, 2022 12:49

Use millFile instead of ./mill

fe2c71c

olafurpg approved these changes Aug 16, 2022

View reviewed changes

olafurpg enabled auto-merge August 16, 2022 08:04

olafurpg added 3 commits August 16, 2022 10:11

Fix import ordering

870685d

Fix MissingBuildToolSuite

60daded

Revert "Use millFile instead of ./mill"

bee918d

This reverts commit fe2c71c. This change was not needed since the working directory of the process is already `index.app.env.workingDirectory`

refactor(test): add millw into test resources

4117135

auto-merge was automatically disabled August 16, 2022 10:40
Head branch was pushed to by a user without write access

ckipp01 requested a review from olafurpg August 16, 2022 10:42

docs: change minimal mill to 0.10.0

2d1ca93

olafurpg enabled auto-merge August 16, 2022 10:47

refactor(test): add in customizable targetRoot

156a6dc

auto-merge was automatically disabled August 16, 2022 11:10
Head branch was pushed to by a user without write access

ckipp01 mentioned this pull request Aug 16, 2022

refactor(build): remove explicit dependency on Bloop #483

Merged

olafurpg merged commit 3fc0b22 into sourcegraph:main Aug 16, 2022

ckipp01 deleted the mill branch August 16, 2022 12:53

ckipp01 mentioned this pull request Aug 16, 2022

Mill support #306

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for the Mill build tool#477

feat: add support for the Mill build tool#477
olafurpg merged 8 commits intosourcegraph:mainfrom
ckipp01:mill

ckipp01 commented Aug 2, 2022 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

ckipp01 commented Aug 16, 2022

Uh oh!

olafurpg left a comment

Uh oh!

olafurpg commented Aug 16, 2022

Uh oh!

ckipp01 commented Aug 16, 2022 •

edited

Loading

Uh oh!

ckipp01 commented Aug 16, 2022

Uh oh!

olafurpg commented Aug 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ckipp01 commented Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

Uh oh!

Uh oh!

Uh oh!

ckipp01 commented Aug 16, 2022

Uh oh!

olafurpg left a comment

Choose a reason for hiding this comment

Uh oh!

olafurpg commented Aug 16, 2022

Uh oh!

ckipp01 commented Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ckipp01 commented Aug 16, 2022

Uh oh!

olafurpg commented Aug 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ckipp01 commented Aug 2, 2022 •

edited

Loading

ckipp01 commented Aug 16, 2022 •

edited

Loading