Exclude folders bsp #3329

pawelsadlo · 2024-08-01T13:16:30Z

There are several limitations implied by intellijBSP client which led to this implementation:
mainly:

There is no way to exclude a directory which is not inside a module content root aka baseDirectory (at least I didnt find a way - excluding .bsp is hardcoded in intellijBSP).
Intellij won't include content root of module if it has no sources defined (this might be a bug in intellijBSP).

This implies that there should be at least one module which content root is equal to project root.

In some sitiations such module is defined by user in build.sc, but not always.
In other cases, a SyntheticRootBspBuildTargetData with bspTarget.baseDirectory == topLevelProjectRoot would be created to handle exclusions.

I went for a solution that is very local to bsp module.

Alternative solutions I though about:

make RootModule extend BSPModule
create synthetic RootModule with BspModule either during bootstrap if there is no module with BspTarget.baseDirectory = topLevelProjectDirectory
create synthetic RootModule with BspModule in bspWorker State

However personally I think that alternative solutions seem overkill for achieving such simple effect as excluding directories.

How it works:

MillBuildServer.State contains additional field - Option[SyntheticRootBspBuildTargetData]
SyntheticRootBspBuildTargetData contains minimal data required for BuildTarget to be reported bydef workspaceBuildTargets (forced by limitation 1).
this field will be Some only if there is no normal module with BspBuildTarget.contentRoot == topLevelProjectRoot
aka. module which can exclude topLevel folders
if SyntheticRootBspBuildTargetData is created, it is reported in def workspaceBuildTargets together with other modules, but filtered out from other bspClient requests (exceptions: buildOutputPaths, and buildTargetSources)
def buildTargetOutputPaths - for a module which has BspBuildTarget.contentRoot == topLevelProjectRoot (so potentially SyntheticRootBspBuildTargetData) i os.walk from topLevelProjectRoot looking for build.sc files and in their folders I exclude ".idea",".bsp","out",".bloop". - os.walk ensures exclusion of folders in nested mill projects
note: following paths are excluded from walk
```
def ignore(path: os.Path): Boolean = {
      path.last.startsWith(".") ||
      path.endsWith(os.RelPath("out")) ||
      path.endsWith(os.RelPath("target")) ||
      path.endsWith(os.RelPath("docs")) ||
      path.endsWith(os.RelPath("mill-build"))
    }
```

Actually if there was no limitations I mentioned, this would be the only required step.
5. Because of limitation 2 i had to report some source for SyntheticRootBspBuildTargetData - chosen "src", but this can be any, even non existing path.

Tested manually:

Navigation of sources
Navigation of meta-builds
Navigation of build.sc
excluded folders appear in IDE
solution does not collide with user-made RootModule in build.sc (actually it has to be a RootModule with JavaModule to be seen by BSP)

…bject

lihaoyi · 2024-08-01T13:30:21Z

Thanks for the PR! Given the topic at hand, I wonder if @jastice has any opinions here on what's the right way to do things. He's the author of BSP on the intellij side, and would likely know more than me the limitations, potential workarounds, and expected future development of the IntelliJ BSP client

jastice · 2024-08-01T13:44:22Z

Forwarding to @AnthonyGrod who is currently working on integrating Mill with the new IJ BSP plugin

…de-folders-bsp

AnthonyGrod · 2024-08-02T11:43:51Z

Hello, at the moment I'm focusing on very basic functionalities of Mill over IntelliJ-BSP plugin integration, but I'll be happy to help when I will finish the basic support.

lihaoyi · 2024-08-02T13:48:52Z

@pawelsadlo can you explain in the PR description how your solution works?

pawelsadlo · 2024-08-02T15:31:28Z

@lihaoyi wrote a description, if anything needs elaboration, tag me

lihaoyi · 2024-08-02T16:26:18Z

Thanks! Will take a look. @lefou should probably review this too since he's more familiar with the BSP side of things

lihaoyi · 2024-08-03T19:24:28Z

At a quick glance this looks reasonable to me

bsp/worker/src/mill/bsp/worker/MillBuildServer.scala

bsp/worker/src/mill/bsp/worker/Utils.scala

…de-folders-bsp

