BSP: do not filter clean-requests for meta-builds #2931
Conversation
Looking at the last change that was made to this code in com-lihaoyi@65fbd53 it's not clear to me why this line was added where we map everything to `BspModule` and then filtering the build targets on that which where in the request. My assumption is that anything that the build client is requesting to clean _should_ be a valid identifier. If not, then that originally came from Mill, so I'm unsure why we have this. Before this change it was causing issues since not everything in `state.rootModules` was a `BspModule` so you'd get a match exception. To expand a bit the build targets here in an example `mill-test` (hello world) project are: ``` BuildTargetIdentifier [ uri = "file:///Users/<me>/Documents/scala-workspace/mill-test/milltest" ],BuildTargetIdentifier [ uri = "file:///Users/<me>/Documents/scala-workspace/mill-test/mill-build" ] ``` But when looking into the `state.rootModules` one of them is ``` /Users/<me>/Documents/scala-workspace/mill-test ``` This was the problematic one. I'm not sure if this is just a mapping issue or not, but the change I made instead just takes the build target identifies given from the client, does a look up in the `state.bspModulesById` mapping, and goes with it. This seems to work and simplifies this unless this is introducing an issue that I don't see. closes com-lihaoyi#2915
|
This is a tricky one. Also, it's most likely underspecified. The issue is, that due to the newly introduced meta-build, all modules of a project depend implicitly on that meta-build. If you clean the meta-build, Mill will need to re-run it before it can understand any new actions. That is probably the reason, why the introduction of the meta-build also excluded it from the clean target. The predecessor of the meta-build was the Ammonite script build, which wasn't included in the In difference to the Mill CLI, where all actions belong to the same build level, in BSP we make the levels transparent, so that all modules and meta-modules can be edited, disregarding of their meta-level. It might be safe to clean all BSP build targets (including the meta-builds), but some review needs to happen whether we properly re-run the meta-build from the next BSP request. In any case, cleaning the meta-build will definitely make the next BSP action take very long. Question is, if that is what the user wants? |
Ahh, ok interesting. Can you clarify what is meant by
In the BSP sense of things, what does this practically mean? Since in this case the user would be requesting a clean/compile, so is there anything else that would really be needed or hindered?
Good question. I'm honestly not really sure. I guess in terms of correctness the BSP server should listen to the request and clean everything that is requested to clean, but like you said, I don't know if a user would really ever want that. I did just check what happens with sbt when using it as your build server and I see the same thing, a request for the build sent in and it is indeed cleaned. Either way, I'm not sure. This was just my attempt to avoid the crash that I was seeing since I kept getting in states that were sort of borked and wanted to see if a full clean/compile would help. |
In the BSP sense, I guess the only impact is that any next request whether already cached or not (even a subsequent I think Mill already takes care of meta-build re-creation. The proof might be, that this PR just works for you. So we could just merge it as-is. |
lefou
changed the title
Looking at the last change that was made to this code in 65fbd53 it's not clear to me why this line was added where we map everything to
BspModuleand then filtering the build targets on that which where in the request. My assumption is that anything that the build client is requesting to clean should be a valid identifier. If not, then that originally came from Mill, so I'm unsure why we have this. Before this change it was causing issues since not everything instate.rootModuleswas aBspModuleso you'd get a match exception.To expand a bit the build targets here in an example
mill-test(hello world) project are:But when looking into the
state.rootModulesone of them isThis was the problematic one. I'm not sure if this is just a mapping issue or not, but the change I made instead just takes the build target identifies given from the client, does a look up in the
state.bspModulesByIdmapping, and goes with it. This seems to work and simplifies this unless this is introducing an issue that I don't see.closes #2915