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

Introduce CompositeProject #4056

Merged
merged 7 commits into from Apr 9, 2018

Conversation

Projects
None yet
5 participants
@ghost

ghost commented Apr 2, 2018

fixes: #3042

/CC the sbt-crossproject "crew" - @sjrd @densh @MasseGuillaume

@eed3si9n

This comment has been minimized.

It might be good to add:

lazy val b = (project in file("b"))
  .settings(
    version := "0.2.0",
  )

and test that it shadows p2 for backward compatibility.

@lightbend-cla-validator

This comment has been minimized.

lightbend-cla-validator commented Apr 2, 2018

Hi @BennyHill,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

http://www.lightbend.com/contribute/cla

@eed3si9n eed3si9n added the ready label Apr 2, 2018

@ghost

This comment has been minimized.

ghost commented Apr 2, 2018

Just did the cla....please shout if I found a way to mess it up

@sjrd

This comment has been minimized.

I'm pretty sure this will not (reliably) implement the "shadowing for backward compat" aspect described in the issue. Unfortunately, we'll need something a bit more complicated, like:

def projects: Seq[Project] = {
  val compositeProjects = ReflectUtilities.allVals[CompositeProject](this).values.toSeq
  /* normally we would simply do
   *   compositeProjects.flatMap(_.componentProjects)
   * here, but we need to make sure that, if two projects with the same id appear in
   * `compositeProjects` and in `compositeProjects.map(_.componentProjects)`, the one in
   * `compositeProjects` wins. This is necessary for backward compatibility with the idiom:
   *   lazy val foo = crossProject
   *   lazy val fooJVM = foo.jvm.settings(...) // this one needs to win
   */
  for (p <- compositeProjects.flatMap(_.componentProjects)) yield {
    compositeProjects.find(_.id == p.id) match {
      case Some(overridingProject) => overridingProject
      case None                    => p
    }
  }
}
@sjrd

This comment has been minimized.

Probably we need a similar dance here as well.

@ghost

This comment has been minimized.

ghost commented Apr 2, 2018

@sjrd Thanks for feedback...tbh, I PR'd early for such feedback.

I'll add you changes 👍

@ghost

This comment has been minimized.

ghost commented Apr 2, 2018

Also, whilst I modified BuildDef, I'm not entirely certain that this is in fact (still) used. I'll continue to update both....

@dwijnand dwijnand changed the title from Fix/3042 to Introduce CompositeProject Apr 3, 2018

