Add support for packagePrefix #1183
Conversation
|
@jvican Could you take a look at this PR and let me know if it makes sense from bloops perspective, and if not what would be the reasonable place to put the package prefix field? |
lukaszwawrzyk
force-pushed
the
package-prefix
branch
from
361fca5
to
304b2ff
Compare
|
Will have a look at this shortly @lukaszwawrzyk |
| @@ -234,6 +234,7 @@ object Config { | |||
| workspaceDir: Option[Path], | |||
| sources: List[Path], | |||
| sourcesGlobs: Option[List[SourcesGlobs]], | |||
| sourceRoots: Option[List[String]], | |||
There was a problem hiding this comment.
Should this be Option[List[Path]]?
There was a problem hiding this comment.
Even if bloop doesn't need this typed as a Path, I believe it's more readable for clients using bloop-config that this should be a path and not a random string. Additionally, Path uses structural sharing resulting in lower memory usage.
| @@ -234,6 +234,7 @@ object Config { | |||
| workspaceDir: Option[Path], | |||
| sources: List[Path], | |||
| sourcesGlobs: Option[List[SourcesGlobs]], | |||
| sourceRoots: Option[List[String]], | |||
There was a problem hiding this comment.
Even if bloop doesn't need this typed as a Path, I believe it's more readable for clients using bloop-config that this should be a path and not a random string. Additionally, Path uses structural sharing resulting in lower memory usage.
project/Dependencies.scala
Outdated
| @@ -13,7 +13,7 @@ object Dependencies { | |||
| val nailgunCommit = "a2520c1e" | |||
|
|
|||
| val zincVersion = "1.3.0-M4+32-b1accb96" | |||
| val bspVersion = "2.0.0-M5" | |||
| val bspVersion = "2.0.0-M5+1-e75ba0aa" // TODO bump bsp to make it compile | |||
There was a problem hiding this comment.
The release is out now
| val bspVersion = "2.0.0-M5+1-e75ba0aa" // TODO bump bsp to make it compile | |
| val bspVersion = "2.0.0-M7" // TODO bump bsp to make it compile |
lukaszwawrzyk
force-pushed
the
package-prefix
branch
from
04f92d0
to
ce22610
Compare
lukaszwawrzyk
force-pushed
the
package-prefix
branch
from
ce22610
to
fd1d50e
Compare
There was a problem hiding this comment.
LGTM 👍 Ideally it would be great to have tests with a change like this but this new field is not relevant for sbt, mill, maven or gradle. We should consider using something like QuickBuild from Metals that generates a multi-project build from a single high-level JSON file
I'm not sure if we should block this PR on tests however. What do you think @jvican?
|
The Windows CI failure looks unrelated |
We can go ahead and merge, no problem. I encourage @lukaszwawrzyk to add some tests for this when he's implemented it; the more tested every feature is the better. |
jvican
added
build server
See build-server-protocol/build-server-protocol#109 for details
I added packagePrefix at project/build target level. Not sure if it should be more granular, and if so, where exactly it should be located as we have sources that is a list of files or dirs but also sourceGlobs. Sources seems to usually contain specific files, attaching packagePrefix to each of these would be quite strange.