Improve workflow: Option to ignore lock.sbt dependencyOverrides when hash is stale #24

htmldoug · 2018-12-09T23:56:41Z

Summary

Changes generation of lock.sbt files to produce something like:

// DON'T EDIT THIS FILE.
// This file is auto generated by sbt-lock 0.5.1-SNAPSHOT.
// https://github.com/tkawachi/sbt-lock/
dependencyOverrides ++= {
  if (!(sbtLockHashIsUpToDate in ThisBuild).value && sbtLockIgnoreOverridesOnStaleHash.value) {
    Set.empty
  } else {
    Set(
      "com.fasterxml.jackson.core" % "jackson-core" % "2.2.2"
    )
  }
}
// LIBRARY_DEPENDENCIES_HASH 7da3f2eacaf3740fecfd2bff3fb1679c8e37241e

Problem Background

The ingrained workflow with sbt is:

change libraryDependencies
> reload
observe changes

I've introduced sbt-lock at work, but we're all getting tripped up by it regularly, which has halted adoption. Coworkers make libraryDependency changes which don't take effect because of lock.sbt, then they spend the next hour assuming the dependency is broken and looking upstream.

I think one contributing factor might that sbt handles dependency resolution automatically. There is no npm install step. You just edit libraryDependencies and you're generally good to go.

It's easy to forget that the repo they're making changes to is one of the few that use sbt-lock. The sbt console logging gets lost in the rest of the log spam noise. Requiring a behavior change is proving too difficult.

Proposed Solution

This PR adds a sbtLockIgnoreOverridesOnStaleHash := true key to allow libraryDependencies changes to take effect on reload, even without a ;unlock;reload;lock. We'll still enforce this in CI builds (e.g. require((sbtLockHashIsUpToDate in ThisBuild).value)), but local development workflow can remain the same way everyone's accustomed to.

I've set a default value of sbtLockIgnoreOverridesOnStaleHash := false to allow behavior to remain as it is today for existing consumers. We'll enable it in all our projects. If you think it's better to eliminate the complexity and just change the behavior, I'd be happy to do that too.

@tkawachi is happy with the approach
update README
Add clearer logging around the new behavior

cc: @Cake @jrouly @patrickjcurran @rcmurphy @arkban

htmldoug · 2018-12-10T00:08:45Z

I'm not sure why https://travis-ci.org/tkawachi/sbt-lock/jobs/465760029 failed. Both test and scripted pass locally. Has oraclejdk7 been removed from travis? I could use some help with that.

tkawachi · 2018-12-11T01:35:24Z

Hi, Thanks for the PR!
I feel good about this approach. Please go ahead.

src/sbt-test/sbt-lock/staleHash/test

htmldoug · 2018-12-12T04:02:38Z

Okay, I'm done addressing my TODOs. Since last review:

oraclejdk7 => openjdk7
README updated
Test assertions of the sbtLockHashIsUpToDate: SettingKey[Boolean] values

Ready for another review.

arkban · 2018-12-12T15:21:16Z

README.md

@@ -25,17 +25,23 @@ addSbtPlugin("com.github.tkawachi" % "sbt-lock" % "0.5.0")
  `lock.sbt` includes `dependencyOverrides` for all dependent library versions.
  Manage it with version control system.
 * `unlock` to delete `lock.sbt` file.
+* `checkLockUpdate` to print whether the lock file needs an update.


It prints? It's not just a value? I'd name it "printLockUpdate" or "printLockStatus" or "printLockNeedsUpdate" or something.

Yep, taskKey[Unit]. This key already existed. I'm just documenting. Going to defer to @tkawachi if he wants to rename it.

If we do rename it, https://www.scala-sbt.org/1.0/docs/Plugins-Best-Practices.html#Key+naming+convention%3A+Use+prefix encourages namespacing them with a prefix.

I agree to rename it. It's better to rename in another PR.

arkban · 2018-12-12T15:21:29Z

src/main/scala/com/github/tkawachi/sbtlock/SbtLockKeys.scala

@@ -8,4 +8,6 @@ object SbtLockKeys {
  val unlock = taskKey[Unit]("Delete a version locking file for sbt-lock")
  val collectLockModuleIDs = taskKey[Seq[ModuleID]]("Collect ModuleIDs to lock")
  val checkLockUpdate = taskKey[Unit]("Check whether a version locking file needs update")


I think the comment here should match the readme.

arkban · 2018-12-12T15:21:55Z

src/main/scala/com/github/tkawachi/sbtlock/SbtLock.scala

+         |    ${Compat.dependencyOverridesType}.empty
+         |  } else {
+         |    ${Compat.dependencyOverridesType}(
+         |      ${moduleLines.mkString(",\n      ")}


but don't you want to get paid more? https://stackoverflow.blog/2017/06/15/developers-use-spaces-make-money-use-tabs/

arkban · 2018-12-12T15:24:20Z

src/main/scala/com/github/tkawachi/sbtlock/SbtLockPlugin.scala

+       * project dependency tree (including transitive ones),
+       * but I don't know how to do that in sbt.
+       */
+      (sbtLockHashIsUpToDate in ThisBuild).value && sbtLockHashIsUpToDate.value


This looks like recursive insanity, I can't believe SBT doesn't just explode when you do this :D

3 layers of interpretation. yay?

arkban · 2018-12-12T15:26:05Z

src/main/scala/com/github/tkawachi/sbtlock/SbtLockPlugin.scala

@@ -67,6 +102,10 @@ object SbtLockPlugin extends AutoPlugin {
              s"Run `;unlock ;reload ;lock` to re-create ${lockFile.name}.")
            logger.warn(
              s"Run just `lock` instead if you want to keep existing library versions.")
+            if (ignoreOnStaleHash) {


This feels like it should go before the if (hashInFile != currentHash) condition, or maybe prevent all the "run lock instructions" stuff from printing so it's not lost in the noise.

Actually I'm wondering if it might be nice to print out this warning whether the lock is up to date or not...

Printing it all the time sounds like more noise. I'd worry about desensitizing people to it.

src/sbt-test/sbt-lock/staleHash/test

htmldoug · 2018-12-13T17:35:19Z

@tkawachi thank you for the quick review, merge, and release. You're wonderful.

Ignore lock.sbt when hash is stale.

aa76fb8

htmldoug force-pushed the workflow branch 2 times, most recently from 441d5b6 to 9215c0c Compare December 12, 2018 02:08

Maybe openjdk7 will work?

8a8bc6b

htmldoug force-pushed the workflow branch from 254eb41 to 7331165 Compare December 12, 2018 02:45

htmldoug commented Dec 12, 2018

View reviewed changes

src/sbt-test/sbt-lock/staleHash/test Show resolved Hide resolved

htmldoug force-pushed the workflow branch from fb338c0 to 36812a1 Compare December 12, 2018 04:00

Logging and minor tweaks.

a9f284f

arkban approved these changes Dec 12, 2018

View reviewed changes

htmldoug force-pushed the workflow branch from 36812a1 to 09efefb Compare December 12, 2018 21:19

Clarify sbtLockHashIsUpToDate description re: transitive project deps.

b9e53b1

htmldoug force-pushed the workflow branch from 09efefb to b9e53b1 Compare December 12, 2018 21:35

tkawachi merged commit 6f3e53e into tkawachi:master Dec 13, 2018

htmldoug deleted the workflow branch December 13, 2018 17:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve workflow: Option to ignore lock.sbt dependencyOverrides when hash is stale #24

Improve workflow: Option to ignore lock.sbt dependencyOverrides when hash is stale #24

htmldoug commented Dec 9, 2018 •

edited

htmldoug commented Dec 10, 2018 •

edited

tkawachi commented Dec 11, 2018

htmldoug commented Dec 12, 2018

arkban Dec 12, 2018

htmldoug Dec 12, 2018 •

edited

tkawachi Dec 13, 2018

arkban Dec 12, 2018

arkban Dec 12, 2018

htmldoug Dec 12, 2018

arkban Dec 12, 2018

htmldoug Dec 12, 2018

arkban Dec 12, 2018

htmldoug Dec 12, 2018

htmldoug commented Dec 13, 2018

Improve workflow: Option to ignore lock.sbt dependencyOverrides when hash is stale #24

Improve workflow: Option to ignore lock.sbt dependencyOverrides when hash is stale #24

Conversation

htmldoug commented Dec 9, 2018 • edited

Summary

Problem Background

Proposed Solution

htmldoug commented Dec 10, 2018 • edited

tkawachi commented Dec 11, 2018

htmldoug commented Dec 12, 2018

Choose a reason for hiding this comment

htmldoug Dec 12, 2018 • edited

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

htmldoug commented Dec 13, 2018

htmldoug commented Dec 9, 2018 •

edited

htmldoug commented Dec 10, 2018 •

edited

htmldoug Dec 12, 2018 •

edited