lihaoyi · 2024-08-05T13:18:06Z

bsp/worker/src/mill/bsp/worker/Utils.scala

+      path.last.startsWith(".") ||
+      path.endsWith(os.RelPath("out")) ||
+      path.endsWith(os.RelPath("target")) ||
+      path.endsWith(os.RelPath("docs")) ||


Why are we ignoring docs/? I'm not aware of such a folder name being treated specially by any build tool

Those are patches ignored during the os.walk in the search of build.sc files (so basically a subprojects), I assumed that we don't expect a subproject to bo located inside docs directory.

The logic is:

We recursively search for build.sc files under top level root of our project. (Directories containing build.sc are potential directories where out, .idea, .bsp , .bloop would be generated).
I search for build.sc instead of .idea, out, .bsp and .bloop because the latter may not be present at the time of bsp import, and may appear later (for example during build of subproject)

We exclude .bsp,.idea,.bloop,out from folders of found in step 1.

lefou

I still think we try to fight a symptom of a bad behaving BSP client here. BSP is about declaring what makes a project, not what-not makes a project. If the BSP client (the IDE) handles more than that, we simply can't and shouldn't control that via BSP.

Hence, I don't think I can properly review this PR, as I don't agree on it's purpose.

lefou · 2024-08-06T06:07:16Z

bsp/worker/src/mill/bsp/worker/Utils.scala

+    def additionalExclusions = projectsRootPaths.flatMap { path =>
+      Seq(
+        outputPathItem(path / ".idea"),
+        outputPathItem(path / "out"),
+        outputPathItem(path / ".bsp"),
+        outputPathItem(path / ".bloop")
+      )
+    }


I'm not convinced excluding hardcoded directories is the right approach for a generic protocol. Esp. since some of these directories aren't even created or managed by us.

lihaoyi · 2024-08-06T18:10:55Z

@pawelsadlo can you point out where in the code we set the excluded folders to be marked as excluded in BSP, and therefore in IntelliJ? I've been trying to ready through the code and haven't figured out how it works, although testing it out it does the right thing

@lefou I think we should do a best-effort here. It's not super elegant, and maybe it should be the IDE's responsibility to read the gitignore file and ignore folders. IntelliJ actually does have that functionality, but it's not automatic (you have to navigate to gitignore and click click), and it doesn't seem fully functional (e.g. it doesn't help you exclude .idea/). I agree it's not great from an implementation architecture perspective, and it's not really generalizable, but I think it's worth doing regardless. If IntelliJ starts handling this automatically on their side at some point in the future, we can re-consider and remove our special casing as necessary

lihaoyi · 2024-08-06T18:27:52Z

bsp/worker/src/mill/bsp/worker/Utils.scala

+    def ignore(path: os.Path): Boolean = {
+      path.last.startsWith(".") ||
+      path.last == "out" ||
+      path.last == "target"
+    }


@pawelsadlo rather than hardcoding things here, and below in additionalExclusions, let's use the .gitignore. We can use JGit to perform these git check-ignore checks quickly without the subprocess overhead using the classes from org.eclipse.jgit.ignore.*. We'd also need to check the equivalent of git ls-files (e.g. TreeWalk) to see if the folder has been added to the index, since folders explicitly added to the git index bypass the gitignore. That will generalize this to support everything that is gitignored, which is probably what a developer working with an arbitrary git codebase wants.

Special-casing git should be the right thing for 99% of developers who use Git for version control today. If .gitignore is not present, we can fall back to only ignoring the top-level out/, .idea/, and .bsp/ folders. I don't think we should traverse recursively and try to find stuff in subfolders.

It's possible someone will come in future asking for hg integration or something, but we can cross that bridge when we get to it

Of course ,however I am on vacation now, I will do this as soon as I have access to my laptop.

Sure! no hurry

lihaoyi · 2024-08-06T18:29:48Z

@pawelsadlo can you also manually test to verify that generated source files inside the excluded out/ folder gets correctly picked up as sources, and doesn't get excluded accidentally?

pawelsadlo · 2024-08-05T17:07:19Z

bsp/worker/src/mill/bsp/worker/Utils.scala

+        sanitizeUri(path) + "/",
+        OutputPathItemKind.DIRECTORY
+      )
+    def additionalExclusions = projectsRootPaths.flatMap { path =>


@lihaoyi these folders are excluded ones, they then are sent to bsp by buildoutputpaths

Got it. Is everything in buildTargetOutputPaths considered excluded?

Formally, according to BSP docs it's BSP client decision what to do with buildOutputPaths response from BSP server. most of clients are excluding them, in particular intellij.
What's more ,in case of intellij there are the limitations I mentioned in the PR description (these are not formalized though ,it's just what I have observed by reading through intellij bsp source code and confirmed by testing )

