Make it easier to test scalafmt local snapshot build #1552

tanishiking · 2019-11-13T17:20:57Z

While I tried to test #1549 locally, I found that scalafmt has mainly two things that make it cumbersome to test locally.

scala build version mismatch

Before this commit, sbt publishLocal publish the build to local ivy with the version something like 2.1.0+xxxx-SNAPSHOT (if people didn't fetch the latest tags to local)

when scalafmt-dynamic tries to download scalafmt-core whose version was 2.1.0+xxxx-SNAPSHOT, it looks for scalafmt built with 2.12 (see:

scalafmt/scalafmt-dynamic/src/main/scala/org/scalafmt/dynamic/ScalafmtDynamicDownloader.scala

Lines 68 to 71 in 778c75f

    
           private def scalaBinaryVersion(version: ScalafmtVersion): String = 
        
             if (version < ScalafmtVersion(0, 7, 0, 0)) "2.11" 
        
             else if (version < ScalafmtVersion(2, 1, 2, 0)) "2.12" 
        
             else "2.13"

)

However, the default scala version is now 2.13 (as of Update Scala to 2.13 #1522) and sbt publishLocal would build scalafmt with 2.13 and publish it as 2.1.0+xxxx-SNAPSHOT.
As a result, scalafmt-dynamic cannot resolve the local build.
- because sbt publishLocal publish 2.13 build with 2.1.0+xxxx-SNAPSHOT to local, but if we specify version=2.1.0+xxxx-SNAPSHOT, scalafmt would look for 2.12 build.

I know we can avoid this situation by building snapshot build with 2.12 instead of 2.13, but it would be handy if we can test the snapshot version only with sbt publishLocal instead of sbt +publishLocal.

Solution

c416668
Introduced localSnapshotVersion for specifying the version in local publishing.
The idea of localSnapshotVersion comes from metals

scalafmt-dynamic doesn't accept SNAPSHOT version

scalafmt-dynamic introduced ScalafmtVersion.scala in #1522. It parses the scalafmt version specified in .scalafmt.conf, and if the version string is invalid, it rejects to resolve with an error.

For example, if .scalafmt.conf contains version=2.2.3-SNAPSHOT for testing local build, scalafmt-dynamic parse 2.2.3-SNAPSHOT as invalid version.

Solution

So with that, I tweaked the regex for scalafmtVersion 67f6f0a so that scalafmt-dynamic can parse snapshot version as a valid scalafmtVersion, and we can test the local builds.

- Before this commit, `sbt publishLocal` publish the build to local ivy with the version something like `1.6.0-RC4+blahblah+yyyymmdd-SNAPSHOT` - when scalafmt-dynamic tries to download scalafmt-core whose version is `1.6.0-xxx-SNAPSHOT`, it looks for scalafmt built with 2.12 - However, the default scala version is now 2.13 and `sbt publishLocal` will build scalafmt with 2.13 and publish it with `1.6.0-xxx-SNAPSHOT`. - As a result, scalafmt-dynamic cannot resolve the local build. I know we can avoid this problem(?) by building snapshot build with 2.12 instead of 2.13, but it would be handy if we can test the snapshot version only with `sbt publishLocal` instead of `sbt +publishLocal`. The idea of localSnapshotVersion is come from [metals](https://github.com/scalameta/metals/blob/05017dc44e384ab8e0a8202ebd128f697594d686/build.sbt#L1-L19)

tanishiking · 2019-11-13T17:21:42Z

build.sbt

@@ -1,12 +1,18 @@
 import Dependencies._
 import sbtcrossproject.CrossPlugin.autoImport.crossProject
+def localSnapshotVersion = "2.2.3-SNAPSHOT"
+def isCI = System.getenv("CI") != null


It will be true in TravisCI https://docs.travis-ci.com/user/environment-variables/#default-environment-variables

olafurpg

LGTM 👍

Would it make sense for scalafmt-dynamic to default to 2.13 for the latest Scalafmt versions?

olafurpg · 2019-11-13T18:58:14Z

build.sbt

@@ -1,12 +1,18 @@
 import Dependencies._
 import sbtcrossproject.CrossPlugin.autoImport.crossProject
+def localSnapshotVersion = "2.2.3-SNAPSHOT"


I am a bit concerned that this version needs to be bumped up every time we cut a new release. It would be nice to avoid that.

We can parse it from git tags

def parseTagVersion: String = { import scala.sys.process._ "git describe --abbrev=0 --tags".!!.drop(1).strip } def localSnapshotVersion: String = s"$parseTagVersion-SNAPSHOT"

Ah, sorry I just realized that it(my local published build with 2.1.0+xxx-SNAPSHOT) happened just because I haven't fetched tags from a remote repository for a while... oh my ignorance...

I am a bit concerned that this version needs to be bumped up every time we cut a new release. It would be nice to avoid that.

Now, I agree with that. 👍

We can parse it from git tags

Awesome! With this localSnapshotVersion, we can simplify the regex in ScalafmtDynamicDownloader 👍

May be document here that it is recommended to do git fetch before local building?

It might be helpful though it won't be a problem for most people who are new to this project :)
BTW, since that documentation is becoming older and things have changed a bit (like release process, document locations), we have to update it in another PR.

poslegm · 2019-11-14T07:25:52Z

scalafmt-dynamic/src/test/scala/tests/ScalafmtVersionSuite.scala

@@ -51,6 +55,11 @@ class ScalafmtVersionSuite extends FunSuite {
      ScalafmtVersion
        .parse("2.1.0-RC1-M4") == Left(InvalidVersionException("2.1.0-RC1-M4"))
    )
+    assert(
+      ScalafmtVersion.parse("2.0.0-RC1-SNAPSHOT") == Left(
+        InvalidVersionException("2.0.0-RC1-SNAPSHOT")


Why is it invalid?

Oh, sorry, it's my mistake, it should be valid.

poslegm · 2019-11-14T07:44:27Z

build.sbt

@@ -1,12 +1,18 @@
 import Dependencies._
 import sbtcrossproject.CrossPlugin.autoImport.crossProject
+def localSnapshotVersion = "2.2.3-SNAPSHOT"


We can parse it from git tags

def parseTagVersion: String = { import scala.sys.process._ "git describe --abbrev=0 --tags".!!.drop(1).strip } def localSnapshotVersion: String = s"$parseTagVersion-SNAPSHOT"

poslegm · 2019-11-17T15:00:57Z

@tanishiking is there another changes you want to do or we can merge it now?

tanishiking · 2019-11-17T15:22:56Z

Thanks!

@tanishiking is there another changes you want to do or we can merge it now?

That's all, we can merge it now

tanishiking commented Nov 13, 2019

View reviewed changes

olafurpg approved these changes Nov 13, 2019

View reviewed changes

poslegm reviewed Nov 14, 2019

View reviewed changes

tanishiking added 2 commits November 14, 2019 22:02

Enable scalafmt-dynamic to parse snapshot versions

1ac6359

Retrieve local snapshot version from git

f14229d

tanishiking force-pushed the refactor-for-local branch from 67f6f0a to f14229d Compare November 14, 2019 13:03

poslegm approved these changes Nov 16, 2019

View reviewed changes

tanishiking merged commit 488ddf2 into scalameta:master Nov 17, 2019

tanishiking deleted the refactor-for-local branch November 17, 2019 15:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make it easier to test scalafmt local snapshot build #1552

Make it easier to test scalafmt local snapshot build #1552

tanishiking commented Nov 13, 2019 •

edited

Loading

tanishiking Nov 13, 2019

olafurpg left a comment

olafurpg Nov 13, 2019

poslegm Nov 14, 2019

tanishiking Nov 14, 2019

poslegm Nov 14, 2019

tanishiking Nov 16, 2019

poslegm Nov 14, 2019

tanishiking Nov 14, 2019

poslegm Nov 14, 2019

poslegm commented Nov 17, 2019

tanishiking commented Nov 17, 2019

	private def scalaBinaryVersion(version: ScalafmtVersion): String =
	if (version < ScalafmtVersion(0, 7, 0, 0)) "2.11"
	else if (version < ScalafmtVersion(2, 1, 2, 0)) "2.12"
	else "2.13"

Make it easier to test scalafmt local snapshot build #1552

Make it easier to test scalafmt local snapshot build #1552

Conversation

tanishiking commented Nov 13, 2019 • edited Loading

scala build version mismatch

Solution

scalafmt-dynamic doesn't accept SNAPSHOT version

Solution

Choose a reason for hiding this comment

olafurpg 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

poslegm commented Nov 17, 2019

tanishiking commented Nov 17, 2019

tanishiking commented Nov 13, 2019 •

edited

Loading