lightbend-labs / dbuild

Multi-project build tool, based on sbt.
https://lightbend-labs.github.io/dbuild
Other
83 stars 14 forks source link

Wip issues mix2 58 83 84 #85

Closed cunei closed 10 years ago

cunei commented 10 years ago

This commit:

jsuereth commented 10 years ago

In the future, let's try to split the pull request into pieces:

Primarily, the "cleanup" PR and the "build a single target project" feel like they should have been separate, at least for easier reviewing. I think I'm done reviewing the PR stuff (just concerns about multi-process/multi-thread issues on the filesystem). Otherwise that part LGTM.

Now for 09909fb

jsuereth commented 10 years ago

Ok, so a few comments on the build-single-project PR, mostly from an architecture standpoint.

cunei commented 10 years ago

About https://github.com/typesafehub/distributed-build/pull/85#issuecomment-29715980 : The fixes evolved in parallel, and the modifications for the four parts rely on one another, which is why there is a single PR for all four sections. I tried to separate the four parts by creating intermediate branches, and preserving the merge comments (despite the fast forward). For easier reviewing, you can see the four sections independently by using GitHub's diffs: https://github.com/cunei/distributed-build/compare/typesafehub:9f3e15651fb50bca9825856a6398f1f2243cea33...077569b09bd3e63128a84340ed6fcc0ac455fc1f https://github.com/cunei/distributed-build/compare/typesafehub:077569b09bd3e63128a84340ed6fcc0ac455fc1f...006eaefffa737d574960b850d77b33594eafec24 https://github.com/cunei/distributed-build/compare/typesafehub:006eaefffa737d574960b850d77b33594eafec24...d74f5a5d681619107a30aa2804387fea5df2d977 https://github.com/cunei/distributed-build/compare/typesafehub:d74f5a5d681619107a30aa2804387fea5df2d977...dd9fb137bd34582dc844695b004dd178759efdda

cunei commented 10 years ago

The runBuild() in SimpleBuildActor is actually just a subroutine called from within SimpleBuildActor itself, it is not an external interface. That may have been the cause of the confusion. In any case, separating the graph filtering in a different method makes sense; I submitted a separate PR (#88).