Got it. Can you also manually test VSCode+Metals to see what the behavior is there? Just so we know what affect this PR will have on those users (if any)

pawelsadlo · 2024-08-07T16:18:14Z

@pawelsadlo can you also manually test to verify that generated source files inside the excluded out/ folder gets correctly picked up as sources, and doesn't get excluded accidentally?

Sure

lihaoyi · 2024-08-19T10:46:49Z

@pawelsadlo just bump on this :) if you don't have the time to continue, I can pay out half the bounty and finish off the PR myself

pawelsadlo · 2024-08-19T12:05:49Z

@pawelsadlo just bump on this :) if you don't have the time to continue, I can pay out half the bounty and finish off the PR myself

@lihaoyi I'm ok with half of bounty , I'll be busy next week anyway, so can't do it soon.

lihaoyi · 2024-08-22T07:38:49Z

I took over the PR and removed the os.walk logic and made it ignore only top-level out/.bsp/.idea/.bloop for now. Further improvements to integrate with .gitignore etc. can come, but this should get us 90% of the value without too much complexity

Manually tested with normal Scala and Java projects with root module, without root module, and with meta-build. All seem to work, with navigation intact in all source and build files and the out/ and .idea/ folders correctly ignored

lihaoyi · 2024-08-22T08:49:56Z

@pawelsadlo can send me an email with your bank transfer details and i can close out the bounty

pawelsadlo · 2024-08-24T06:19:20Z

@lihaoyi sent you the details

pawelsadlo added 12 commits August 1, 2024 13:26

adding SyntheticRootBspBuildTargetData

d43aff2

adding topLevelBuildRopt param to BspServer, BspWorker, State

4407915

add syntheticRootBspBuildTarget to State

a058b18

extract makeBuildTarget to Utils, create outputPaths

45d1d9e

extract makeBuildTarget to Utils, create outputPaths

7526305

filter synthetic module from bspClient requests

ce5c96e

change completableTasks, handle syntheticRootmodule

22e7874

fixing name

a695dba

extract SyntheticRootBspBuildTargetData creation logic to companion o…

5ef1b67

…bject

docs

870acf5

moving syntheticRootBspBuildTarget creation logic to companion object

c0207b5

fixing agg

d547e34

pawelsadlo and others added 5 commits August 1, 2024 15:59

format

613eb59

Merge branch 'main' into exclude-folders-bsp

5786d81

format

0107574

Merge remote-tracking branch 'ps_mill/exclude-folders-bsp' into exclu…

fde6910

…de-folders-bsp

format

2ade14a

pawelsadlo marked this pull request as ready for review August 1, 2024 15:38

Update SyntheticRootBspBuildTargetData.scala

392280b

lihaoyi requested review from lihaoyi and lefou August 2, 2024 16:26

lihaoyi reviewed Aug 3, 2024

View reviewed changes

bsp/worker/src/mill/bsp/worker/MillBuildServer.scala Show resolved Hide resolved

lihaoyi reviewed Aug 3, 2024

View reviewed changes

bsp/worker/src/mill/bsp/worker/Utils.scala Outdated Show resolved Hide resolved

pawelsadlo added 2 commits August 4, 2024 10:31

review suggestions

a0e8162

Merge remote-tracking branch 'ps_mill/exclude-folders-bsp' into exclu…

445d76d

…de-folders-bsp

lihaoyi reviewed Aug 5, 2024

View reviewed changes

Update Utils.scala

6dee463

lihaoyi approved these changes Aug 6, 2024

View reviewed changes

lefou reviewed Aug 6, 2024

View reviewed changes

