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

Settings should use dynamic dispatch semantics #2899

Open
dwijnand opened this issue Jan 9, 2017 · 15 comments
Open

Settings should use dynamic dispatch semantics #2899

dwijnand opened this issue Jan 9, 2017 · 15 comments
Labels

Comments

@dwijnand
Copy link
Member

dwijnand commented Jan 9, 2017

steps

val baseDir = settingKey[File]("")
val srcDir = settingKey[File]("")

inThisBuild(Seq(
  baseDir := file("/"),
  srcDir := baseDir.value / "src"
))

val a = project settings (baseDir := file("/a"))
val b = project settings (baseDir := file("/b"))

problem

> show baseDir
[info] {.}/*:baseDir
[info] 	/
[info] a/*:baseDir
[info] 	/a
[info] b/*:baseDir
[info] 	/b
[success] Total time: 0 s, completed 09-Jan-2017 22:58:41
> show srcDir
[info] {.}/*:srcDir
[info] 	/src
[info] a/*:srcDir
[info] 	/src
[info] b/*:srcDir
[info] 	/src
[success] Total time: 0 s, completed 09-Jan-2017 22:58:50

expectation

In ThisBuild scope, srcDir is defined in terms of baseDir.
Later, in project a and project b, in project scope baseDir is redefined.
Intuitively, imho, a/*:srcDir should be /a/src not /src.

notes

sbt.version=0.13.13

Also, using taskKeys instead of settingKeys changes nothing.

@dwijnand dwijnand added the Bug label Jan 9, 2017
@eed3si9n
Copy link
Member

eed3si9n commented Jan 9, 2017

Your expectation doesn't match mine in terms of inThisBuild(...), inConfig(...), etc..
The RHS of the srcDir := baseDir.value / "src" should be scoped to the scope you said it should be scoped to. You might want some other feature request, but I disagree to categorize this as a bug.

@dwijnand
Copy link
Member Author

That's because you know the implementation details of inThisBuild.

Would you accept my expection if I used:

baseDir in ThisBuild := file("/")
 srcDir in ThisBuild := baseDir.value / "src"

or in Global or any other combination you can come up with?

What I'm trying to define is a default definition for srcDir, defined in terms of baseDir, as baseDir / "src".
Given that universal, project-agnostic definition, I want to refine the value of baseDir and have srcDir respond appropriately.
That is what I believe is the intuitive expectations using sbt's scopes.

With regards to categorization, I've learnt it's very hard to define things (bug, feature requests, workarounds, hacks, api usages, etc) - one man's bug is another man's feature request. I personally feel that this is a bug, one which lots of sbt users and developers have suffered through, typically by throwing things at walls and giving up completely on scopes (ie. using commonSettings over inThisBuild).

@eed3si9n
Copy link
Member

eed3si9n commented Jan 10, 2017

Using the terminology that I defined in http://www.scala-sbt.org/0.13/docs/Basic-Def.html, we can break down srcDir in ThisBuild := baseDir.value / "src" into three parts.

  • key,
  • operator, and
  • setting body

Let's just focus on key first. Key is srcDir in ThisBuild. ThisBuild is not a magical inheritance, it's just a value {url}in subproject axis. There's only one setting named that, and therefore it can take only one meaning. We're in agreement till here right?

You wrote:

I want to refine the value of baseDir and have srcDir respond appropriately.

That's where things get weird. We can't have one key that can "respond".
If you want to put dependent key in a subproject, and the subproject-scoped srcDir to change, you would have to put them in the subproject (project settings) up front.
Auto plugins can inject project settings across the board, and you can even depend on other auto plugins too.

@dwijnand
Copy link
Member Author

When keys not defined in project-scope fall back to ThisBuild, then it is essentially inheritance. Only instead of being physical Scala code inheritance, it's inheritance done as values.

To put it back into the code it's emulating:

import java.io.File

trait ThisBuildSettings {
  def baseDir: File = new File("/")
  def srcDir: File = new File(baseDir, "src")
}

object ProjectA extends ThisBuildSettings {
  override def baseDir: File = new File("/a")
}

object Main {
  def main(args: Array[String]): Unit = {
    println(s"ProjectA srcDir: ${ProjectA.srcDir}")
  }
}
$ scala T.scala
ProjectA srcDir: /a/src

I know and understand what the workarounds to the bug are.

@dwijnand dwijnand self-assigned this Sep 19, 2017
@dwijnand dwijnand added this to the 1.1.0 milestone Sep 19, 2017
@eed3si9n eed3si9n added Enhancement and removed Bug labels Sep 27, 2017
@eed3si9n eed3si9n modified the milestones: 1.1.0, 1.something Oct 15, 2017
@dwijnand dwijnand modified the milestones: 1.something, 1.2.0 Dec 13, 2017
@dwijnand dwijnand changed the title Key dependencies across scopes is unintuitive Settings should use "invokevirtual" semantics Dec 13, 2017
@dwijnand dwijnand modified the milestones: 1.2.0, 1.something Jan 9, 2018
@fommil
Copy link
Contributor

fommil commented Jan 18, 2018

related to #2534

@dwijnand
Copy link
Member Author

Some details of derive, which is a workaround for this issue: https://gist.github.com/eed3si9n/bf561bce36ca72b5f659

@dwijnand
Copy link
Member Author

dwijnand commented Jul 3, 2019

Here's an example for the task axis, from the sbt/sbt gitter room:

if scalaVersion has a different value for scope run, shouldn't it be used when using the run command (as opposed to compile) for example

@eatkins
Copy link
Contributor

eatkins commented Jul 3, 2019

I don’t really understand the use case. Shouldn’t a task do the same thing no matter how it is evaluated? If there is a different scalaVersion for compile and run, are we supposed to recompile the project when we invoke run after we’ve previously invoked compile? Adding some kind of dynamic dispatch where the current command sets the scope seems likely to create even more confusion about how a task or setting is defined.

@dwijnand
Copy link
Member Author

dwijnand commented Jul 3, 2019

It's a different evaluation model than it is currently, but I think it's viable, yes.

Modelled in Scala:

trait HasCompile {
  def sources: Seq[File]
  def scalaVersion: String = "2.12.8"
  def compile(): Seq[File] = Zinc.compile(sources, scalaVersion)
}

trait HasRun extends HasCompile {
  override def scalaVersion: String = "2.13.0" // user override for `run`
  def run(): Unit = {
    val classfiles = compile()
    Zinc.run(classfiles)
  }
}

@eatkins
Copy link
Contributor

eatkins commented Jul 3, 2019

I agree with the initial issue that it would be an improvement if settings were bound late so that you'd get /a/src instead of /src in the a project.

My interpretation of the gitter discussion is that the user expects settings set at the task level to propagate to the task's delegates. I don't think that makes sense. Extending your HasCompile example, it would like if we had

trait HasScalaVersion {
  def scalaVersion: String
}
trait HasCompile with HasScalaVersion {
  override def scalaVersion: String = "2.12.8"
  def sources: Seq[File]
  def compile(): Seq[File] = Zinc.compile(sources, scalaVersion)
}

trait HasRun {
  def run(): Unit
}
class Runner(val compiler: HasCompile) extends HasRun with HasScalaVersion {
  override def scalaVersion: String = "2.13.0"
  override def run(): Unit = Zinc.run(compiler.compile())
}

and we expected Runner to generate 2.13.0 paths.

@eatkins
Copy link
Contributor

eatkins commented Jul 3, 2019

Delegation and inheritance have different semantics and expectations. I think there is a fair argument to be made that sbt would be easier for many scala users if it used inheritance. Martin Odersky has more or less made that argument: https://docs.google.com/document/d/1QdtRJGxlKTiXcAxsjXWLtWVzeZyUzGVDh8oOYOiuhwg/edit.

@dwijnand
Copy link
Member Author

dwijnand commented Oct 3, 2019

Doesn't my example use inheritance and yours use delegation?

Anyways I'm less sure about task-level delegation than build/project level delegation.

@dwijnand
Copy link
Member Author

#5202 another case.

@OlegYch
Copy link
Contributor

OlegYch commented Feb 25, 2020

is this still an issue if ThisBuild doesn't exist? (i think it shouldn't)

@dwijnand
Copy link
Member Author

Yes, I think so. Even at default definition level (i.e. Global) and project level it would be nice to redefine a setting and have that propagate to default definitions:

// default definitions from sbt
Global / scalaVersion := "2.13.1"
Global / crossScalaVersions := Seq(scalaVersion)

// in your project
val foo = project.settings(
  scalaVersion := "2.12.10" // this should propate to crossScalaVersions
)

(Btw, that example works today due to "derived" settings, which is the hackaround to this issue, and problematic in its own way.)

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

No branches or pull requests

5 participants