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

Mill doesn't evict older versions of transitive dependencies from moduleDeps #2732

Closed
lolgab opened this issue Sep 11, 2023 · 1 comment · Fixed by #2735
Closed

Mill doesn't evict older versions of transitive dependencies from moduleDeps #2732

lolgab opened this issue Sep 11, 2023 · 1 comment · Fixed by #2735
Labels
bug The issue represents an bug
Milestone

Comments

@lolgab
Copy link
Member

lolgab commented Sep 11, 2023

Reproduction:

// build.sc
import mill._
import mill.scalalib._

object bar extends ScalaModule {
  def scalaVersion = "2.13.12"
  def moduleDeps = Seq(foo)
  def ivyDeps = super.ivyDeps() ++ Agg(
    ivy"org.typelevel::cats-core::2.9.0",
  )
}
object foo extends ScalaModule {
  def scalaVersion = "2.13.12"
  def ivyDeps = super.ivyDeps() ++ Agg(
    ivy"io.circe::circe-core::0.14.0"
  )
}

classpath with Mill 0.11.2 (note the two artifacts of cats-core_2.13):

> mill show bar.compileClasspath
[
  "ref:v0:c984eca8:/Users/lorenzo/scala/repro/foo/compile-resources",
  "qref:v1:19c63377:/Users/lorenzo/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/io/circe/circe-core_2.13/0.14.0/circe-core_2.13-0.14.0.jar",
  "qref:v1:0004e741:/Users/lorenzo/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scala-library/2.13.12/scala-library-2.13.12.jar",
  "qref:v1:03c52e9e:/Users/lorenzo/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/io/circe/circe-numbers_2.13/0.14.0/circe-numbers_2.13-0.14.0.jar",
  "qref:v1:59fac2bc:/Users/lorenzo/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/typelevel/cats-core_2.13/2.6.1/cats-core_2.13-2.6.1.jar",
  "qref:v1:b856e496:/Users/lorenzo/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/typelevel/cats-kernel_2.13/2.6.1/cats-kernel_2.13-2.6.1.jar",
  "qref:v1:cfb647ca:/Users/lorenzo/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/typelevel/simulacrum-scalafix-annotations_2.13/0.5.4/simulacrum-scalafix-annotations_2.13-0.5.4.jar",
  "ref:v0:c984eca8:/Users/lorenzo/scala/repro/out/foo/compile.dest/classes",
  "ref:v0:c984eca8:/Users/lorenzo/scala/repro/bar/compile-resources",
  "qref:v1:f482fc22:/Users/lorenzo/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/typelevel/cats-core_2.13/2.9.0/cats-core_2.13-2.9.0.jar",
  "qref:v1:045a4a25:/Users/lorenzo/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/typelevel/cats-kernel_2.13/2.9.0/cats-kernel_2.13-2.9.0.jar"
]

classpath with Mill 0.10.12 (which has a single artifact of cats-core_2.13):

> mill show bar.compileClasspath
[
  "ref:c984eca8:/Users/lorenzo/scala/repro/foo/resources",
  "ref:c984eca8:/Users/lorenzo/scala/repro/out/foo/compile.dest/classes",
  "ref:c984eca8:/Users/lorenzo/scala/repro/bar/resources",
  "qref:f482fc22:/Users/lorenzo/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/typelevel/cats-core_2.13/2.9.0/cats-core_2.13-2.9.0.jar",
  "qref:0004e741:/Users/lorenzo/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scala-library/2.13.12/scala-library-2.13.12.jar",
  "qref:19c63377:/Users/lorenzo/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/io/circe/circe-core_2.13/0.14.0/circe-core_2.13-0.14.0.jar",
  "qref:045a4a25:/Users/lorenzo/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/typelevel/cats-kernel_2.13/2.9.0/cats-kernel_2.13-2.9.0.jar",
  "qref:03c52e9e:/Users/lorenzo/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/io/circe/circe-numbers_2.13/0.14.0/circe-numbers_2.13-0.14.0.jar"
]
@lefou
Copy link
Member

lefou commented Sep 11, 2023

Looks like an regression, probably introduced in #2425. The culprit seems to be in this code

def transitiveCompileClasspath: T[Agg[PathRef]] = T {
T.traverse(
(moduleDeps ++ compileModuleDeps).flatMap(_.transitiveModuleDeps).distinct
)(m => T.task { m.compileClasspath() ++ Agg(m.compile().classes) })()
.flatten
}

When traversing modules, we should only collect local resources, but not include already resolved ivyDeps, as we want to resolve all transitive ivyDeps later consistently. But the included compileClasspath might already contain resolved ivyDeps. That way, we end up with multiple resolved versions of the same dependency.

@lolgab lolgab added the bug The issue represents an bug label Sep 13, 2023
lihaoyi added a commit that referenced this issue Sep 14, 2023
Fixes #2732

We introduce a new `localCompileClasspath`, which differs from
`compileClasspath` in that it leaves out the
`transitiveCompileClasspath` from upstream modules and `resolvedIvyDeps`
from third party dependencies.

Also tried to do some cleanup of `JavaModule`, refactoring out a
`transitiveModuleCompileModuleDeps` helper to DRY things up,
re-arranging things so all the `fooModuleDeps` helpers are all together.

Tested via a new unit test
`mill.scalalib.HelloWorldTests.multiModuleClasspaths`, which fails on
master and passes on this PR. This test case asserts the overall "shape"
of the classpaths, which should help rule out an entire class of
mis-configuration w.r.t. how the classpaths are wired up
@lefou lefou added this to the 0.11.3 milestone Sep 14, 2023
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
2 participants