[internal] BSP: use seperate config file for defining groups for BSP build targets

[tdyas]

Introduce a separate config file in TOML format for defining groupings of source code used in computing BSP build targets. This allows more metadata to be specified in the definition of BSP build targets without making configuration in pants.toml unwieldy. The --experimental-bsp-groups-config-files option takes a list of config file paths that are merged together to form the final set of groups used to compute the BSP build targets.

Each config file contains a groups dictionary where entries look as follows:

# The dictionary key is used to identify the group. It must be unique.
[groups.ID1]:
# One or more Pants address specs defining what targets to include in the group.
addresses = [
    "src/jvm::",
    "tests/jvm::",
]
# Filter targets to a specific resolve. Targets in a group must be from a single resolve.
# Format of filter is `TYPE:RESOLVE_NAME`. The only supported TYPE is `jvm`. RESOLVE_NAME must be
# a valid resolve name.
resolve = "jvm:jvm-default"
display_name = "Display Name"  # (Optional) Name shown to the user in the IDE.
base_directory = "path/from/build/root"  # (Optional) Hint to the IDE for where the build target should "live."

Note: Filtering by resolve will be implemented in a follow-on PR since it entails changes to interfaces between core BSP rules and language-specific BSP rules which would make this PR more complex than it needs to be.

[stuhood]

Before we go ahead with landing the "manually define the BSP build targets" approach, it would be good to sketch out some more design on #14563 (or another ticket).

Because I continue to think that while "which Pants targets should have BSP build targets" makes sense as something to configure using a config file like this, which build targets are actually created out of the selected subset is something that we can automate. And I feel like that would probably be good, since it avoids needing to think about how to name (or display name) build targets, and avoids putting users in situations where they might create cycles between the targets which they have manually defined.

[tdyas]

which build targets are actually created out of the selected subset is something that we can automate

Given our prior discussions, I thought the plan was for ./pants experimental-bsp to write out a default config file. This PR does not attempt to implement that logic. I still think it is premature to remove the configurability from the user, not knowing how such default config will work out in practice.

[stuhood]

I still think it is premature to remove the configurability from the user

That is not what I am suggesting. I said:

while "which Pants targets should have BSP build targets" makes sense as something to configure using a config file like this ...

i.e.: I agree that a user should configure the subset of the repo that is loadable as targets. But I don't think that they should have to name or declare individual build targets: those can be calculated from the subset.

Here is the whole workflow I was thinking of:

1. A config file describes which subset of the repo should be loadable as build targets using a CLI spec (and a resolve filter). The config file does not name the build targets, or need to define more than one.
2. When a BSP connection is established, the config file declares the "roots" for the run: everything reachable from the roots (with the filter) is "eligible".
3. The "eligible" Pants targets are coarsened/grouped into BSP build targets automatically, using a combination of the algorithm behind CoarsenedTarget and grouping by directory to ensure that each BSP build target completely owns its directory.

The advantage here (as mentioned above) is not needing to name build targets, or define multiple build targets such that they don't form cycles (since the cycle removal is automated: happy to help implement that).

[tdyas]

The "eligible" Pants targets are coarsened/grouped into BSP build targets automatically, using a combination of the algorithm behind CoarsenedTarget and grouping by directory to ensure that each BSP build target completely owns its directory.

IntelliJ's data model assumes that IntelliJ modules will basically own entire source roots, not just a subset of directories and files under a source root. "CoarsenedTarget and grouping by directory" is actually still much too fine-grained to use for aggregating Pants targets into BSP build targets. The closest analogue would be to compare IntelliJ modules to sbt or maven projects.

A config file describes which subset of the repo should be loadable as build targets using a CLI spec (and a resolve filter). The config file does not name the build targets, or need to define more than one.

This does not help differentiate between two source roots that should be BSP build targets (e.g. foo/src/jvm and bar/src/jvm source roots) versus source roots that should be one BSP build target (src/jvm and test/jvm). I would rather not have to make Pants opinionated about trying to figure that out just yet. The config file in this PR allows that decision to be punted until after some research is done, and after getting users to try out BSP.

It is experimental for a reason, so we can research while getting user feedback.

[stuhood]

"CoarsenedTarget and grouping by directory" is actually still much too fine-grained to use for aggregating Pants targets into BSP build targets. The closest analogue would be to compare IntelliJ modules to sbt or maven projects.

If the granularity that you're going for is "sbt or maven projects", the effect of this config file as implemented ("manually declared targets") would be that in addition to declaring your targets in your BUILD files, you would also need to declare on the order of dozens to hundreds of non-cyclic build targets in this file.

