Mill support: enable importing projects which contain build.mill(.scala) but not wrapper script #674
Conversation
…la) but not wrapper script Not all Mill projects necessarily have a wrapper script in the root. The wrapper script may be on the PATH. But _all_ Mill projects contain `build.mill` or `build.mill.scala`, so that's _the_ robust way to recognize them.
|
What about pre-0.12 projects, they would have |
|
They would need to update to >= 0.12? My attitude towards this is "who cares about old Mill"? But if there's an actual demand, I'm not opposed. And the change is trivial, so I actually did it :) |
This info may be still valuable to debug IDE issues as well as running with more rare setups like `vim` or `emacs`. Inspired by JetBrains/intellij-scala#674 from https://github.com/sideeffffect
|
"old mill" Right now I'm blocked on com-lihaoyi/mill#3955, which was fixed but needs to be released as 0.12.3 |
|
@nafg fair enough. Btw I thought I did it, but I didn't, so I've made the change now: |
|
Hello @azdrojowa123 @vasilmkd @SrTobi @timaliberdov @unkarjedy , could you please have a look at this PR? 🙏 |
bsp/src/org/jetbrains/bsp/project/importing/MillProjectInstaller.scala
Outdated
Show resolved
Hide resolved
bsp/src/org/jetbrains/bsp/project/importing/MillProjectInstaller.scala
Outdated
Show resolved
Hide resolved
| @@ -34,7 +34,8 @@ class MillProjectInstaller extends BspProjectInstallProvider { | |||
| Success(Seq(file.getAbsolutePath, "-i", "mill.bsp.BSP/install")) | |||
| case Some(file) if isLegacyMill => | |||
| Success(Seq(file.getAbsolutePath, "-i", "mill.contrib.BSP/install")) | |||
| case _ => Failure(new IllegalStateException("Unable to install BSP as this is not a Mill project")) | |||
| case None => | |||
| Success(Seq("mill", "-i", "mill.bsp.BSP/install")) | |||
There was a problem hiding this comment.
Did you check how it would look on UI when there is also no mill available on PATH?
The process will fail with some non zero exit code, but will there be a meaningful error message in the UI?
There was a problem hiding this comment.
I didn't. I assume it will fail the same way as when Seq(file.getAbsolutePath, "-i", "mill.bsp.BSP/install") fails -- which is it will print the errors in the terminal (build view).
There was a problem hiding this comment.
I decided to check this and have some thoughts - in the "old implementation" when Success(Seq(file.getAbsolutePath, "-i", "mill.contrib.BSP/install")) was executed indeed the errors were displayed in the terminal, but because we were sure that the mill script existed we can assume that these errors were useful (at least to some certain level).
Now If the Mill is not installed globally, a very unclear error message is displayed in the terminal.
To prevent this, we can implement the same solution as in the ScalaCliProjectInstaller. In the #installCommand, there should be a check to verify if Mill is installed globally. If it's not, a more meaningful error message should be displayed. Perhaps the best approach would be to add this check to the #canImport method, but for now, let's stay consistent with the implementation in ScalaCliProjectInstaller. If needed, it can always be refactored and moved in the future.
There was a problem hiding this comment.
In the
#installCommand, there should be a check to verify if Mill is installed globally.
I'm not against. But I'm not familiar with the code base. Can you please make this improvement after this PR is merged?
There was a problem hiding this comment.
@sideeffffect
I will add this before merging (anyway I have to pass it on my own through our internal CI) but I'll take care of it tomorrow :)
bsp/src/org/jetbrains/bsp/project/importing/MillProjectInstaller.scala
Outdated
Show resolved
Hide resolved
|
Hello everybody, what can I do to help this move forward? I see that there is an error in CI, but it doesn't seem related to this PR: Can you please re-run? |
|
The tests failure is indeed unrelated and is actually muted on our CI. |
|
@azdrojowa123 Could you or Antoni please take a look at it and merge it to 243 Nightly via personal branch? |
|
Hello, |
Not all Mill projects necessarily have a wrapper script in the root. The wrapper script may be on the PATH.
But all Mill projects contain
build.millorbuild.mill.scala, so that's the robust way to recognize them./cc @lihaoyi