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.

Sign up for GitHub

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

Jump to bottom

feat: add support for the Mill build tool #477

Merged
merged 8 commits into from
Aug 16, 2022
Merged

feat: add support for the Mill build tool #477

merged 8 commits into from
Aug 16, 2022
+365 −7

Conversation

ckipp01
Copy link
Contributor

@ckipp01 ckipp01 commented Aug 2, 2022 •
edited
Loading

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

@ckipp01 ckipp01 mentioned this pull request Aug 2, 2022
Closed
@ckipp01
feat: add support for the Mill build tool
c0fa032 
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
Copy link
Contributor Author

ckipp01 commented Aug 16, 2022

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
olafurpg approved these changes Aug 16, 2022
View reviewed changes
Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

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

@olafurpg olafurpg enabled auto-merge August 16, 2022 08:04
@olafurpg
Fix import ordering
870685d
@olafurpg
Fix MissingBuildToolSuite
60daded
@olafurpg
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`
@olafurpg
Copy link
Member

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.

auto-merge was automatically disabled August 16, 2022 10:40

Head branch was pushed to by a user without write access

@ckipp01
Copy link
Contributor Author

ckipp01 commented Aug 16, 2022 •
edited
Loading

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 ckipp01 requested a review from olafurpg August 16, 2022 10:42
@olafurpg olafurpg enabled auto-merge August 16, 2022 10:47
auto-merge was automatically disabled August 16, 2022 11:10

Head branch was pushed to by a user without write access

@ckipp01
Copy link
Contributor Author

ckipp01 commented Aug 16, 2022

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.

ckipp01 added a commit to ckipp01/scip-java that referenced this pull request Aug 16, 2022
@ckipp01
dep: remove explicit reliance on Bloop
209183f 
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.
ckipp01 added a commit to ckipp01/scip-java that referenced this pull request Aug 16, 2022
@ckipp01
refactor(build): remove explicit dependency on Bloop
8389929 
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.
@ckipp01 ckipp01 mentioned this pull request Aug 16, 2022
Merged
@olafurpg olafurpg merged commit 3fc0b22 into sourcegraph:main Aug 16, 2022
@ckipp01 ckipp01 deleted the mill branch August 16, 2022 12:53
ckipp01 added a commit to ckipp01/scip-java that referenced this pull request Aug 16, 2022
@ckipp01
refactor(build): remove explicit dependency on Bloop
fb06640 
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.
@ckipp01 ckipp01 mentioned this pull request Aug 16, 2022
Closed
@olafurpg
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olafurpg olafurpg

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ckipp01 @olafurpg