Additionally, AFAICT from #14885, the primary unit of progress is "compilation or testing has completed for $buildtarget": we shouldn't expect users to need to manually declare lots of build targets in order to get useful progress information.

On the other hand, the number of distinct directories in a project should fall somewhere between the number of files, and the number of hypothetical "sbt or maven projects" / "src directories". It would be much less fine grained than Pants targets in general, and probably closer to the other end of the spectrum.

This does not help differentiate between two source roots that should be BSP build targets (e.g. foo/src/jvm and bar/src/jvm source roots) versus source roots that should be one BSP build target (src/jvm and test/jvm).

Is it actually necessary for src vs tests to be grouped into a single BSP target...? That would be surprising.

[tdyas]

Is it actually necessary for src vs tests to be grouped into a single BSP target...? That would be surprising.

It is not, but maybe users would want to do that? I don't know either way. The sbt BSP integration creates separate main and test BSP build targets for each sbt project.

[tdyas]

Additionally, AFAICT from #14885, the primary unit of progress is "compilation or testing has completed for $buildtarget": we shouldn't expect users to need to manually declare lots of build targets in order to get useful progress information.

That is not the case. The BSP protocol progress indicator notification uses an abstract notion of "work units" with integer fields for total and progress. The BSP server can define those however it wants.

[stuhood]

Additionally, AFAICT from #14885, the primary unit of progress is "compilation or testing has completed for $buildtarget": we shouldn't expect users to need to manually declare lots of build targets in order to get useful progress information.

That is not the case. The BSP protocol progress indicator notification uses an abstract notion of "work units" with fields for integer fields for total and progress. The BSP server can define that however it wants.

Yea, having looking at it a bit more, that makes sense. build/publishDiagnostics is the more likely target for streaming status for "parts" of a particular compile.

But I think that this point ("unit of work") still applies when it comes to what the IDE will allow a user to actually ask to be compiled or tested. For example: if I want to compile or test one directory, but the defined build target is larger than that, I'll still end up compiling/testing "everything in the build target".

[tdyas]

But I think that this point ("unit of work") still applies when it comes to what the IDE will allow a user to actually ask to be compiled or tested. For example: if I want to compile or test one directory, but the defined build target is larger than that, I'll still end up compiling/testing "everything in the build target".

This assumes that the IDE does not merge build targets when mapping to its own data model. IntelliJ's Scala plugin does that if build targets share the same "base directory." I don't know offhand though how that impacts trying to build the resulting merged IntelliJ module.

https://github.com/JetBrains/intellij-scala/blob/6d248c5d50d0389cbe6f0f8855274d397f485b39/bsp/src/org/jetbrains/bsp/project/importing/BspResolverLogic.scala#L168-L171

[stuhood]

@tdyas and I discussed this out of band, and came to the following understanding:

• There was some confusion on this issue because the "build targets" in the config are currently doing double duty by acting both as:
  1. The "project"/"scope"/"team" selection method (To Be Named: abbreviated as p,s,t from here on), so that after loading the BSP connection file, users would interact with the "build target" that is equivalent to their p,s,t.
  2. The unit of compilation/testing.
• We will need to disconnect the p,s,t selection method from the unit of compilation / testing, either by:
  • Having the ./pants bsp goal accept a p,s,t name to load, and expect than name to live in one of the config files.
  • Having the ./pants bsp goal accept a single, explicit config file to load.

The goal of differentiating the p,s,t from the "BSP build target" would be to give us the flexibility to create more than one build target in the future (even if for now we only create one).

[stuhood]

Given that, there are a few naming and semantics changes which should be made to this config format to decouple the p,s,t concept from "build targets" (and the config file should maybe be an explicit argument to the ./pants bsp goal, depending on the answer to the "p,s,t selection" question above.)

[tdyas]

Latest commit renames "targets" to "groups" in the BSP TOML config files.

[stuhood]

Thanks. I think that making the selection change here would make this code much simpler, so I'd suggest doing that... but not a blocker.

[stuhood]

Based on my comment about disconnecting the selection of the group (p,s,t) from the number of build targets created, I think that you will either want to:

1. add another option which selects which group to load from the config
2. remove support for putting more than one group in a file (and for multiple files), and then have this option point to a single file which defines a single group.

My preference would be for 2, both because it avoids coupling multiple teams/groups together unnecessarily, and because it's simpler, with only one option overall. But not blocking.

[tdyas]

My preference is (1). Consider that the user may more than one group active. They could be part of multiple teams (but still not all teams); or common infrastructure libraries could be their own group and the user wants to activate both their own team and the common team.

[tdyas]

(with the option specifying 1+ groups to select)

[tdyas]

I'll rename the option as well so it does not have "targets" in the name.