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

Build fails when using scala ivydep in scalamodule with java moduledep #860

Closed
nrktkt opened this issue May 6, 2020 · 9 comments · Fixed by #1574
Closed

Build fails when using scala ivydep in scalamodule with java moduledep #860

nrktkt opened this issue May 6, 2020 · 9 comments · Fixed by #1574
Labels
bug The issue represents an bug workaround-available
Milestone

Comments

@nrktkt
Copy link
Contributor

nrktkt commented May 6, 2020

Given

// build.sc
import mill._, scalalib._
object core extends JavaModule {
  object scalaapi extends ScalaModule {
    def scalaVersion = "2.13.1"
    def moduleDeps = Seq(core)
    def ivyDeps = Agg(ivy"org.typelevel::cats-core:2.1.1")
  }
  object javaapi extends JavaModule {
    def moduleDeps = Seq(core, scalaapi)
  }
}

attempting to compile will yield

[42/57] core.scalaapi.compile 
[info] Compiling 6 Scala sources to /home/nfischer/Code/2Auth/out/core/scalaapi/compile/dest/classes ...
[info] Done compiling.
[55/57] core.javaapi.compileClasspath 
1 targets failed
core.javaapi.compileClasspath java.lang.AssertionError: assertion failed: Not a Java dependency: Dep(Dependency(org.typelevel:cats-core, 2.1.1, Configuration(default(compile)), Set(), Publication(, Type(), Extension(), Classifier()), false, true),Binary(false),false)
    scala.Predef$.assert(Predef.scala:223)
    mill.scalalib.Lib$.depToDependencyJava(Lib.scala:24)
    mill.scalalib.CoursierModule.$anonfun$resolveCoursierDependency$3(CoursierModule.scala:17)
    mill.scalalib.CoursierModule.$anonfun$resolveDeps$2(CoursierModule.scala:30)
    scala.collection.immutable.Stream.map(Stream.scala:418)
    mill.scalalib.Lib$.resolveDependencies(Lib.scala:65)
    mill.scalalib.CoursierModule.$anonfun$resolveDeps$1(CoursierModule.scala:27)
    mill.define.ApplyerGenerated.$anonfun$zipMap$2(ApplicativeGenerated.scala:7)
    mill.define.Task$MappedDest.evaluate(Task.scala:380)

on 0.6.2

@lefou
Copy link
Member

lefou commented May 6, 2020

On a first glance, it looks like when resolving the ivy deps of javaapi we also resolve all transitive deps from other modules, which include also scala dependencies. But in the scope of the JavaModule we no longer know the scala version to use.

@lefou
Copy link
Member

lefou commented May 6, 2020

2 workaround come to my mind:

  • make the javaapi project a ScalaModule and adapt the artifactSuffix to be empty
  • write the dependency as pure java dependency: ivy"org.typelevel:cats-scalaBinVersion}:2.1.1"

@nrktkt
Copy link
Contributor Author

nrktkt commented May 6, 2020

as suggested by @lefou a workaround is to make javaapi a ScalaModule and def artifactSuffix = ""

@lefou lefou added bug The issue represents an bug workaround-available labels May 6, 2020
@lefou
Copy link
Member

lefou commented May 6, 2020

We could transform ivy-deps of other modules to be just "java" dependencies without any cross-version semantic. This can of course lead to issues too.

Thinking a bit more about this issue, I think we need some more safe-guarding when using transitive dependencies of moduleDeps, e.g. in case we depend on another module which uses a different Scala version.

@lefou
Copy link
Member

lefou commented Nov 20, 2020

I currently hit this issue again when working on a polyglot project, which has Java, Java + AspectJ, Scala and Kotlin modules. But I think this issue will hit us more frequently now, when people start the try out the new Scala 2.13 - Scala 3 forward compatibility feature.

I think the fact, that we bind the cross value for transitive dependencies in the dependent module is wrong. Instead, each modules should resolve the cross- and platform placeholder by itself. This has the downside, that it is possible to bring more that one Scala-binary-version-line into the dependency tree. We probably should invent some other mechanism to check for consistency, or maybe we can offload that to coursier (I don't know how)?

@lefou
Copy link
Member

lefou commented Nov 20, 2020

I think each module should implement some checks, e.g. a ScalaModule should check that each transitively module dependency has a compatible or event the same scala version selected. Other modules, like ScalaJsModule need to also check for the same selected platform.

@lolgab
Copy link
Member

lolgab commented Nov 20, 2021

We could transform ivy-deps of other modules to be just "java" dependencies without any cross-version semantic. This can of course lead to issues too.

Thinking a bit more about this issue, I think we need some more safe-guarding when using transitive dependencies of moduleDeps, e.g. in case we depend on another module which uses a different Scala version.

I'm facing probably another instance of this issue in Ammonite, where Scala 3 dependencies are converted into Scala 2.13 dependencies when we try to make a Scala 2.13 module depend on a Scala 3 module.
It seems like the suffix of the dependencies is evaluated according to the depending module and not according to the module that declares them.

For example, given this project:

import mill._
import mill.scalalib._

object scala213 extends ScalaModule {
  def scalaVersion = "2.13.6"
  def moduleDeps = Seq(scala3)
  def scalacOptions = Seq("-Ytasty-reader")
}
object scala3 extends ScalaModule {
  def scalaVersion = "3.0.2"
  def ivyDeps = Agg(
    ivy"com.lihaoyi::upickle:1.4.0"
  )
}

the ivyDepsTree is:

./mill --disable-ticker scala213.ivyDepsTree
└─ com.lihaoyi:upickle_2.13:1.4.0
   ├─ com.lihaoyi:ujson_2.13:1.4.0
   │  └─ com.lihaoyi:upickle-core_2.13:1.4.0
   │     └─ com.lihaoyi:geny_2.13:0.6.10
   ├─ com.lihaoyi:upack_2.13:1.4.0
   │  └─ com.lihaoyi:upickle-core_2.13:1.4.0
   │     └─ com.lihaoyi:geny_2.13:0.6.10
   └─ com.lihaoyi:upickle-implicits_2.13:1.4.0
      └─ com.lihaoyi:upickle-core_2.13:1.4.0
         └─ com.lihaoyi:geny_2.13:0.6.10

While in Sbt the dependencies are resolved using the _3 suffix (as it should be IMO):

lazy val scala213 = project
  .settings(
    scalaVersion := "2.13.6",
    scalacOptions += "-Ytasty-reader"
  )
  .dependsOn(scala3)

lazy val scala3 = project
  .settings(
    scalaVersion := "3.0.2",
    libraryDependencies += "com.lihaoyi" %% "upickle" % "1.4.0"
  )
sbt:scala3-sandwitch> scala213/dependencyTree
[info] scala213:scala213_2.13:0.1.0-SNAPSHOT [S]
[info]   +-scala3:scala3_3:0.1.0-SNAPSHOT
[info]     +-com.lihaoyi:upickle_3:1.4.0
[info]     | +-com.lihaoyi:ujson_3:1.4.0
[info]     | | +-com.lihaoyi:upickle-core_3:1.4.0
[info]     | |   +-com.lihaoyi:geny_3:0.6.10
[info]     | |   
[info]     | +-com.lihaoyi:upack_3:1.4.0
[info]     | | +-com.lihaoyi:upickle-core_3:1.4.0
[info]     | |   +-com.lihaoyi:geny_3:0.6.10
[info]     | |   
[info]     | +-com.lihaoyi:upickle-implicits_3:1.4.0
[info]     |   +-com.lihaoyi:upickle-core_3:1.4.0
[info]     |     +-com.lihaoyi:geny_3:0.6.10
[info]     |     
[info]     +-org.scala-lang:scala3-library_3:3.0.2 [S]
[info]     

lolgab added a commit to lolgab/Ammonite that referenced this issue Nov 20, 2021
As stated [here](com-lihaoyi/mill#860 (comment))
Mill doesn't handle correctly the Scala 2.13 modules depending on Scala
3 modules. This happens because dependencies are resolved at using
the suffix from the leaf module instead of every module resolving its dependencies
on its own. So if a module compiled with Scala 3 depends on libraries
with the `_3` suffix, once another module using Scala 2.13 depends on it
all the dependencies get transformed to use the `_2.13` suffix.
Sbt maintains instead the original suffixes.
This commit introduces a preprocessing on the dependencies to convert
them to manually add the `_3` suffix
@lefou
Copy link
Member

lefou commented Nov 20, 2021

I haven't looked at the code right now, but we have a defined way to translate each mill-dependency to a coursier-dependency. I think it's resolveCoursierDependency. It's probably worth a POC to use the resolveCoursierDependency task of that module which also contributed the dependency. (Currently we always use that module which resolves the final dependency tree.) The result may contain potential conflicting dependencies, but in such a case, it probably needs intervention for the user anyways. We should try to make some minimal sanity checks, e.g. to not have multiple scala versions etc.

@lefou
Copy link
Member

lefou commented Dec 15, 2022

I now have a fully working PR, which fixes this issue.

It comes with a slight API change. I'd welcome any review or feedback on it. If there are no objections, I'm going to merge it in the next days.

lefou added a commit that referenced this issue Dec 16, 2022
`mill.scalalib.Dep` represents a generic dependency, which include
potential cross-compilation properties. E.g. it knows whether it is
specific to a `platform` but it does not specify the platform. And it
knows whether it is specific to a Scala binary or full version, but it
does not specify the exact Scala version. This mean we can define a
`Dep` without knowing those context.

In this PR I introduce a new type `mill.scalalib.BoundDep`, which is
kind-of a `Dep` where all these potential open context bindings are
bound to an exact context. There is no further cross-resolution
necessary nor possible. By having a separate type, we always know with
what kind of dependency we work. Also, we can be explicit about the
"when" the binding happens.

When mixing dependencies of multiple modules, we also mix contexts.
Until this PR, we had no way to get it right, as `Dep`s could come from
different contexts. Now, we can safely mix `BoundDep`s as they are
already context-bound.

We could encode the bound-state in the `Dep`, but this would result in
only a dynamic property. To ensure a resolved state, we always would
need to run some runtime checks. Also, it could lead to situations,
where we need a resolved dep, but have not the proper context to do the
resolution. This is exactly what I implemented in the initial attempt,
but it didn't worked out very well. With the separated types, it is
always clear from the API, what kind of dependency we expect.

We could also just resolve to a `coursier.Dependency`, but that would
loose the `force` flag. Also, we want to rather decouple our API from
the coursier API, which seem easier when we avoid to hand over direct
coursier API to the user.

I `@deprecated` the existing `CoursierModule.resolveCoursierDependency`
and bases the new `CoursierModule.bindDependency` on this old task. This
should ease the transition. It's up for revisit, when we decouple from
coursier API altogether.

This fixes 
#860.
@lefou lefou added this to the 0.11.0-M1 milestone Dec 16, 2022
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 workaround-available
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants