Updating to sbt 1.x #11

laughedelic · 2017-10-02T23:09:37Z

This is based on the branch from #10.

I tried first to make minimal changes to make it compile. It seems to pass all tests on CI, except one: example-sonatype. It says destination file exists and overwrite == false, but the version is randomly generated, so I don't know what is the real issue.

And besides that, it still needs some work. Some things have to be improved and I think some places could be simplified. I'll continue with this tomorrow.

jvican · 2017-10-02T23:14:18Z

src/main/scala/ch/epfl/scala/sbt/release/ReleaseEarlyPlugin.scala

-    val releaseEarlyPublishTo: Def.Initialize[Option[sbt.Resolver]] = {
-      Def.setting {
+    val releaseEarlyPublishTo: Def.Initialize[Task[Option[sbt.Resolver]]] = {
+      Def.taskDyn {


Why is this a task instead of a dynamic setting?

Because in sbt-1.0 publisTo became a task.

I really wonder why. I just went throught the code in sbt-bintray and sbt-sonatype and they don't seem to need this to be a task. The default code in sbt defaults to None. @dwijnand Sorry for bothering, but do you know anything about the reason for this signature change?

Relevant issues:

Change publishTo type from setting Key to task Key sbt/sbt#2059

[sbt 1.0] Make publishTo & otherResolvers TaskKey's sbt/sbt#2662

Alright, makes sense. @dwijnand No need for you to intervene!

jvican · 2017-10-02T23:15:22Z

src/main/scala/ch/epfl/scala/sbt/release/ReleaseEarlyPlugin.scala

    // Ensure licenses before releasing
    val useBintray = !PrivateKeys.releaseEarlyIsSonatype.value
-    if (useBintray) bintray.BintrayKeys.bintrayEnsureLicenses.value
-    if (hasErrors) sys.error(Feedback.fixRequirementErrors)
+    if (useBintray) Def.task {


No need to wrap in a task here, you can return bintray.BintrayKeys.bintrayEnsureLicenses directly.

Right. I'll fix it.

Also see https://github.com/scalacenter/sbt-release-early/pull/11/files/558ded9b100c95408c46fa0be149115148b11203#r142282198. It's the same situation here.

Oh, nevermind, I just saw the removed hasErrors line here.

Fixed in df66800

jvican · 2017-10-02T23:15:51Z

src/main/scala/ch/epfl/scala/sbt/release/ReleaseEarlyPlugin.scala

@@ -491,7 +496,7 @@ trait Helper {
      nextState.remainingCommands.toList match {
        case Nil => nextState
        case head :: tail =>
-          runCommand(head, nextState.copy(remainingCommands = tail))
+          runCommand(head.commandLine, nextState.copy(remainingCommands = tail))


Oh wow, this is weird. Why is this required?

I think before it took a String, but now it's an Exec. I'm not familiar with this stuff, I tried .toString here, it didn't work, so I replaced it with .commandLine, but I still don't have much idea about it. Sorry 🤷‍♂️

I'll have a closer look then, don't worry!

jvican · 2017-10-02T23:17:04Z

src/sbt-test/sbt-release-early/simple-module-missing-developers/project/plugins.sbt

@@ -1,5 +1,5 @@
 logLevel := Level.Warn
-addSbtPlugin("com.jsuereth" % "sbt-pgp" % "1.0.0")
+addSbtPlugin("com.jsuereth" % "sbt-pgp" % "1.1.0")


I thikn we should remove all these explicit plugin declarations here, sbt-pgp is brought in by the sbt-release-early plugin. I'll do this in a follow-up commit tomorrow.

Done in a76ab92

jvican · 2017-10-02T23:17:49Z

project/BuildPlugin.scala

@@ -15,7 +15,8 @@ object BuildPlugin extends AutoPlugin {
 }

 object BuildDefaults {
-  import sbt.{url, file, richFile}
+  import sbt.url
+  import sbt.io.syntax._


Can we avoid this wildcard import and import instead richFile and file directly?

Fixed in df66800

jvican · 2017-10-02T23:26:43Z

src/main/scala/ch/epfl/scala/sbt/release/ReleaseEarlyPlugin.scala

@@ -474,10 +476,13 @@ trait Helper {
        } else hasError
    }

+    if (hasErrors) sys.error(Feedback.fixRequirementErrors)


Why has this new line be added here?

Just some freestyle formatting 😜 I just noticed that you have a scalafmt.conf, but the plugin is not included. Should I apply scalafmt formatting?

Ah, and this line was moved from below to be called before that resulting def-task is returned. If it fails here, it doesn't change anything, so I needed to move it.

But don't we have now two repeated lines in the same source? This is a new addition, it does not replace or remove anything. The same line seems to exist in checkRequirementsTask.

I'm not sure I understand. This is in the validatePomTask. This line is similar to the one in checkRequirementsTask, but does a different check. So both tasks have a if (hasErrors) sys.error line. In this function I moved the line from here.

laughedelic · 2017-10-02T23:34:24Z

src/main/scala/ch/epfl/scala/sbt/release/ReleaseEarlyPlugin.scala

+    val bintrayCredentialsOpt =
+      catching(classOf[NoSuchElementException])
+        .opt(bintray.BintrayKeys.bintrayEnsureCredentials.value)
+


This should be done better. In the previous version it was called in an if branch, which means that it was always called. I changed it to equivalent code just calling outside of if. But it could be done better if everything was build around the taskDyn-Def.task pattern..

IIRC it was called inside the if branch on purpose to check that credentials are okay independently of our own checks. So this hoisted up value is way better as it is.

I don't know. I just see that there is a check useSonatype (which is PrivateKeys.releaseEarlyIsSonatype.value) and I think that if we use sonatype, there is no point to check bintray credentials, even if we can catch the exception and wrap it in an option.

I think the whole task could be structured more like this:

Def.taskDyn { // ... if (useSonatype) Def.task { // do sonatype related checks } else Def.task { // do bintray related checks } }

Sure thing, this is an improvement over the status quo.

Done in a7be687

laughedelic · 2017-10-02T23:51:55Z

src/main/scala/ch/epfl/scala/sbt/release/ReleaseEarlyPlugin.scala

@@ -128,7 +128,7 @@ object ReleaseEarly {

    /* Sbt bug: `Def.sequential` here produces 'Illegal dynamic reference' when
     * used inside `Def.taskDyn`. This is reported upstream, unclear if it can be fixed. */
-    private val StableDef = new sbt.TaskSequential {}
+    private val StableDef = new sbt.internal.TaskSequential {}


I think that the part where this is used could be simplified:

https://github.com/scalacenter/sbt-release-early/blob/a76ab921a6523f834dbbfc33a6a0659856c954b7/src/main/scala/ch/epfl/scala/sbt/release/ReleaseEarlyPlugin.scala#L294-L302

Could you explain the logic behind this code? You want to run all tasks from the releaseEarlyProcess sequence and return a unit, right?

This code gets a sequence of tasks that may or may not have been modified by users and execute them inside a dynamic task via sequential. The return type is always ()

I simplified it just a bit: 7af1257. Couldn't do anything about that strange StableDef thing, but I found the issue you mention in the comment.

jonas · 2017-10-03T14:50:54Z

build.sbt

  .settings(
    sbtPlugin := true,
    pgpPublicRing := file("/drone/.gnupg/pubring.asc"),
    pgpSecretRing := file("/drone/.gnupg/secring.asc"),
-    scalaVersion := "2.10.6",
+    scalaVersion := "2.12.3",
+    sbtVersion in Global := "1.0.2",


If the plan is to cross publish for both sbt 0.13 and sbt 1.0 I suggest to replacing these with

crossSbtVersions := List("0.13.16", "1.0.2"), scalaVersion := { (sbtBinaryVersion in pluginCrossBuild).value match { case "0.13" => "2.10.6" case _ => "2.12.3" } },

I don't know what's @jvican's plan, but I actually don't see any point in cross publishing for 0.13 and 1.0 (now that 1.0 has been released). Especially if the code needs to be altered, it only makes it harder to maintain.
And if some users are "stuck" on sbt 0.13, they can happily use previous version of the plugin until they are ready to upgrade.

Yeah, let's not cross-build. If this really becomes a problem for users, we can rethink it in the future. Those stuck in 0.13.x can use 1.2.

This decision will obviously require a 2.0 release.

laughedelic · 2017-10-03T19:09:13Z

src/main/scala/ch/epfl/scala/sbt/release/ReleaseEarlyPlugin.scala

    // Ensure licenses before releasing
    val useBintray = !PrivateKeys.releaseEarlyIsSonatype.value
-    if (useBintray) bintray.BintrayKeys.bintrayEnsureLicenses.value
-    if (hasErrors) sys.error(Feedback.fixRequirementErrors)
+    if (useBintray) bintray.BintrayKeys.bintrayEnsureLicenses


I think this check is unnecessarily duplicated in releaseEarlyPublish:

https://github.com/scalacenter/sbt-release-early/blob/584cf9eae646baf87d9869a7de328f0970eb9a45/src/main/scala/ch/epfl/scala/sbt/release/ReleaseEarlyPlugin.scala#L244

Does it mean that it checks it even for sonatype?

Given that we're already doing the checks in the right place, that line should be deleted 😉

Done in 77b7b70

laughedelic · 2017-10-04T18:29:11Z

src/main/scala/ch/epfl/scala/sbt/release/ReleaseEarlyPlugin.scala

@@ -239,8 +240,8 @@ object ReleaseEarly {
      else if (!Keys.isSnapshot.value && !ThisPluginKeys.releaseEarlyNoGpg.value)
        Pgp.PgpKeys.publishSigned
      // Else, use the normal hijacked bintray publish task
-      else Keys.publish
-    } dependsOn (Bintray.bintrayEnsureLicenses)
+      else (Keys.publish in Bintray.bintray)


I added this explicitly, because if user has a custom publish task definition, it will be used instead of the bintray one.
So here we should run Bintray's publish to have an unambiguous behaviour, while if user still wants to override publishing, they can do it through releaseEarlyPublish task key.

I'm not sure this is what we want. We want to be non-intrusive -- if users have a custom publish, that should be the one we should use. It's the same case with publishSigned, if someone redefines publishSigned we want to execute its task not the original one. Task aggregation is important IMO.

OK, I might be wrong, but my opinion regarding this is based on the experience I got when I tried to use sbt-bintray: I'm also using some company plugin, which sets up publishTo to some other (Amazon S3) resolver. And it's not obvious from the user perspective, which plugin will win. As a user, if I'm adding releaseEarlyWith := BintrayPublisher, I would be surprised, that in the end publish task is overtaken by some other setting. On the other hand, if I really mean to use some custom settings, I still can override releaseEarlyPublish key (which should be then mentioned in docs).

That said, I'm not trying really to convince you to keep this change, just bringing more thoughts to the context..

@laughedelic I still think that relaying on the normal publish task is the way to go. I can imagine lots of scenarios where those settings may be redefined by the user and they expect their definition to be the one executed. We should aim for being as less intrusive and as much intuitive as possible. People are used to redefine publish and publishSigned, let's not complicate their life expecting them to do that redefinition in releaseEarly 😄. I'm adding a commit to this branch that removes this change, no need for you to do it!

Fine with me 👍

jvican · 2017-10-05T09:08:38Z

I'll be looking at the CI error today and reviewing this PR 😄.

jvican · 2017-10-06T11:11:32Z

Was trying to see what the CI error is, but didn't get my head around it yet. Seems to be related to a regression in sbt-sonatype.

jvican · 2017-10-06T11:11:50Z

Or, perhaps, a regression in sbt/sbt. I'll have more news on this today. Sorry for the delay @laughedelic

laughedelic · 2017-10-06T16:41:54Z

OK 👌
Is #12 related to this?

- Removed wildcard import - Removed unnecessary Def.task wrapping

jvican · 2017-10-09T16:14:39Z

@laughedelic I rebased your work, fixed some stuff and added some improvements. CI seems to pass. Go through the diff again and let me know what you think. If you agree, merge it 😉.

jvican · 2017-10-09T16:15:03Z

When this is merged, I'll update docs and cut 2.0.0.

laughedelic · 2017-10-09T16:16:56Z

Great! I'm glad that you fixed it! I'm going to take a look at it tonight.

jvican

LGTM. Awaiting @laughedelic's approval.

laughedelic

LGTM 👌 Feel free to squash my commits if you want.

laughedelic · 2017-10-09T19:20:13Z

src/main/scala/ch/epfl/scala/sbt/release/ReleaseEarlyPlugin.scala

@@ -239,7 +239,7 @@ object ReleaseEarly {
        logger.info(Feedback.logReleaseSonatype(projectName))
        // Trick to make sure that 'sonatypeRelease' does not change the name
        import Sonatype.{sonatypeRelease => _, sonatypeOpen => _}
-        val toRun = s";sonatypeOpen $projectName;publishSigned;sonatypeRelease"
+        val toRun = s";sonatypeOpen $projectName;$projectName/publishSigned;sonatypeRelease"


So this was the fix for that CI failure? Another sbt scoping trap...

Yeah, it was a legit issue... once you go the command way, you need to manually speciify which command you really meant. Glad I found this, it was a real bug.

laughedelic · 2017-10-09T19:24:53Z

src/main/scala/ch/epfl/scala/sbt/release/ReleaseEarlyPlugin.scala

@@ -388,14 +384,13 @@ trait Helper {
    logger.info(Feedback.logCheckRequirements(projectName))

    def check(checks: (Boolean, String)*): Unit = {
-      val errors = checks.collect { case (false, feedback) => logger.error(feedback) }
+      val errors = checks.collect { case (true, feedback) => logger.error(feedback) }


😅 sorry for this.. Did you just notice it yourself or did some test reveal it? I think all tests passed (except the broken one) on the commit introducing this bug.

Don't worry. Yeah, it was weird that they passed. This is one of the reasons why scripted is not ideal to test sbt plugins, it's difficult to know why something succeeded or failed.

laughedelic added the enhancement label Oct 2, 2017

laughedelic self-assigned this Oct 2, 2017

laughedelic requested a review from jvican October 2, 2017 23:09

jvican reviewed Oct 2, 2017

View reviewed changes

laughedelic commented Oct 2, 2017

View reviewed changes

jonas reviewed Oct 3, 2017

View reviewed changes

laughedelic commented Oct 3, 2017

View reviewed changes

laughedelic commented Oct 4, 2017

View reviewed changes

laughedelic requested a review from jvican October 4, 2017 18:34

laughedelic mentioned this pull request Oct 6, 2017

Rescope flags to Global #12

Merged

jvican force-pushed the sbt-1.x branch from 6b67411 to 1258961 Compare October 9, 2017 12:13

jvican and others added 9 commits October 9, 2017 17:03

Fix sonatype releaser

dd51cc2

Trying to update to sbt-1.0.2 and Scala 2.12.3 with minimal changes

01cfb18

Minimal necessary fixes to the test

f6208a1

Updated Scala version in tests

4409bc5

Updated meta-project sbt to 1.0.2

92cecd6

Fixed Exec string representation

8d71217

Minor fixes:

daea8d4

- Removed wildcard import - Removed unnecessary Def.task wrapping

Removed explicit sbt-pgp dependency in tests

c376caa

Restructured checkRequirementsTask

e1079e0

laughedelic and others added 3 commits October 9, 2017 17:03

Removed unnecessary taskDyn wrapping for StableDef.sequential

8de5f0a

Try to identify CI failure

97926b3

Fix small styling issues

d9c1ff4

jvican force-pushed the sbt-1.x branch from 614bcf5 to d9c1ff4 Compare October 9, 2017 15:04

jvican added 5 commits October 9, 2017 17:14

Remove log levels

72f330d

Fix oversight of not using taskDyn

e1db434

Fix error handling for requirements checks

ed08482

Disable damn scaladoc generation

3b6f970

Disable doc and src packaging in tests

958b3ea

jvican approved these changes Oct 9, 2017

View reviewed changes

jvican mentioned this pull request Oct 9, 2017

Allow users to disable instant releases #15

Merged

laughedelic commented Oct 9, 2017

View reviewed changes

jvican merged commit 19ef547 into master Oct 10, 2017

laughedelic deleted the sbt-1.x branch October 10, 2017 14:40

Updating to sbt 1.x #11

Updating to sbt 1.x #11

Conversation

laughedelic commented Oct 2, 2017 • edited Loading

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

laughedelic Oct 2, 2017 • edited Loading

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

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

laughedelic Oct 2, 2017 • edited Loading

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

laughedelic Oct 3, 2017 • edited Loading

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 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican commented Oct 5, 2017

jvican commented Oct 6, 2017

jvican commented Oct 6, 2017

laughedelic commented Oct 6, 2017

jvican commented Oct 9, 2017

jvican commented Oct 9, 2017

laughedelic commented Oct 9, 2017

jvican left a comment

Choose a reason for hiding this comment

laughedelic left a comment

Choose a reason for hiding this comment

laughedelic Oct 9, 2017 • edited Loading

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

laughedelic commented Oct 2, 2017 •

edited

Loading

laughedelic Oct 2, 2017 •

edited

Loading

laughedelic Oct 2, 2017 •

edited

Loading

laughedelic Oct 3, 2017 •

edited

Loading

jvican Oct 9, 2017 •

edited

Loading

laughedelic Oct 9, 2017 •

edited

Loading