scala-steward icon indicating copy to clipboard operation
scala-steward copied to clipboard

Published 20 hours ago • verified publisher icon scala-steward-org

Misleading "Unable to bump version" is logged when the version was already bumped

Open alexklibisz opened this issue 2 years ago • 10 comments

When there are two deps both referencing the same val for their versions, the second update will print a misleading warning. I would expect that there is no warning, since the second update was, practically speaking, applied.

I'll create a PR to reproduce this.

alexklibisz avatar Jan 10 '23 00:01 alexklibisz

I looked into this a bit more today, running scala-steward on a private repo. I noticed the following interesting behavior:

I created an empty project with a build.sbt like this:

ThisBuild / version := "0.1.0-SNAPSHOT"

ThisBuild / scalaVersion := "2.13.10"

lazy val circeVersion = "0.13.0"

lazy val root = (project in file("."))
  .settings(
    name := "scala-steward-2906",
    libraryDependencies ++= Seq(
      "io.circe" %% "circe-core" % circeVersion,
      "io.circe" %% "circe-parser" % circeVersion
    )
  )

When I run scala-steward with no .scala-steward.conf, it correctly updates the circeVersion and does not print a warning.

Then I added a .scala-steward.conf to group the updates:

pullRequests.grouping = [
  { name = "circe", "title" = "Circe updates", "filter" = [{"group" = "io.circe"}] },
]

In this case, it still correctly updates the version, but it does print the warning:

2023-02-12 08:37:14,230 INFO  Found 1 update:
  circe (group) {
    io.circe:(circe-core, circe-core_2.13) : 0.13.0 -> 0.14.4
    io.circe:(circe-parser, circe-parser_2.13) : 0.13.0 -> 0.14.4
  }
...
2023-02-12 08:37:15,043 WARN  Unable to bump version for update io.circe:(circe-parser, circe-parser_2.13) : 0.13.0 -> 0.14.4

I'll do some more debugging and see if I can make a PR to resolve this.

alexklibisz avatar Feb 12 '23 16:02 alexklibisz

I was able to replicate this behavior in a "real" project. For now I think I'm just going to avoid the pullRequests.grouping configuration.

alexklibisz avatar Feb 12 '23 17:02 alexklibisz

I added some println debugging in my fake circe project, on this commit: https://github.com/scala-steward-org/scala-steward/pull/2907/commits/39b618f79d1c850baba647018614f67b38674a8f. Specifically in the findUpdateReplacements and applyUpdateReplacements methods in EditAlg. Below are the respective logs. I pulled out the parts that looked relevant, but also attached the full log files (incl. many debug lines).

With no grouping and no warning:

2023-02-12 09:10:40,320 INFO  Found 2 updates:
  io.circe:(circe-core, circe-core_2.13) : 0.13.0 -> 0.14.4
  io.circe:(circe-parser, circe-parser_2.13) : 0.13.0 -> 0.14.4
2023-02-12 09:10:40,325 INFO  alexklibisz/scala-steward-2906 is outdated:
  new version: io.circe:(circe-core, circe-core_2.13) : 0.13.0 -> 0.14.4
  new version: io.circe:(circe-parser, circe-parser_2.13) : 0.13.0 -> 0.14.4
2023-02-12 09:10:40,326 INFO  Nurture alexklibisz/scala-steward-2906
2023-02-12 09:10:40,329 INFO  Found 1 update:
  io.circe:{(circe-core, circe-core_2.13), (circe-parser, circe-parser_2.13)} : 0.13.0 -> 0.14.4
2023-02-12 09:10:40,356 INFO  Process update io.circe:{(circe-core, circe-core_2.13), (circe-parser, circe-parser_2.13)} : 0.13.0 -> 0.14.4
findUpdateReplacements
  update = ForGroupId(NonEmptyList(ForArtifactId(CrossDependency(NonEmptyList(Dependency(io.circe,ArtifactId(circe-core,Some(circe-core_2.13)),0.13.0,None,None,None))),NonEmptyList(0.14.4),None,None), ForArtifactId(CrossDependency(NonEmptyList(Dependency(io.circe,ArtifactId(circe-parser,Some(circe-parser_2.13)),0.13.0,None,None,None))),NonEmptyList(0.14.4),None,None)))
  versionPositions = List(ScalaVal(Position(build.sbt,105,0.13.0),lazy ,circeVersion))
  modulePositions = List(ModulePosition(Position(build.sbt,240,io.circe),Position(build.sbt,254,circe-core),Position(build.sbt,268,circeVersion,)), ModulePosition(Position(build.sbt,289,io.circe),Position(build.sbt,303,circe-parser),Position(build.sbt,319,circeVersion)))
2023-02-12 09:10:40,908 INFO  Create branch update/circe-core-0.14.4
applyUpdateReplacements
  data = ...
  update = ForGroupId(NonEmptyList(ForArtifactId(CrossDependency(NonEmptyList(Dependency(io.circe,ArtifactId(circe-core,Some(circe-core_2.13)),0.13.0,None,None,None))),NonEmptyList(0.14.4),None,None), ForArtifactId(CrossDependency(NonEmptyList(Dependency(io.circe,ArtifactId(circe-parser,Some(circe-parser_2.13)),0.13.0,None,None,None))),NonEmptyList(0.14.4),None,None)))
  updateReplacements = List(Replacement(Position(build.sbt,105,0.13.0),0.14.4))
2023-02-12 09:10:41,021 INFO  Push 1 commit(s)
2023-02-12 09:10:41,999 INFO  Create PR update/circe-core-0.14.4
2023-02-12 09:10:50,348 INFO  Created PR https://github.com/alexklibisz/scala-steward-2906/pull/5
2023-02-12 09:10:50,446 INFO  ──────────── Total time: Steward alexklibisz/scala-steward-2906: 23s 158ms ────────────
2023-02-12 09:10:50,447 INFO  ──────────── Total time: run: 23s 524ms ────────────

Full logs: no-warning.log

With grouping and with warning:

2023-02-12 08:59:30,741 INFO  Found 2 updates:
  io.circe:(circe-core, circe-core_2.13) : 0.13.0 -> 0.14.4
  io.circe:(circe-parser, circe-parser_2.13) : 0.13.0 -> 0.14.4
2023-02-12 08:59:30,745 INFO  alexklibisz/scala-steward-2906 is outdated:
  new version: io.circe:(circe-core, circe-core_2.13) : 0.13.0 -> 0.14.4
  new version: io.circe:(circe-parser, circe-parser_2.13) : 0.13.0 -> 0.14.4
2023-02-12 08:59:30,746 INFO  Nurture alexklibisz/scala-steward-2906
2023-02-12 08:59:30,753 INFO  Found 1 update:
  circe (group) {
    io.circe:(circe-core, circe-core_2.13) : 0.13.0 -> 0.14.4
    io.circe:(circe-parser, circe-parser_2.13) : 0.13.0 -> 0.14.4
  }
2023-02-12 08:59:30,771 INFO  Process update circeEAD
2023-02-12 08:59:31,016 INFO  Create branch update/circe
findUpdateReplacements
  update = ForArtifactId(CrossDependency(NonEmptyList(Dependency(io.circe,ArtifactId(circe-core,Some(circe-core_2.13)),0.13.0,None,None,None))),NonEmptyList(0.14.4),None,None)
  versionPositions = List(ScalaVal(Position(build.sbt,105,0.13.0),lazy ,circeVersion))
  modulePositions = List(ModulePosition(Position(build.sbt,240,io.circe),Position(build.sbt,254,circe-core),Position(build.sbt,268,circeVersion,)))
applyUpdateReplacements
  data = ...
  update = ForArtifactId(CrossDependency(NonEmptyList(Dependency(io.circe,ArtifactId(circe-core,Some(circe-core_2.13)),0.13.0,None,None,None))),NonEmptyList(0.14.4),None,None)
  updateReplacements = List(Replacement(Position(build.sbt,105,0.13.0),0.14.4))
findUpdateReplacements
  update = ForArtifactId(CrossDependency(NonEmptyList(Dependency(io.circe,ArtifactId(circe-parser,Some(circe-parser_2.13)),0.13.0,None,None,None))),NonEmptyList(0.14.4),None,None)
  versionPositions = List()
  modulePositions = List(ModulePosition(Position(build.sbt,289,io.circe),Position(build.sbt,303,circe-parser),Position(build.sbt,319,circeVersion)))
2023-02-12 08:59:31,493 WARN  Unable to bump version for update io.circe:(circe-parser, circe-parser_2.13) : 0.13.0 -> 0.14.4date/circe --
2023-02-12 08:59:31,508 INFO  Push 1 commit(s)eam origin update/circe
2023-02-12 08:59:32,694 INFO  Create PR update/circe--files-with-matches 0.13.0--files-with-matches 0.13.0
2023-02-12 08:59:38,993 INFO  Created PR https://github.com/alexklibisz/scala-steward-2906/pull/4
2023-02-12 08:59:39,087 INFO  ──────────── Total time: Steward alexklibisz/scala-steward-2906: 21s 638ms ────────────
2023-02-12 08:59:39,088 INFO  ──────────── Total time: run: 22s 13ms ────────────

Full logs: warning.log

alexklibisz avatar Feb 12 '23 17:02 alexklibisz

The main difference I see is in the update parameter passed to findUpdateReplacements:

  • without grouping: a single call o findUpdateReplacement w/ update = ForGroupId(NonEmptyList(ForArtifactId(CrossDependency(NonEmptyList(Dependency(io.circe,ArtifactId(circe-core,Some(circe-core_2.13)),0.13.0,None,None,None))),NonEmptyList(0.14.4),None,None), ForArtifactId(CrossDependency(NonEmptyList(Dependency(io.circe,ArtifactId(circe-parser,Some(circe-parser_2.13)),0.13.0,None,None,None))),NonEmptyList(0.14.4),None,None)))
  • with grouping: two calls to findUpdateReplacement w/ update = ForArtifactId(CrossDependency(NonEmptyList(Dependency(io.circe,ArtifactId(circe-core,Some(circe-core_2.13)),0.13.0,None,None,None))),NonEmptyList(0.14.4),None,None) and update = ForArtifactId(CrossDependency(NonEmptyList(Dependency(io.circe,ArtifactId(circe-parser,Some(circe-parser_2.13)),0.13.0,None,None,None))),NonEmptyList(0.14.4),None,None)

alexklibisz avatar Feb 12 '23 17:02 alexklibisz

So there might be two questions here:

  1. Why does presence or absence of grouping produce different updates for the same dependency?
  2. Why does a ForArtifactId(CrossDependency(...)) update produce this warning?

alexklibisz avatar Feb 12 '23 17:02 alexklibisz

One last though, and then I have to wrap up my investigation soon:

I added another println to applyNewUpdate in NurtureAlg, to print the data.update parameter.

  • Without grouping: data.update = ForGroupId(NonEmptyList(ForArtifactId(CrossDependency(NonEmptyList(Dependency(io.circe,ArtifactId(circe-core,Some(circe-core_2.13)),0.13.0,None,None,None))),NonEmptyList(0.14.4),None,None), ForArtifactId(CrossDependency(NonEmptyList(Dependency(io.circe,ArtifactId(circe-parser,Some(circe-parser_2.13)),0.13.0,None,None,None))),NonEmptyList(0.14.4),None,None)))
  • With grouping: data.update = Grouped(circe,Some(Circe updates),List(ForArtifactId(CrossDependency(NonEmptyList(Dependency(io.circe,ArtifactId(circe-core,Some(circe-core_2.13)),0.13.0,None,None,None))),NonEmptyList(0.14.4),None,None), ForArtifactId(CrossDependency(NonEmptyList(Dependency(io.circe,ArtifactId(circe-parser,Some(circe-parser_2.13)),0.13.0,None,None,None))),NonEmptyList(0.14.4),None,None)))

So, in this case, the Grouped update and the ForGroupId update are actually equivalent, but they are treated differently based on the grouping. That difference seems to originate in Update.groupByPullRequestGroup. Maybe we can update that method to identify cases like this one, and return a ForGroupId instead of a Grouped?

alexklibisz avatar Feb 12 '23 18:02 alexklibisz

So the reason this happens is because Grouped updates work with single ForArtifactId updates. So when two update dependencies use the same val for the version, the first one updates the val and the second one doesn't do anything and logs the warning. There is no point on making individual updates aware of the changes their predecessor updates have made. The reason why this was made this way is because there is no reason to have both ForArtifactId and ForGroupId inside Grouped updates since the latter were created to "join" together updates for the same groupId so it would only create one PR.

alejandrohdezma avatar Feb 13 '23 16:02 alejandrohdezma

So, in this case, the Grouped update and the ForGroupId update are actually equivalent, but they are treated differently based on the grouping. That difference seems to originate in Update.groupByPullRequestGroup. Maybe we can update that method to identify cases like this one, and return a ForGroupId instead of a Grouped?

Also for this, if we generate a ForGroupId instead of Grouped we are not doing what the user has stated, which could lead to automations not working since the PR created would not respect the branch/PR-title set in the grouping setting.

alejandrohdezma avatar Feb 13 '23 16:02 alejandrohdezma

Thanks for the insight. I'll have to re-read a few times to make sure I follow. In the meantime, do you see any way to avoid the incorrect warnings?

alexklibisz avatar Feb 13 '23 16:02 alexklibisz

🤔 Nothing that would require refactors here and there so Grouped supported both ForArtifactId and ForGroupId. Or maybe just remove the log or change it to be debug? cc @exoego

alejandrohdezma avatar Feb 13 '23 16:02 alejandrohdezma