lihaoyi reviewed Aug 6, 2024

View reviewed changes

pawelsadlo commented Aug 7, 2024

View reviewed changes

lihaoyi added 8 commits August 22, 2024 14:50

top level ignored folders only

ce1e933

.

94a01c9

Merge branch 'main' into 3329

800c87b

.

7a6e3b3

.

2533d49

.

3172101

.

ea37b64

Merge branch 'main' into 3329

362981f

.

1c8ad5f

lihaoyi merged commit 5041301 into com-lihaoyi:main Aug 22, 2024
31 checks passed

lihaoyi added this to the 0.12.0 milestone Aug 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exclude folders bsp #3329

Exclude folders bsp #3329

pawelsadlo commented Aug 1, 2024 •

edited

Loading

lihaoyi commented Aug 1, 2024

jastice commented Aug 1, 2024

AnthonyGrod commented Aug 2, 2024 •

edited

Loading

lihaoyi commented Aug 2, 2024

pawelsadlo commented Aug 2, 2024

lihaoyi commented Aug 2, 2024

lihaoyi commented Aug 3, 2024

lihaoyi Aug 5, 2024

pawelsadlo Aug 5, 2024 •

edited

Loading

lefou left a comment •

edited

Loading

lefou Aug 6, 2024 •

edited

Loading

lihaoyi commented Aug 6, 2024 •

edited

Loading

lihaoyi Aug 6, 2024 •

edited

Loading

pawelsadlo Aug 7, 2024

lihaoyi Aug 7, 2024

lihaoyi commented Aug 6, 2024

pawelsadlo Aug 5, 2024

lihaoyi Aug 7, 2024

pawelsadlo Aug 7, 2024

lihaoyi Aug 8, 2024

pawelsadlo commented Aug 7, 2024

lihaoyi commented Aug 19, 2024

pawelsadlo commented Aug 19, 2024

lihaoyi commented Aug 22, 2024 •

edited

Loading

lihaoyi commented Aug 22, 2024

pawelsadlo commented Aug 24, 2024 •

edited

Loading

Exclude folders bsp #3329

Exclude folders bsp #3329

Conversation

pawelsadlo commented Aug 1, 2024 • edited Loading

lihaoyi commented Aug 1, 2024

jastice commented Aug 1, 2024

AnthonyGrod commented Aug 2, 2024 • edited Loading

lihaoyi commented Aug 2, 2024

pawelsadlo commented Aug 2, 2024

lihaoyi commented Aug 2, 2024

lihaoyi commented Aug 3, 2024

lihaoyi Aug 5, 2024

Choose a reason for hiding this comment

pawelsadlo Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

lefou left a comment • edited Loading

Choose a reason for hiding this comment

lefou Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

lihaoyi commented Aug 6, 2024 • edited Loading

lihaoyi Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

pawelsadlo Aug 7, 2024

Choose a reason for hiding this comment

lihaoyi Aug 7, 2024

Choose a reason for hiding this comment

lihaoyi commented Aug 6, 2024

pawelsadlo Aug 5, 2024

Choose a reason for hiding this comment

lihaoyi Aug 7, 2024

Choose a reason for hiding this comment

pawelsadlo Aug 7, 2024

Choose a reason for hiding this comment

lihaoyi Aug 8, 2024

Choose a reason for hiding this comment

pawelsadlo commented Aug 7, 2024

lihaoyi commented Aug 19, 2024

pawelsadlo commented Aug 19, 2024

lihaoyi commented Aug 22, 2024 • edited Loading

lihaoyi commented Aug 22, 2024

pawelsadlo commented Aug 24, 2024 • edited Loading

pawelsadlo commented Aug 1, 2024 •

edited

Loading

AnthonyGrod commented Aug 2, 2024 •

edited

Loading

pawelsadlo Aug 5, 2024 •

edited

Loading

lefou left a comment •

edited

Loading

lefou Aug 6, 2024 •

edited

Loading

lihaoyi commented Aug 6, 2024 •

edited

Loading

lihaoyi Aug 6, 2024 •

edited

Loading

lihaoyi commented Aug 22, 2024 •

edited

Loading

pawelsadlo commented Aug 24, 2024 •

edited

Loading