Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to propose sbt version update #665

Merged
merged 10 commits into from
Jul 8, 2019

Conversation

exoego
Copy link
Contributor

@exoego exoego commented Jul 6, 2019

Closes #101.
Steward will propose update if they found sbt.version=X.Y.Z-SOMETHING.
It does not update SbtVersion that used for conditional build like below

sbtSeries.value match {
    case "0.13" => SbtVersion("0.13.17")
    case "1.0"  => defaultSbtVersion
    case _      => defaultSbtVersion
}

since such build requires cautious update.

I could not find how to perform system-test against actual repos...
How should I perform ?

@exoego exoego changed the title Add ability to update sbt version Add ability to propose sbt version update Jul 6, 2019
@codecov
Copy link

codecov bot commented Jul 6, 2019

Codecov Report

Merging #665 into master will increase coverage by 0.26%.
The diff coverage is 78.26%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #665      +/-   ##
==========================================
+ Coverage   60.26%   60.52%   +0.26%     
==========================================
  Files          78       78              
  Lines        1042     1059      +17     
  Branches       21       20       -1     
==========================================
+ Hits          628      641      +13     
- Misses        414      418       +4
Impacted Files Coverage Δ
.../main/scala/org/scalasteward/core/io/FileAlg.scala 88% <ø> (-0.47%) ⬇️
...ain/scala/org/scalasteward/core/edit/EditAlg.scala 92.85% <100%> (+1.19%) ⬆️
.../main/scala/org/scalasteward/core/io/package.scala 100% <100%> (ø) ⬆️
...la/org/scalasteward/core/sbt/data/SbtVersion.scala 100% <100%> (ø) ⬆️
.../main/scala/org/scalasteward/core/sbt/SbtAlg.scala 62.26% <70%> (+0.04%) ⬆️
...main/scala/org/scalasteward/core/sbt/package.scala 68.75% <71.42%> (+8.75%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7d1971...94c1e18. Read the comment docs.

@exoego exoego force-pushed the update-sbt-version branch 2 times, most recently from ff33f5e to 0ce2cd7 Compare July 6, 2019 09:07
@fthomas
Copy link
Member

fthomas commented Jul 6, 2019

Thanks @exoego for working on this. I think a few more things are needed for this feature:

  1. isSourceFile needs to be updated to return true for project/build.properties files.
  2. SbtAlg.getUpdatesForRepo needs to include Update.Single for sbt like you've used in the test (Update.Single("org.scala-sbt", "sbt", "1.2.7", Nel.of("1.2.8")). sbt-updates doesn't include updates for sbt, so we have to implement this ourself. In the first iteration of this feature I would propose to use sbt.defaultSbtVersion as latest for the 1.x series and add a new sbt.latestSbtVersion_0_13 for the 0.13.x series. The sbt version of a Repo can then be queried via sbt or by reading project/build.properties (which I'd prefer because it is faster.) With this information you can now create an Update.Single and include it in getUpdatesForRepo.

I'm not sure if we then need a new UpdateHeuristic. Maybe the heuristics we already have are good enough. And to test it I would run Scala Steward with a test repo.

@exoego exoego force-pushed the update-sbt-version branch from 0ce2cd7 to 7676425 Compare July 7, 2019 01:29
@exoego
Copy link
Contributor Author

exoego commented Jul 7, 2019

Updated SbtAlg.getUpdatesForRepo and reverted UpdateHeuristic in 7676425

@exoego exoego force-pushed the update-sbt-version branch from 7676425 to a45d616 Compare July 7, 2019 07:06
@fthomas
Copy link
Member

fthomas commented Jul 7, 2019

I just tested your branch with one of my repos and it works: fthomas/properly#26
This is great!

I also pushed two minor fixes.

@fthomas
Copy link
Member

fthomas commented Jul 7, 2019

I tried this on a few more repos:

And they all look good.

The only one that also updated some unrelated version number was this:

I'm not sure yet if we should restrict sbt updates to build.properties files.

@exoego
Copy link
Contributor Author

exoego commented Jul 7, 2019

It looks like UpdateHeuristic.original is used for sbt update.
Since UpdateHeuristic.strict requires groupId in the line, but build.properties dont' have groupId,
so UpdateHeuristicoriginal with a Regex like (?i)(.*)((sbt).*?)\Q1.2.8\E(.?) is used.
Such regex can matches addSbtPlugin("com.jsuereth" % "sbt-pgp" % "1.2.8") too.

I'm not sure yet if we should restrict sbt updates to build.properties files.

I think restriction is needed.
Where is a good place for such file restriction ?
It seems that FileAlg should not care about which UpdateHeuristic is used.
So I assume file restriction should be added to like Update classes like

// Update.Single
final case class Single(
      groupId: String,
      artifactId: String,
      currentVersion: String,
      newerVersions: Nel[String],
      configurations: Option[String] = None,
      fileReestrictions: Option[Regex]  = None
  ) extends Update {

// SbtAlg
Update.Single("org.scala-sbt", "sbt", currentVer, Nel.of(newVer),
    Some("build.properties".r))

// EditAlg
def applyUpdateTo[G[_]: Traverse](files: G[File], update: Update): F[Unit] = {
  val actions = UpdateHeuristic.all.map { heuristic =>
    logger.info(s"Trying heuristic '${heuristic.name}'") >>
       // RESTRICTION HERE 
      fileAlg.editFiles(files, heuristic.replaceF(update))
  }
  bindUntilTrue(actions).void
}

@fthomas
Copy link
Member

fthomas commented Jul 7, 2019

The file restriction would be computed in EditAlg and then passed to FileAlg.findSourceFilesContaining instead of using the static isSourceFile.

This would restrict sbt updates to only edit project/build.properties files and nothing else. I'm trying to think of situations where the sbt version might be used somewhere else which we would then miss.

@fthomas fthomas added the enhancement New feature or request label Jul 7, 2019
@fthomas fthomas added this to the 0.3.0 milestone Jul 7, 2019
@exoego
Copy link
Contributor Author

exoego commented Jul 8, 2019

The file restriction would be computed in EditAlg and then passed to FileAlg.findSourceFilesContaining instead of using the static isSourceFile.

Added such dynamic Update-based file filter in 94c1e18

I'm trying to think of situations where the sbt version might be used somewhere else which we would then miss.

To use sbt version somewhere in build, I think users should use the sbt key sbtVersion instead of literal.
So I assume it is a trivial limitation if we apply sbt update only to build.properties in 1st iteration.

Copy link
Member

@fthomas fthomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks again, @exoego!

@fthomas fthomas merged commit 0e8bcca into scala-steward-org:master Jul 8, 2019
@exoego exoego deleted the update-sbt-version branch July 8, 2019 10:44
@kubukoz
Copy link
Contributor

kubukoz commented Jul 8, 2019

kubukoz/sup#84 it works, thanks for this update!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Propose sbt updates
3 participants