Refactor the build #428

jvican · 2017-10-12T09:54:54Z

The build is now structure in the following way:

build.sbt keeps the project definitions.
BuildPlugin.scala keeps the logic.

This way, at a glance, contributors can see which modules the build defines.

This PR does not change the logic of the build. I'll open a new PR after this
one that will improve the scripted test setup, and add sbt-release-early via
a new version of sbt-houserules.

I hope this change makes the build more readable and easier to hack in the future.

dwijnand · 2017-10-12T10:46:59Z

project/Header.scala

+    SbtHeaderKeys.headerMappings ++= Map(
+      HeaderFileType.scala -> HeaderCommentStyle.CStyleBlockComment,
+      HeaderFileType.java -> HeaderCommentStyle.CStyleBlockComment
+    ),


This isn't necessary. It's the default definition: https://github.com/sbt/sbt-header/blob/v3.0.2/src/main/scala/de/heikoseeberger/sbtheader/HeaderPlugin.scala#L131-L134

dwijnand · 2017-10-12T10:48:14Z

project/Header.scala

-// object CustomHeaderPlugin extends AutoPlugin {
-//   override def requires = plugins.JvmPlugin && HeaderPlugin
-//   override def trigger = allRequirements
+object CustomHeaderPlugin extends AutoPlugin {


Could you add headerCheck and test:headerCheck enforcement in CI please?

dwijnand · 2017-10-12T10:49:46Z

project/BuildPlugin.scala

+
+  override def projectSettings: Seq[Def.Setting[_]] = BuildImplementation.projectSettings
+  override def buildSettings: Seq[Def.Setting[_]] = BuildImplementation.buildSettings
+  override def globalSettings: Seq[Def.Setting[_]] = BuildImplementation.globalSettings


Why the indirection?

dwijnand · 2017-10-12T10:49:58Z

project/BuildPlugin.scala

+object BuildPlugin extends AutoPlugin {
+  override def requires = sbt.plugins.JvmPlugin
+  override def trigger = allRequirements
+  val autoImport = BuildKeys


Why the indirection?

A matter of taste, I guess. I find that having BuildKeys being an object outside of the AutoPlugin definition helps readability. Less nesting and more clarity. I also want to define keys independently of the logic of the build (their implementation).

dwijnand · 2017-10-12T10:53:43Z

project/BuildPlugin.scala

+  // Ids that we will use to name our projects in build.sbt
+  val CompilerInterfaceId = "compiler-interface"
+  val CompilerBridgeId = "compiler-bridge"
+  val ZincApiInfoId = "zinc-apiinfo"


These aren't ids, they're the name of the directories the projects are defined in, and will I expect will have broken the custom commands.

Yup. This would have worked pre-1.0.

dwijnand · 2017-10-12T11:25:57Z

project/BuildPlugin.scala

+  override def globalSettings: Seq[Def.Setting[_]] = BuildImplementation.globalSettings
+}
+
+object BuildKeys {


This name isn't good because it contains a bunch of things that aren't keys.

dwijnand · 2017-10-12T11:26:48Z

project/BuildPlugin.scala

+      Command.command("runBenchmarks")(st => runPreSetup :: runBenchmark :: tearDownResources :: st)
+    }
+
+    val all: List[Command] = List(crossTestBridges, publishBridgesAndSet, publishBridgesAndTest)


all not containing all is confusing.

dwijnand · 2017-10-12T11:27:23Z

build.sbt


 // defines Java structures used across Scala versions, such as the API structures and relationships extracted by
 //   the analysis compiler phases and passed back to sbt.  The API structures are defined in a simple
 //   format from which Java sources are generated by the sbt-contraband plugin.
-lazy val compilerInterface = (project in internalPath / "compiler-interface")
+lazy val compilerInterface = (project in file(CompilerInterfaceId))


dwijnand · 2017-10-12T11:30:17Z

project/BuildPlugin.scala

+  val benchmarksTestDir = sbt.IO.createTemporaryDirectory
+
+  val sourcesForAllScalaVersionsSetting: Seq[Def.Setting[_]] =
+    List(Keys.unmanagedSourceDirectories ++= BuildDefaults.handleScalaSpecificSources.value)


Why the indirection? We already have a brand-new and obscure "sourcesForAllScalaVersionsSetting", why not define here what it does instead of deferring to BuildDefaults.handleScalaSpecificSources and then depending on build.sbt to make sure to call inCompileAndTest?

Yup, we can move unmanagedSourceDirectories to the build.sbt.

dwijnand · 2017-10-12T11:46:43Z

project/BuildPlugin.scala

+    ScalafmtKeys.scalafmtOnCompile := true,
+    ScalafmtKeys.scalafmtVersion := "1.2.0",
+    ScalafmtKeys.scalafmtOnCompile in Sbt := false,
+    Keys.description := "Incremental compiler of Scala",


Keys are in a single shared namespace, which is why you can refer to them by name (e.g TaskKey[Unit]("publishSigned") := (())). So we should just import them instead of referring to them like this. Particularly when you end up with this repetition:

GitKeys.gitUncommittedChanges := BuildDefaults.gitUncommitedChanges.value, BintrayKeys.bintrayPackage := "zinc", ScalafmtKeys.scalafmtOnCompile := true,

Could you revert back please?

Keys are in a single shared namespace,

Which conflicts with other things in scope because they have too many generic names, like moduleID et al. I don't want to accidentally be referring to a local variable named this way because it's a source of bugs.

The good things of having everything accessible by Keys are:

It's clear that the keys I'm defining come from sbt (which is really clear for us if we don't use Keys, but not for people reading this build and unfamiliar with some less popular keys).

I can do val moduleID = Keys.moduleID.value. Otherwise I would have to invent a similar name which doesn't clash with moduleID. I could choose between id (id of what? project? module?), module (which is not really true, it's an id of a module) or another variation, but this is weird and I don't want to spend time thinking how to name local variables that are straightforward to identify.

Does this make sense?

I don't want to accidentally be referring to a local variable named this way because it's a source of bugs.

Could you provide an example?

It's clear that the keys I'm defining come from sbt

I don't think that's a strong enough reason to diverge in style.

I can do val moduleID = Keys.moduleID.value

You can still do that with import Keys._.

Another reason not to go with this new style is that when we have slash syntax the build code and shell notation will be unified, so we'll want to use in both cases:

Compile / javacOptions

not

Compile / Keys.javacOptions

You can still do that with import Keys._.

No, you cannot. If you do import Keys._, moduleID will be in scope, so if I define val moduleID = moduleID.value Scala will think it's a recursive definition and will fail both at compile-time and runtime (in case you ever make it to typecheck).

Could you provide an example?

Say I define a local method called publish that defines how a thing is going to be published. If I've done import Keys._, publish will resolve to different terms depending on the scope I use them, which is misleading and a source of bugs. I don't want my code to change its meaning depending on the place I use my names. Not only it's not refactor friendly, it's confusing and dangerous.

The problem gets worse when you don't even know that you're importing a term from Keys.scala that is conflicting with a term you've defined. What about artifact, doc, mainClass, run and the other bazillion of names that are really likely to appear in my build logic and conflict with sbt keys definitions?

Another reason not to go with this new style is that when we have slash syntax the build code and shell notation will be unified

That's a matter of style. I don't mind the second version. My arguments here have to do with semantics and user readability (in terms of not having name clashes -- when users read they don't need to think if a term is defined at the end of the file, in an enclosing scope or comes from sbt -- they have a clear idea that Keys.artifact means the setting key from sbt).

Inside of an AutoPlugin definition it is customary to import Keys._. I tend to import any autoImports from plugins that I works on to emulate what one would have in build.sbt.

Yes, it is customary @eed3si9n. But I believe that, based on my technical explanations before, this particular implementation is superior than the "customary practice". I don't see any technical benefit on making the build plugin implementation be more similar to build.sbt. Perhaps you can elaborate on that?

You both asked me to remove this. I did, but in the other PR because rebasing otherwise was hell. #429

I think this is good to go.

Dale's been looking at this PR more than me, so I'll defer my vote to him.
@dwijnand Is this PR good to go as is? if not could you make a list of pending items?

jvican · 2017-10-12T12:11:16Z

@dwijnand Feedback addressed. I invite you to have another look. Before merge, I'll fixup the last commits together to make sure everything passes CI.

jvican · 2017-10-12T12:36:46Z

CONTRIBUTING.md

@@ -31,8 +31,7 @@ Once you understand the basics of incremental compilation, start having a look
 at open tickets you can help with. All issues are labelled and will give you an
 idea about its difficulty and scope.

-Hacking on Zinc should not seem like a difficult task. Zinc does not implement
-a compiler, it defines the logic to analyse dependencies based on the compiler
+Hacking on Zinc should not seem like a difficult task. Zinc does not implement a compiler, it defines the logic to analyse dependencies based on the compiler


This change is not intentional, but does not change the guide. If it clutters up the diff, I can remove it...

jvican · 2017-10-12T12:40:02Z

CONTRIBUTING.md

+
+How is the Zinc build structured? Let's see it.
+
+|Project name| Project description|


This part should be carefully reviewed. I did my best to explain what these projects are used for, but this structure pre-dates me, so someone has to confirm. Related to #286.

Looks good enough to me.

jvican · 2017-10-12T13:16:56Z

For some weird reason, GitHub does not show the latest commit and its CI status.

dwijnand

For some reason I can't respond to the thread above, so I'll respond here:

No, you cannot. If you do import Keys._, moduleID will be in scope, so if I define val moduleID = moduleID.value Scala will think it's a recursive definition and will fail both at compile-time and runtime (in case you ever make it to typecheck).

I said you can still do val moduleID = Keys.moduleID.value.  

Say I define a local method called publish that defines how a thing is going to be published. If I've done import Keys._, publish will resolve to different terms depending on the scope I use them, which is misleading and a source of bugs. I don't want my code to change its meaning depending on the place I use them. Not only it's not refactor friendly, it's confusing and dangerous. 
 The problem gets worse when you don't even know that you're importing a term from Keys.scala that is conflicting with a term you've defined. What about artifact, doc, mainClass, run and the other bazillion of names that are really likely to appear in my build logic and conflict with my definitions?  

That's Scala for you, nothing to do with sbt per-se. You need to play with scala scopes and/or find other names.  

That's a matter of style. I don't mind the second version. My arguments here have to do with semantics and user readability (in terms of not having name clashes -- when users read they don't need to think if a term is defined at the end of the file, in an enclosing scope or comes from sbt -- they have a clear idea that Keys.artifact means the setting key from sbt).

  I don't think the user cares where a key is defined, and would find it more user-friendly if the same syntax as the shell were used.

dwijnand · 2017-10-12T12:58:58Z

CONTRIBUTING.md

+
+How is the Zinc build structured? Let's see it.
+
+|Project name| Project description|


Looks good enough to me.

dwijnand · 2017-10-12T12:59:23Z

CONTRIBUTING.md

@@ -31,8 +31,7 @@ Once you understand the basics of incremental compilation, start having a look
 at open tickets you can help with. All issues are labelled and will give you an
 idea about its difficulty and scope.

-Hacking on Zinc should not seem like a difficult task. Zinc does not implement
-a compiler, it defines the logic to analyse dependencies based on the compiler
+Hacking on Zinc should not seem like a difficult task. Zinc does not implement a compiler, it defines the logic to analyse dependencies based on the compiler


dwijnand · 2017-10-12T13:15:40Z

project/BuildPlugin.scala

+
+  override def projectSettings: Seq[Def.Setting[_]] = BuildImplementation.projectSettings
+  override def buildSettings: Seq[Def.Setting[_]] = BuildImplementation.buildSettings
+  override def globalSettings: Seq[Def.Setting[_]] = Nil


dwijnand · 2017-10-12T13:16:02Z

project/BuildPlugin.scala

+      Command.command("runBenchmarks")(st => runPreSetup :: runBenchmark :: tearDownResources :: st)
+    }
+
+    def all(bridge: Project, interface: Project, apiInfo: Project, bench: Project): Seq[Command] = {


Could you make these ProjectReference please? We don't need the actual project, just a reference to it.

We cannot because we need id, and id is not defined in ProjectReference.

implicit def projectToRef(p: Project): ProjectReference = LocalProject(p.id)

If only this returned LocalProject instead of ProjectReference..

@dwijnand Yeah... can maybe this new method be added in sbt 1.x? I think it's bincompat:

@ class A class B edefined class A @ class B extends A defined class B @ implicit def stringToA(s: String): A = new A defined function stringToA @ implicit def stringToB(s: String): B = new B defined function stringToB

yep, I think we should consider it.

jvican · 2017-10-12T14:13:53Z

I said you can still do val moduleID = Keys.moduleID.value.  

Sure, but then are you not inconsistent? You use a wildcard import and then you use the prefix to disambiguate -- it's weird.

Also, this assumes you know what import sbt.Keys._ does import. We know, but most users don't.

That's Scala for you, nothing to do with sbt per-se. You need to play with scala scopes and/or find other names.  

Agreed, nobody said is sbt specific. But it's an important problem. I want to write my build so that scoping problems are minimized and I don't need to be constantly thinking about variations of my names (that are stupid and can bring bugs, as said before) just to avoid name clashes produced by unnecessary wildcard imports.

I'm not the only one that does it this way, see this comment by Li Haoyi: https://contributors.scala-lang.org/t/proposed-syntax-for--root-/1035/37?u=jvican.

  I don't think the user cares where a key is defined, and would find it more user-friendly if the same syntax as the shell were used.

Well, I don't know if they care, but I believe they should care because the place where it's defined alters semantics of their code...

@dwijnand PR is updated with your last feedback. Thanks!

eed3si9n · 2017-10-12T16:09:53Z

build.sbt

@@ -288,7 +288,7 @@ lazy val zincCompileCore = (project in internalPath / "zinc-compile-core")
 // defines Java structures used across Scala versions, such as the API structures and relationships extracted by
 //   the analysis compiler phases and passed back to sbt.  The API structures are defined in a simple
 //   format from which Java sources are generated by the sbt-contraband plugin.
-lazy val compilerInterface = (project in internalPath / "compiler-interface")
+lazy val compilerInterface = (project in file("compiler-interface"))


My opinion is that some datatype might be exposed, but in general the notion of compiler interface (abstraction over Scala compiler version) should be considered internal to Zinc. In other words, unless otherwise stated, we reserve the right to change compiler interface signature from Zinc 1.0 to 1.1, as long as it's not exposed to Zinc API.

My opinion is that some datatype might be exposed, but in general the notion of compiler interface (abstraction over Scala compiler version) should be considered internal to Zinc.

But the compiler interface does not only expose abstractions over Scala versions, it has all the facilities that are afterwards exposed to downstream users (loggers, reporters, scala instance, etc). Lots of these things also leak to sbt's API.

In other words, unless otherwise stated, we reserve the right to change compiler interface signature from Zinc 1.0 to 1.1, as long as it's not exposed to Zinc API.

This rule you propose is unexpected to me and doesn't satisfy the bincompat guarantees that other Scala tools do, like scalac. It's not even the same than the one for sbt, right?

I think we should enforce that binary compatibility is not broken in minor releases, especifically for Zinc, which is not only used by sbt but also by other builds tools that also have strong bincompat guarantees.

In any case, binary compatibility aside, I think this change is good. We currently have two top-level modules (one zinc and another one zinc-compile). The compiler interface is in my opinion the most important module in the project, and I think we should highlight it by placing it in the root of the project. It doesn't make sense to me that zinc-compile looks, at first glance, more important than the compiler interface.

Let's continue #438

eed3si9n · 2017-10-12T16:17:59Z

project/BuildPlugin.scala

+    ScalafmtKeys.scalafmtOnCompile := true,
+    ScalafmtKeys.scalafmtVersion := "1.2.0",
+    ScalafmtKeys.scalafmtOnCompile in Sbt := false,
+    Keys.description := "Incremental compiler of Scala",


Inside of an AutoPlugin definition it is customary to import Keys._. I tend to import any autoImports from plugins that I works on to emulate what one would have in build.sbt.

jvican · 2017-10-16T08:58:47Z

Is there anything I can do here to move this forward? I've replied to all your concerns/questions.

jvican · 2017-10-17T16:15:48Z

Commits have been squashed. It looks like GitHub is not catching up with the parent branch.

This is a squashed commit. This is a general list of the things that are done. - Port common settings and commands to BuildPlugin - Remove repeated resolvers - Move `relaxNon212` to the build plugin - Move build and alternative publish settings. This commit consolidates the build settings and the settings to set up the alternative publishing process into the `BuildPlugin`. - Move `noPublish` to the `BuildPlugin` - Move the rest of the build logic to the plugin

Duhemm · 2017-11-21T10:14:27Z

CONTRIBUTING.md

+|zincPersist|The project that persists incremental compiler's data into a binary file.|
+|zincCore|The project that defines relations, analysis, stamps, and essential core utils.| 
+|zincBenchmarks|The project that defines the benchmarks.|
+|zincIvyIntegration|The project that defines the ivy utilities to fetch compiler bridges.|


I thought that we agreed to rename that project to zincLmIntegration, but it looks like it didn't make it to the PR that superseded mine. See: #335 (comment)

Maybe now is a good time to do it?

I'd very much prefer not to touch this PR again, I've already done too many changes 😄

Duhemm · 2017-11-21T10:50:16Z

CONTRIBUTING.md

+
+|Key|Use|
+|---|---|
+|crossTestBridges|Runs compiler bridge unit tests for all scala versions.|


Will it still be true after #453 gets in? I don't know why we don't test the bridge with 2.13.0-M2.

#453 has to be rebased on top of this, and Eugene has to do whatever changes do not hold in his PR.

Duhemm · 2017-11-21T10:52:44Z

CONTRIBUTING.md

+|zincClasspath|The project that provides basic utilities to load libraries with classloaders and represents Scala instances.|
+|zincScripted|The project that defines the scripted logic to run Zinc's integration test suite.|
+|compilerInterface|The public binary interface used to connect the bridges with the Zinc modules. It is written in Java and uses Contraband.|
+|compilerBridge|The module that defines the compiler plugin phases that provide incrementality for all Scala versions.|


We'll need to add compilerBridgeTest to the table after #453 gets in.

Same comment as before.

eed3si9n · 2017-11-21T20:30:06Z

Only bug fixes (or improvements needed for bug fixes) goes into 1.0.x branch, so this PR imo should target 1.x.

jvican · 2017-11-21T22:04:37Z

@eed3si9n That policy has not been discussed between you and I at any point, and that does not correspond with other PRs that have been merged into the 1.0.x line.

This PR and the other one have been here for more than 25 days waiting for a review, and hasn't yet been reviewed since our last meeting three weeks ago... I've put so much time into rebasing the PRs and adapting it to other changes, that I'm not willing to do it again.

eed3si9n · 2017-11-22T01:00:59Z

The 1.x vs 1.0.x convention was discussed here - https://github.com/sbt/sbt-contrib/blob/master/src/main/markdown/2017-08-16.md#general-release-cycle.
If there has been 1.0.x merges that are not for hot fixes, then likely they were mistakes.

As per the prioritization of PRs, the themes around each releases are discussed in sbt 1 meetings, and discussions in last few meetings have been wrapping things up for 1.0.4 and 1.1-RC. It's not uncommon to have PR sit on the queue for a while if they are not critical part of the milestone. For example sbt/sbt#3621 has been sitting since Oct 8 (I think it's ready to merge, but hey).

jvican · 2017-11-22T08:08:01Z

This is not sbt, this is Zinc, and the Scala Center has proved more than several times its great involvement in this project. To the best of my knowledge, we both have an spoken deal to drive the direction of this project, which is important because the Scala community needs our collaboration. That means that whatever it's applicable to sbt/sbt, it's not necessarily applicable here. As you said in numerous occasions, sbt != zinc and we're not Lightbend.

From reading your message, it looks like you're the only one that chooses the priority of the PRs and I'm sorry to tell you but that's not how it works. Your actions have consequences in my work, and that's why it's good to discuss things before taking unilateral actions.

Your wish is also inconsistent, lots of things that are not bug fixes have been merged into the 1.0.x line, sorry.

I don't think I'm asking too much when I say that I want my work to be respected. If it's not reviewed on time and keeps being delayed, it's not my fault. I've been rebasing this thing and I've already given up on lots of design decisions just to make you happy. Same with the bincompat restrictions that you want and I don't.

jvican · 2017-11-22T08:09:26Z

And for those reading this discussion, here's where our disagreement started: #453.

jvican · 2017-11-22T12:10:53Z

You have to understand that we're working on a team, and if you want to make a change at the last minute and override all the work that other team members have put into Zinc, it's not nice. It means I need to stop the world, come here, and update all the stuff just because you think your PR has more priority than mine. And this wouldn't have happened if you had reviewed my PRs in due time (and ironically you ping me several times in less than one week to review your PR because it's blocking a release).

I've done many concessions in the past weeks to accommodate your likes and what not (including your controversial opinions on bincompat), and I see no effort from your side to respect the work and time I am investing in this project, sometimes on my free time.

If you considered this was a change required in 1.x, you should have said that one month ago, and discussed with me this rule (not take the decision unilaterally in some old sbt meeting minutes I didn't attend). Not doing so is wasting my time and our time. You can at least ping me in Gitter to notify me your plan to ship something in 1.x that overrides my changes.

Being in a team means agreeing to make things happen together, not taking unilateral actions and then asking others to catch up with them.

It's also important that you don't assume that the sbt release process is the same. If we're making this thing happen, and improving the incremental compiler for the whole community, then it's only reasonable we take different decisions (because the sbt team is not the only one in charge of this project).

I think support for Scala 2.13.x is important. But I'm only happy to merge your fix to support Scala 2.13.x if you are the one that rebases all my changes in 1.x (keeping git history) once your PR gets merged. As an author of these two PRs, I will obviously address more feedback if I get it, but I refuse to swallow the consequences of your poor management of this issue.

I know you're used to take decisions in sbt/sbt, but I hope you don't realistically hope to dictate how development of Zinc should be when more than one party is involved into it.

And I say this in the nicest of the tones. I'm just being honest and direct.

eed3si9n · 2017-11-23T00:25:23Z

This is not sbt, this is Zinc, and the Scala Center has proved more than several times its great involvement in this project. To the best of my knowledge, we both have an spoken deal to drive the direction of this project, which is important because the Scala community needs our collaboration. That means that whatever it's applicable to sbt/sbt, it's not necessarily applicable here. As you said in numerous occasions, sbt != zinc and we're not Lightbend.

Agreed. And it's worth repeating that both Lightbend Tooling team and Scala community are grateful for Scala Center's contributions.

From reading your message, it looks like you're the only one that chooses the priority of the PRs and I'm sorry to tell you but that's not how it works. Your actions have consequences in my work, and that's why it's good to discuss things before taking unilateral actions.

I've seen this claim repeated a few times, but I see it in a different way. During the development leading up to sbt 1 we've held multiple planning meetings together to discuss the todo list on what goes into RC-x. After sbt 1.0, there's been sbt 1 meetings every week since August to coordinate what goes in, when, and who wants to work on them. We discuss Zinc task planning here too. So even though Lightbend is the owner of Zinc, and I'm the lead, the governance of sbt or Zinc has been relatively open.

Your wish is also inconsistent, lots of things that are not bug fixes have been merged into the 1.0.x line, sorry.

There's been three hot fix releases of Zinc thus far from 1.0.x branch: 1.0.1, 1.0.2, and 1.0.3.
As far as I know, all of the changes that went into them were for bug fixes that we shipped together with sbt 1.0.x hot fixes. If there has been any other PRs, then it had to be low/zero risk (like adding headers) or a mistake. Could you point to an example?

I don't think I'm asking too much when I say that I want my work to be respected. If it's not reviewed on time and keeps being delayed, it's not my fault. I've been rebasing this thing and I've already given up on lots of design decisions just to make you happy. Same with the bincompat restrictions that you want and I don't.

I don't think it's a matter of respect. If there's an objectively verifiable bug in the code (e.g. using the result of a method that sometimes returns null, causing NPE), I think the review process for a fix would be straightforward since the fix can also be verified. Refactoring starts to involve matters of taste, and it could take a while to converge choices/decisions. Given the 59 comments, it's been given enough attention.

I also understand that PR taking a long time is frustrating, and we should work towards improving that.

eed3si9n · 2017-11-23T01:42:30Z

You have to understand that we're working on a team, and if you want to make a change at the last minute and override all the work that other team members have put into Zinc, it's not nice. It means I need to stop the world, come here, and update all the stuff just because you think your PR has more priority than mine. And this wouldn't have happened if you had reviewed my PRs in due time (and ironically you ping me several times in less than one week to review your PR because it's blocking a release).

Since there's been multiple conversations on Gitter etc around 1.0.x or sbt 1.0.x hot fixes, I was totally under the impression that you knew of the 1.x and 1.0.x distinction and were on board with the idea.

If the criteria or the process was somehow not clear, I think it was a miscommunication on my part. As I wrote in the previous comment too, "what goes into what release" is one of the things we discuss in the sbt 1 meetings. In other words, it's not decided unilaterally. If you can't attend it personally, you're welcome to send someone else, write in a email, or schedule chat with me afterwards.

I've done many concessions in the past weeks to accommodate your likes and what not (including your controversial opinions on bincompat), and I see no effort from your side to respect the work and time I am investing in this project, sometimes on my free time.

See #428 (comment). I acknowledge that long PR process can be frustrating, but I don't think it's a matter of respect. The PR was sent in on Oct 12, and within 24h @dwijnand has given lots of feedback. It dropped from our radar for a while when we couldn't bring it to consensus in time. Other PRs during this period were reviewed and merged when it was simple (like related #438).

Also note that 2 days ago I've asked Dale to come up with a pending list if he thinks there's any work remaining on this PR when you pinged me.

If you considered this was a change required in 1.x, you should have said that one month ago, and discussed with me this rule (not take the decision unilaterally in some old sbt meeting minutes I didn't attend). Not doing so is wasting my time and our time. You can at least ping me in Gitter to notify me your plan to ship something in 1.x that overrides my changes.

You're right that either Dale or I should've spotted that this PR targets 1.0.x branch much sooner. This was an oversight on our part.

Being in a team means agreeing to make things happen together, not taking unilateral actions and then asking others to catch up with them.

Agreed. To be fair though, there were attendees from Scala Center in the initial meeting when we discussed the branching strategy, so from my point of view these decisions were made though discussion not by fiat.

It's also important that you don't assume that the sbt release process is the same. If we're making this thing happen, and improving the incremental compiler for the whole community, then it's only reasonable we take different decisions (because the sbt team is not the only one in charge of this project).

Yes. My goal for Zinc is to be a useful incremental compiler for any build tool, which might take different decisions in terms of incrementality, strictness on repeatability (Bazel / Pants comes to mind here) etc. The strategy sbt uses for its releases is SemVer, and it does carry over to Zinc in many ways, but I am hopeful that we can achieve these goals.

I think support for Scala 2.13.x is important. But I'm only happy to merge your fix to support Scala 2.13.x if you are the one that rebases all my changes in 1.x (keeping git history) once your PR gets merged. As an author of these two PRs, I will obviously address more feedback if I get it, but I refuse to swallow the consequences of your poor management of this issue.

If what's needed to ship Scala 2.13.x is hinging on time to git, both Dale and I are happy to take on the task. After merging 1.0.x into 1.x, hopefully it would be a straight merge.

dwijnand · 2017-11-23T11:03:33Z

If what's needed to ship Scala 2.13.x is hinging on time to git, both Dale and I are happy to take on the task. After merging 1.0.x into 1.x, hopefully it would be a straight merge.

See #456

jvican requested review from dwijnand, eed3si9n and Duhemm and removed request for dwijnand October 12, 2017 09:55

jvican added build discussion Scala Center labels Oct 12, 2017

dwijnand requested changes Oct 12, 2017

View reviewed changes

jvican force-pushed the improvements-sbt-build branch 3 times, most recently from 9f29520 to c24b2d9 Compare October 12, 2017 12:35

jvican commented Oct 12, 2017

View reviewed changes

jvican force-pushed the improvements-sbt-build branch from c24b2d9 to f431825 Compare October 12, 2017 12:47

jvican mentioned this pull request Oct 12, 2017

Reorder subproject dependencies #286

Closed

jvican force-pushed the improvements-sbt-build branch 2 times, most recently from f780be3 to a3526d9 Compare October 12, 2017 13:16

dwijnand requested changes Oct 12, 2017

View reviewed changes

jvican force-pushed the improvements-sbt-build branch from a3526d9 to 8c52632 Compare October 12, 2017 13:50

jvican mentioned this pull request Oct 12, 2017

Set up sbt-release-early and change build semantics #429

Closed

eed3si9n requested changes Oct 12, 2017

View reviewed changes

jvican removed the request for review from Duhemm October 12, 2017 22:24

jvican added this to the 1.0.3 milestone Oct 16, 2017

jvican force-pushed the improvements-sbt-build branch from 5e17b7e to 502d889 Compare October 17, 2017 16:21

dwijnand added this to the 1.1.0 milestone Nov 2, 2017

jvican force-pushed the improvements-sbt-build branch 3 times, most recently from 72221a3 to f617213 Compare November 20, 2017 20:28

jvican added 7 commits November 20, 2017 21:38

Add headers to missing files

62a0cb2

Address Dale's feedback

59392e4

Add checks, remove defaults and add headers to test sources

fc859fa

Add build information to the CONTRIBUTING guide

d46f22b

Add project structure diagram to the guide

2ad8ac6

Address last piece of feedback from Dale

fd34606

jvican force-pushed the improvements-sbt-build branch from f617213 to fd34606 Compare November 20, 2017 20:38

jvican requested a review from Duhemm November 20, 2017 21:34

Duhemm reviewed Nov 21, 2017

View reviewed changes

jvican mentioned this pull request Nov 21, 2017

"sbt '++ 2.13.0-M2!' compile" does not work with sbt 1.0.0 #453

Merged

dwijnand mentioned this pull request Nov 22, 2017

Consider changing projectToRef to return LocalProject instead of ProjectReference sbt/sbt#3757

Closed

dwijnand mentioned this pull request Nov 23, 2017

Refactor the build^2 #456

Closed

dwijnand modified the milestones: 1.1.0, 1.something Dec 13, 2017

dwijnand closed this Jan 10, 2018

dwijnand removed this from the 1.something milestone May 4, 2020


		How is the Zinc build structured? Let's see it.

		\|Project name\| Project description\|

Refactor the build #428

Refactor the build #428

Conversation

jvican commented Oct 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican commented Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican commented Oct 12, 2017

dwijnand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican commented Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

jvican Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican commented Oct 16, 2017

jvican commented Oct 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eed3si9n commented Nov 21, 2017

jvican commented Nov 21, 2017 • edited Loading

eed3si9n commented Nov 22, 2017

jvican commented Nov 22, 2017 • edited Loading

jvican commented Nov 22, 2017

jvican commented Nov 22, 2017 • edited Loading

eed3si9n commented Nov 23, 2017

eed3si9n commented Nov 23, 2017

dwijnand commented Nov 23, 2017

jvican Oct 12, 2017 •

edited

Loading

jvican Oct 12, 2017 •

edited

Loading

jvican Oct 12, 2017 •

edited

Loading

jvican commented Oct 12, 2017 •

edited

Loading

jvican Oct 12, 2017 •

edited

Loading

jvican commented Oct 12, 2017 •

edited

Loading

jvican Oct 12, 2017 •

edited

Loading

jvican commented Nov 21, 2017 •

edited

Loading

jvican commented Nov 22, 2017 •

edited

Loading

jvican commented Nov 22, 2017 •

edited

Loading