@@ -119,7 +119,13 @@ sealed trait ProjectDefinition[PR <: ProjectReference] {
if (ts.isEmpty) Nil else s"$label: $ts" :: Nil
}
sealed trait Project extends ProjectDefinition[ProjectReference] {
trait CompositeProject {

This comment has been minimized.

@dwijnand

dwijnand Apr 3, 2018

Member

could you seal this please?

This comment has been minimized.

@ghost

ghost Apr 3, 2018

In the implementation of, say, CrossProject, we would want to extend this. In the sbt-test, I just create an instance, but in reality, I think we would want to extend.

Of course, I may have got the use wrong. But my impression was that by doing this, its then possible to have a CrossProject as a „real“ Project

This comment has been minimized.

@ghost

ghost Apr 3, 2018

Hm... maybe I‘m wrong, sigh. OK 👍

This comment has been minimized.

@sjrd

sjrd Apr 3, 2018

Er ... @BennyHill is right, here. If it's sealed, this thing is completely useless ;) The whole point of this thing is that we can indeed have CrossProject implement that trait (which is a pure Java interface, btw). Otherwise, how do you propose we leverage this in sbt-crossproject?

This comment has been minimized.

@ghost

ghost Apr 3, 2018

OK.... How about that when I do the other changes, I update the tests to mimick invisaged usage. That way, it^s clear-cut

This comment has been minimized.

@sjrd

sjrd Apr 4, 2018

what if we provide a ProjectsList subclass?

What exactly does that buy you, if that one is open, compared to having the trait open? The only thing I see is that you would be able to add a concrete field in a binary compatible way, but ... that would be weird anyway.

For things that build on top of it, it's a limitation, as they cannot extend ProjectsList and another class, but they could extend CompositeProject and a class, because CompositeProject is an interface.

This comment has been minimized.

@dwijnand

dwijnand Apr 4, 2018

Member

What exactly does that buy you, if that one is open

it wouldn't be open final class ProjectsList that extends CompositeProject I was thinking.

This comment has been minimized.

@sjrd

sjrd Apr 4, 2018

Then I don't see how I can use it in CrossProject either, since I need crossProject to return something that is both a CompositeProject (for sbt to pick it up as such) and have methods that are specific to CrossProjects (so that I can do .dependsOn(foo.jvm) in some other JVM project, at least. If my only options to have a CompositeProject are Project which is sealed and ProjectsList which is final, it's no good.

Basically, think about how I can declare CrossProject so that the following typechecks and is identified as a CompositeProject:

lazy val foo = crossProject
  .settings(...)
  .jvmSettings(...)
  .jsSettings(...)

lazy val bar = project
  .settings(...)
  .dependsOn(foo.jvm)

This comment has been minimized.

@sjrd

sjrd Apr 4, 2018

With an open CompositeProject, it's super easy. I just alter the declaration of CrossProject with extends CompositeProject and

def componentProjects: Seq[Project] = projects.values.toSeq

This comment has been minimized.

@dwijnand

dwijnand Apr 4, 2018

Member

got it. thanks for detailing the use case.

@dwijnand dwijnand changed the base branch from 1.1.x to 1.x Apr 3, 2018

@dwijnand

This comment has been minimized.

Member

dwijnand commented Apr 3, 2018

I've retargeted this to be merged into 1.x because it's a bit more than just a straight bug fix.

however that's caused merge conflicts with unrelated changes already in 1.x - sorry!

@ghost

This comment has been minimized.

ghost commented Apr 3, 2018

Heh, I know - I went to change it before the PR....but saw the changes and as I was unsure of the right branch to use, just submitted. It was 50/50, never mind, really np 😄

Alistair-Johnson added some commits Apr 4, 2018

Merge branch '1.x' of https://github.com/BennyHill/sbt into fix/3042
Conflicts:
	main/src/main/scala/sbt/internal/BuildDef.scala
	main/src/main/scala/sbt/internal/EvaluateConfigurations.scala
@ghost

This comment has been minimized.

ghost commented Apr 4, 2018

just fyi, one more commit to come with a mock CrossProject extends CompositeProject ....

@ghost

This comment has been minimized.

ghost commented Apr 4, 2018

Actually, is this OK as is?

check := {
val verJvm = (version in foo.jvm).?.value
assert (verJvm == Some("0.2.0"))

This comment has been minimized.

@eed3si9n

eed3si9n Apr 5, 2018

Member

Nice

@eed3si9n

overall LGTM
could you add a release note in https://github.com/sbt/sbt/tree/1.x/notes/1.2.0 plz?

@sjrd

I have a series of comments, but basically all related to the same root cause: I think the refactoring you applied with uniqueId gives the wrong answer.

def componentProjects: Seq[Project]
}
object CompositeProject {

This comment has been minimized.

@sjrd

sjrd Apr 5, 2018

This companion object can probably be private[sbt]. Users of sbt don't need to know about uniqueId.

}
CompositeProject.uniqueId(compositeProjects)

This comment has been minimized.

@sjrd

sjrd Apr 5, 2018

If you first flatMap, then call uniqueId, you have already lost the distinction between things that were in the original list versus those added via the componentProjects. That can't properly work: they will all be kept.

This comment has been minimized.

@sjrd

sjrd Apr 5, 2018

It should be possible to first call CompositeProject.expand(definitions.values(loader)), then map the result with .map(resolveBase(file.getParentFile, _)).

def projects: Seq[Project] = ReflectUtilities.allVals[Project](this).values.toSeq
def projects: Seq[Project] =
CompositeProject.uniqueId(
ReflectUtilities.allVals[CompositeProject](this).values.toSeq.flatMap(_.componentProjects))

This comment has been minimized.

@sjrd

sjrd Apr 5, 2018

Same problem that I mentioned in the other comment, obviously.

* lazy val foo = crossProject
* lazy val fooJVM = foo.jvm.setting
*/
def uniqueId(compositeProjects: Seq[Project]): Seq[Project] = {

This comment has been minimized.

@sjrd

sjrd Apr 5, 2018

Basically, this method should take the original Seq[CompositeProject], not a Seq[Project]. If it takes the Seq[Project], it's too late: you don't know what were the projects that were in the initial compositeProjects versus those that were added through componentProjects.

This comment has been minimized.

@sjrd

sjrd Apr 5, 2018

Hence, I would recommend calling this method expand, not uniqueId, since its job would be to expand the list of CompositeProject into the appropriate list of Projects.

def componentProjects: Seq[Project] = Seq(jvm, js)
}
lazy val fooJVM = foo.jvm.settings(version := "0.2.0") // this one needs to win

This comment has been minimized.

@sjrd

sjrd Apr 5, 2018

This is very good.

To make it even better, you can also declare the lazy val fooJS, but above the lazy val foo (they're lazy vals, so it will work). But that should ensure that the shadowing treatment does not accidentally work via the ordering (which I suspect is currently the case).

This comment has been minimized.

@ghost

ghost Apr 6, 2018

(which I suspect is currently the case).

Indeed, adding a lazy val fooJS first does fail...

@eed3si9n

Yea. @sjrd is right about the uniqueId.

@ghost

This comment has been minimized.

ghost commented Apr 5, 2018

Thanks @sjrd for the thorough review, feedback and suggestions. I will do these tomorrow with fresh eyes, along with the release notes

@sjrd

sjrd approved these changes Apr 7, 2018

:)

* lazy val fooJVM = foo.jvm.settings(...)
* }}}
*/
def expand(projects: Seq[Project], compositeProjects: Seq[CompositeProject]): Seq[Project] = {

This comment has been minimized.

@sjrd

sjrd Apr 7, 2018

You can compute projects inside expand from compositeProjects once and for all, since all projects are also composite projects, so that call sites don't need to know about this:

val projects = compositeProjects.collect {
  case project: Project => project
}

No need to pass it as an additional parameter when expand can compute it itself, especially since the call sites are basically collect ing as well to give it to expand anyway.

This comment has been minimized.

@ghost

ghost Apr 7, 2018

Yep, so I've removed it - thx 👍

@dwijnand dwijnand added this to the 1.2.0 milestone Apr 9, 2018

uniqueId concern addressed

@dwijnand dwijnand merged commit 4fc45e0 into sbt:1.x Apr 9, 2018

3 checks passed

Codacy/PR Quality Review Good work! A positive pull request.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dwijnand dwijnand removed the ready label Apr 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment