Forced version in some cross operations

[ignasi35]

(I think this is a regression introduced in some of the changes in Cross in version 1.3.5and forward)

steps

Given a project with 3 modules:

• a has crossScalaVersions := Seq("2.12.10") and scalaVersion := "2.12.10"
• b has crossScalaVersions := Seq("2.12.10", "2.13.1") and scalaVersion := "2.12.10"
• c has crossScalaVersions := Seq("2.12.10") and scalaVersion := "2.12.10". calso depends on a

And a root which aggregates all the modules above and sets crossScalaVersions := Nil and scalaVersion := "2.12.10".

When running +publishLocal both a and c are published for 2.12.10 and 2.13.1.

See akka/akka-grpc#907 for a reproducer. That PR executes +publishLocal as expected when run on sbt 1.3.4 but fails when run on any version after 1.3.5.

problem

Starting sbt 1.3.5 using + will often Force the scala version instead of Setting it. The implication is that some projects that don't define a particular scala version
in the crossScalaVersions are added to the task anyway.

expectation

Given the project above, I would expect the following artifacts only:

• a_2.12.jar
• b_2.12.jar
• b_2.13.jar
• c_2.12.jar

notes

1. The reproducer I linked above seems to have/miss something since I'm not sure I can reproduce this problem in the Lagom 1.6.x branch build. The Lagom 1.6.x branch build has an even more complex cross-version setup since it support not only 2.12 and 2.13 but also sbt 0.13 and 1.0 (which requires some artifacts to be built for scala 2.10.
2. in the reproducer I linked the project c I refer to in my examples above is actually an SbtPlugin (no surprise there). The particularity of the SbtPlugin in akka-grpc is that it depends on another, external SbtPlugin.

[raboof]

I took some time to narrow this down, and have a 'reproducer' at https://github.com/raboof/sbt-reproduce-5497/blob/master/build.sbt#L6 - sbt +compile to reproduce.

Turns out this use of ProjectRef here was essential in reproducing the behavior.

This reproducer shows a scenario that 'worked' up until and including sbt 1.3.4, and no longer after that. It does not really motivate that this should work 😄 . The 1.3.4 behavior happened to be convenient in Akka gRPC, but perhaps the new behavior is actually more reasonable - I haven't looked into that yet.

[ignasi35]

Turns out this use of ProjectRef here was essential in reproducing the behavior.

But the akka-grpc build is not using ProjectRef, is it? There are muliple places where the version is forced and I wonder if your reproducer just raised another bug. 😅

[raboof]

It definitely is, in project/ReflectiveCodeGen.scala - which I'll admit is a bit exotic, here be dragons, you have been warned :D

…

On Thu, 16 Apr 2020, 18:29 Ignasi Marimon-Clos, ***@***.***> wrote: Turns out this use of ProjectRef here was essential in reproducing the behavior. But the akka-grpc build is not using ProjectRef, is it? There are muliple <https://github.com/sbt/sbt/pull/5494/files> places where the version is forced and I wonder if your reproducer just raised another bug. 😅 — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#5497 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABAGEEJ2AX6JVFD3QNTCLLRM4W7VANCNFSM4MCLXQ7Q> .

[eed3si9n]

When Roper brought over my sbt-doge into sbt 1.x in #2613 there were no !.

sbt 0.13 cross building

sbt 0.13, + was implemented as command composition:

https://github.com/sbt/sbt/blob/v0.13.18/main/src/main/scala/sbt/Cross.scala#L106-L115

  lazy val crossBuild = Command.arb(requireSession(crossParser), crossHelp) { (state, command) =>
    val x = Project.extract(state)
    import x._
    val versions = crossVersions(state)
    val current = scalaVersion in currentRef get structure.data map (SwitchCommand + " " + _) toList;
    if (versions.isEmpty) command :: state
    else {
      versions.map(v => s"$SwitchCommand $v $command") ::: current ::: state
    }
  }

So given crossVersions on the current project set to Seq("2.12.11", "2.13.2"), +publishLocal would desugar as:

> ++ 2.12.11 publishLocal
> ++ 2.13.2 publishLocal

Note that in sbt 0.13 ++ worked like sbt 1.x's ++ with !, so we can think of them as:

> ++ 2.12.11! publishLocal
> ++ 2.13.2! publishLocal

And also note that this happened regardless of the individual subprojects' crossVersions settings, and aggregation happened by means of .aggregate(...) mechanism. Compared to sbt-doge that iterates over crossVersions for all subprojects, sbt 0.13 system feels barbaric.

doge bites

However sbt-doge was not without fault. When we moved to the new sbt-doge system, heterogenous cross building improved but it also lost the ability to run commands, aliases, and input tasks, because in sbt-doge I've made an assumption that the cross arguments are tasks. This was reported as #2776.

Roper came to the rescue again in #3446. To increase the backward compatibility of sbt 0.13 (This was still in 2017 so that was important), he came up with dual system in which cross argument would be parsed to see if it was a command or a task:

// Detect whether a task or command has been issued
val allCommands = Parser.parse(aggCommand, Act.aggregatedKeyParser(x)) match {
  case Left(_) =>
    // It's definitely not a task

    .... This is a code path emulating sbt 0.13
    // Execute using a blanket switch
    projCrossVersions.toMap.apply(currentRef).flatMap { version =>
      // Force scala version
      Seq(s"$SwitchCommand $verbose $version!", aggCommand)
    }

  case Right(_) =>
    // We have a key, we're likely to be able to cross build this using the per project behaviour.

    .... This is a code path emulating sbt-doge
}

Because we're emulating sbt 0.13 behavior on Left(_) case, the ! in there I think is acceptable. For example, if someone made a command to run test then publishLocal, it will run that for all aggregated projects.

input task support and !

#3446 added the command support, but what about input tasks like testOnly?
This was still broken in sbt 1.2.8 (#4715).

Ethan contributed #5265 to restore the input task support, which was released as part of 1.3.5. This regressed in parallel task processing so Ethan later sent in #5329 as well. These two PRs together introduced ! to task cross building. I reviewed both PRs and when I noticed !s, I figured they would be ok as long as the build state would be restored at the end. Looks like I was wrong.

commandsByVersion.flatMap {
  case (v, commands) =>
    commands match {
      case Seq(c) => Seq(s"$SwitchCommand $verbose $v! $c")
      case Seq()  => Nil // should be unreachable
      case multi if fullArgs.isEmpty =>
        Seq(s"$SwitchCommand $verbose $v! all ${multi.mkString(" ")}")
      case multi => Seq(s"$SwitchCommand $verbose $v!") ++ multi
    }
}

In all of these cases, I am guessing that ! are in fact unnecessary.

What we should aim for I think is figuring out the exact corner cases for these ! or lack thereof and express them as scripted tests.

[eed3si9n]

Here are some tests I added - #5512