Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BSP's dependencyModules do not return transitive libraries #3165

Open
tgodzik opened this issue May 17, 2024 · 8 comments
Open

BSP's dependencyModules do not return transitive libraries #3165

tgodzik opened this issue May 17, 2024 · 8 comments
Labels
bug The issue represents an bug

Comments

@tgodzik
Copy link

tgodzik commented May 17, 2024

While investigating scalameta/metals#6419 I realized that dependencyModules returns only the Scala 3 library for a simple Scala 3 project. It's missing the Scala 2.13 library.

My guess is that no transitive dependencies are being returned.

I can work around it by not requesting dependencyModules from Mill BSP server, but I wanted to mention it before doing a workaround.

@lefou
Copy link
Member

lefou commented May 17, 2024

You are right, there is no resolution step in the current implementation. It's probably an oversight.

P.S. I've planned to touch that code as part of a fix for #3148, but that will no happen in the near next days.

@tgodzik
Copy link
Author

tgodzik commented May 17, 2024

There is no hurry, unless this will take more than a month I don't think I need to add a workaround. Thanks!

@tgodzik
Copy link
Author

tgodzik commented May 17, 2024

Or did I misunderstand and it would be better to add it for now?

@lefou lefou added the bug The issue represents an bug label May 17, 2024
@lefou
Copy link
Member

lefou commented May 17, 2024

The fix in the right place, e.g. call resolveDeps, should be easier than any workaround.

override def buildTargetDependencyModules(params: DependencyModulesParams)
: CompletableFuture[DependencyModulesResult] =
completableTasks(
hint = "buildTargetDependencyModules",
targetIds = _ => params.getTargets.asScala.toSeq,
tasks = { case m: JavaModule =>
T.task { (m.transitiveCompileIvyDeps(), m.transitiveIvyDeps(), m.unmanagedClasspath()) }
}
) {
case (
ev,
state,
id,
m: JavaModule,
(transitiveCompileIvyDeps, transitiveIvyDeps, unmanagedClasspath)
) =>
val ivy = transitiveCompileIvyDeps ++ transitiveIvyDeps
val deps = ivy.map { dep =>
// TODO: add data with "maven" data kind using a ...
// MavenDependencyModule
new DependencyModule(dep.dep.module.repr, dep.dep.version)
}
val unmanged = unmanagedClasspath.map { dep =>
new DependencyModule(s"unmanaged-${dep.path.last}", "")
}
new DependencyModulesItem(id, (deps ++ unmanged).iterator.toSeq.asJava)
} {
new DependencyModulesResult(_)
}

@megri
Copy link
Contributor

megri commented May 30, 2024

Any updates on this? Perhaps I can help (I reported the original issue here: scalameta/metals#6419)

@lefou
Copy link
Member

lefou commented May 30, 2024

@megri Go ahead, I haven't started yet.

@megri
Copy link
Contributor

megri commented Jun 14, 2024

The fix in the right place, e.g. call resolveDeps, should be easier than any workaround.

override def buildTargetDependencyModules(params: DependencyModulesParams)
: CompletableFuture[DependencyModulesResult] =
completableTasks(
hint = "buildTargetDependencyModules",
targetIds = _ => params.getTargets.asScala.toSeq,
tasks = { case m: JavaModule =>
T.task { (m.transitiveCompileIvyDeps(), m.transitiveIvyDeps(), m.unmanagedClasspath()) }
}
) {
case (
ev,
state,
id,
m: JavaModule,
(transitiveCompileIvyDeps, transitiveIvyDeps, unmanagedClasspath)
) =>
val ivy = transitiveCompileIvyDeps ++ transitiveIvyDeps
val deps = ivy.map { dep =>
// TODO: add data with "maven" data kind using a ...
// MavenDependencyModule
new DependencyModule(dep.dep.module.repr, dep.dep.version)
}
val unmanged = unmanagedClasspath.map { dep =>
new DependencyModule(s"unmanaged-${dep.path.last}", "")
}
new DependencyModulesItem(id, (deps ++ unmanged).iterator.toSeq.asJava)
} {
new DependencyModulesResult(_)
}

Perhaps I'm misunderstanding the proposed solution but wouldn't it be weird to call m.resolveDeps in this method? That is, shouldn't these dependencies be resolved elsewhere?

@lefou
Copy link
Member

lefou commented Jun 21, 2024

Perhaps I'm misunderstanding the proposed solution but wouldn't it be weird to call m.resolveDeps in this method? That is, shouldn't these dependencies be resolved elsewhere?

@megri There is some resolution needed, that's for sure. Of course we already have a target which resolves all ivy dependencies used for compilation (JavaModule.resolvedIvyDeps), but it is only returning some jars, not the full Maven coordinates, which are clearly needed for the dependencyModules request. So we have multiple options here:

  1. Refactor JavaModule to also return the coordinates for each jar, which isn't trivial since there is no API in coursier to get that easily. You either get some resolved metadata without the jar location or some jar location without the metadata. At least from what I have discovered about the API.

  2. Just run the resolution locally in this BSP request (by doing something like what resolvedIvyDeps does). Since coursier has a cache and is fast, it should not impose much overhead. Otherwise, we have the same limitations regarding the coursier API, so it's probably easier to resolve and assemble the dependency data locally and only refactor after we have found a nice way to assemble it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The issue represents an bug
Projects
None yet
Development

No branches or pull requests

3 participants