Skip to content

Commit

Permalink
Shorten Dep serialization format in common cases (#3490)
Browse files Browse the repository at this point in the history 
Fixes #3417

We do a best-effort approach where we try to reverse the `Dep.parse`
logic, and at the end do a `==` check to see if the round tripped `Dep`
is identical to the original. There's some overhead with the round trip
comparison - effectively for each write-read of a `Dep` we add an
additional `read` - but this isn't a hot code path so it shouldn't be a
big deal. In exchange we get a pretty good guarantee of correctness that
later on when someone round-trips the same `Dep` off of disk, they get
the same result. We do the same for `BoundDep` and re-use most of the
`Dep` logic

The sophistication of our `def parse` and `def unparse` logic can be
extended over time, but this should work well for 99% of dependencies
which are simple without fancy exclusions, publication, configuration,
optional, transitive, etc.

Added some assertions to `ResolveDepsTests` to check the round tripping
for common scenarios, both simplified and not

Before:

```scala
lihaoyi mill$ ./mill show main.api.ivyDeps
[
  {
    "dep": {
      "module": {
        "organization": {
          "value": "com.lihaoyi"
        },
        "name": {
          "value": "os-lib"
        },
        "attributes": {}
      },
      "version": "0.10.7-M1",
      "configuration": {
        "value": "default(compile)"
      },
      "minimizedExclusions": {
        "data": "coursier.core.MinimizedExclusions.ExcludeNone"
      },
      "publication": {
        "name": "",
        "type": {
          "value": ""
        },
        "ext": {
          "value": ""
        },
        "classifier": {
          "value": ""
        }
      },
      "optional": false,
      "transitive": true
    },
    "cross": {
      "$type": "mill.scalalib.CrossVersion.Binary",
      "platformed": false
    },
    "force": false
  },
  {
    "dep": {
      "module": {
        "organization": {
          "value": "com.lihaoyi"
        },
        "name": {
          "value": "upickle"
        },
        "attributes": {}
      },
      "version": "3.3.1",
      "configuration": {
        "value": "default(compile)"
      },
      "minimizedExclusions": {
        "data": "coursier.core.MinimizedExclusions.ExcludeNone"
      },
      "publication": {
        "name": "",
        "type": {
          "value": ""
        },
        "ext": {
          "value": ""
        },
        "classifier": {
          "value": ""
        }
      },
      "optional": false,
      "transitive": true
    },
    "cross": {
      "$type": "mill.scalalib.CrossVersion.Binary",
      "platformed": false
    },
    "force": false
  },
  {
    "dep": {
      "module": {
        "organization": {
          "value": "com.lihaoyi"
        },
        "name": {
          "value": "pprint"
        },
        "attributes": {}
      },
      "version": "0.9.0",
      "configuration": {
        "value": "default(compile)"
      },
      "minimizedExclusions": {
        "data": "coursier.core.MinimizedExclusions.ExcludeNone"
      },
      "publication": {
        "name": "",
        "type": {
          "value": ""
        },
        "ext": {
          "value": ""
        },
        "classifier": {
          "value": ""
        }
      },
      "optional": false,
      "transitive": true
    },
    "cross": {
      "$type": "mill.scalalib.CrossVersion.Binary",
      "platformed": false
    },
    "force": false
  },
  {
    "dep": {
      "module": {
        "organization": {
          "value": "com.lihaoyi"
        },
        "name": {
          "value": "fansi"
        },
        "attributes": {}
      },
      "version": "0.5.0",
      "configuration": {
        "value": "default(compile)"
      },
      "minimizedExclusions": {
        "data": "coursier.core.MinimizedExclusions.ExcludeNone"
      },
      "publication": {
        "name": "",
        "type": {
          "value": ""
        },
        "ext": {
          "value": ""
        },
        "classifier": {
          "value": ""
        }
      },
      "optional": false,
      "transitive": true
    },
    "cross": {
      "$type": "mill.scalalib.CrossVersion.Binary",
      "platformed": false
    },
    "force": false
  },
  {
    "dep": {
      "module": {
        "organization": {
          "value": "org.scala-sbt"
        },
        "name": {
          "value": "test-interface"
        },
        "attributes": {}
      },
      "version": "1.0",
      "configuration": {
        "value": "default(compile)"
      },
      "minimizedExclusions": {
        "data": "coursier.core.MinimizedExclusions.ExcludeNone"
      },
      "publication": {
        "name": "",
        "type": {
          "value": ""
        },
        "ext": {
          "value": ""
        },
        "classifier": {
          "value": ""
        }
      },
      "optional": false,
      "transitive": true
    },
    "cross": {
      "$type": "mill.scalalib.CrossVersion.Constant",
      "value": "",
      "platformed": false
    },
    "force": false
  }
]
lihaoyi mill$ ./mill show main.api.transitiveIvyDeps
[
  {
    "dep": {
      "module": {
        "organization": {
          "value": "com.lihaoyi"
        },
        "name": {
          "value": "os-lib_2.13"
        },
        "attributes": {}
      },
      "version": "0.10.7-M1",
      "configuration": {
        "value": "default(compile)"
      },
      "minimizedExclusions": {
        "data": "coursier.core.MinimizedExclusions.ExcludeNone"
      },
      "publication": {
        "name": "",
        "type": {
          "value": ""
        },
        "ext": {
          "value": ""
        },
        "classifier": {
          "value": ""
        }
      },
      "optional": false,
      "transitive": true
    },
    "force": false
  },
  {
    "dep": {
      "module": {
        "organization": {
          "value": "com.lihaoyi"
        },
        "name": {
          "value": "upickle_2.13"
        },
        "attributes": {}
      },
      "version": "3.3.1",
      "configuration": {
        "value": "default(compile)"
      },
      "minimizedExclusions": {
        "data": "coursier.core.MinimizedExclusions.ExcludeNone"
      },
      "publication": {
        "name": "",
        "type": {
          "value": ""
        },
        "ext": {
          "value": ""
        },
        "classifier": {
          "value": ""
        }
      },
      "optional": false,
      "transitive": true
    },
    "force": false
  },
  {
    "dep": {
      "module": {
        "organization": {
          "value": "com.lihaoyi"
        },
        "name": {
          "value": "pprint_2.13"
        },
        "attributes": {}
      },
      "version": "0.9.0",
      "configuration": {
        "value": "default(compile)"
      },
      "minimizedExclusions": {
        "data": "coursier.core.MinimizedExclusions.ExcludeNone"
      },
      "publication": {
        "name": "",
        "type": {
          "value": ""
        },
        "ext": {
          "value": ""
        },
        "classifier": {
          "value": ""
        }
      },
      "optional": false,
      "transitive": true
    },
    "force": false
  },
  {
    "dep": {
      "module": {
        "organization": {
          "value": "com.lihaoyi"
        },
        "name": {
          "value": "fansi_2.13"
        },
        "attributes": {}
      },
      "version": "0.5.0",
      "configuration": {
        "value": "default(compile)"
      },
      "minimizedExclusions": {
        "data": "coursier.core.MinimizedExclusions.ExcludeNone"
      },
      "publication": {
        "name": "",
        "type": {
          "value": ""
        },
        "ext": {
          "value": ""
        },
        "classifier": {
          "value": ""
        }
      },
      "optional": false,
      "transitive": true
    },
    "force": false
  },
  {
    "dep": {
      "module": {
        "organization": {
          "value": "org.scala-sbt"
        },
        "name": {
          "value": "test-interface"
        },
        "attributes": {}
      },
      "version": "1.0",
      "configuration": {
        "value": "default(compile)"
      },
      "minimizedExclusions": {
        "data": "coursier.core.MinimizedExclusions.ExcludeNone"
      },
      "publication": {
        "name": "",
        "type": {
          "value": ""
        },
        "ext": {
          "value": ""
        },
        "classifier": {
          "value": ""
        }
      },
      "optional": false,
      "transitive": true
    },
    "force": false
  },
  {
    "dep": {
      "module": {
        "organization": {
          "value": "org.scala-lang"
        },
        "name": {
          "value": "scala-library"
        },
        "attributes": {}
      },
      "version": "2.13.14",
      "configuration": {
        "value": "default(compile)"
      },
      "minimizedExclusions": {
        "data": "coursier.core.MinimizedExclusions.ExcludeNone"
      },
      "publication": {
        "name": "",
        "type": {
          "value": ""
        },
        "ext": {
          "value": ""
        },
        "classifier": {
          "value": ""
        }
      },
      "optional": false,
      "transitive": true
    },
    "force": true
  },
  {
    "dep": {
      "module": {
        "organization": {
          "value": "com.lihaoyi"
        },
        "name": {
          "value": "mill-moduledefs_2.13"
        },
        "attributes": {}
      },
      "version": "0.11.0-M2",
      "configuration": {
        "value": "default(compile)"
      },
      "minimizedExclusions": {
        "data": "coursier.core.MinimizedExclusions.ExcludeNone"
      },
      "publication": {
        "name": "",
        "type": {
          "value": ""
        },
        "ext": {
          "value": ""
        },
        "classifier": {
          "value": ""
        }
      },
      "optional": false,
      "transitive": true
    },
    "force": false
  },
  {
    "dep": {
      "module": {
        "organization": {
          "value": "com.kohlschutter.junixsocket"
        },
        "name": {
          "value": "junixsocket-core"
        },
        "attributes": {}
      },
      "version": "2.10.0",
      "configuration": {
        "value": "default(compile)"
      },
      "minimizedExclusions": {
        "data": "coursier.core.MinimizedExclusions.ExcludeNone"
      },
      "publication": {
        "name": "",
        "type": {
          "value": ""
        },
        "ext": {
          "value": ""
        },
        "classifier": {
          "value": ""
        }
      },
      "optional": false,
      "transitive": true
    },
    "force": false
  }
]
```

After:

```scala
lihaoyi mill$ ./mill show main.api.ivyDeps
[
  "com.lihaoyi::os-lib:0.10.7-M1",
  "com.lihaoyi::upickle:3.3.1",
  "com.lihaoyi::pprint:0.9.0",
  "com.lihaoyi::fansi:0.5.0",
  "org.scala-sbt:test-interface:1.0"
]

lihaoyi mill$ ./mill show main.api.transitiveIvyDeps
[
  "com.lihaoyi:os-lib_2.13:0.10.7-M1",
  "com.lihaoyi:upickle_2.13:3.3.1",
  "com.lihaoyi:pprint_2.13:0.9.0",
  "com.lihaoyi:fansi_2.13:0.5.0",
  "org.scala-sbt:test-interface:1.0",
  {
    "dep": {
      "module": {
        "organization": {
          "value": "org.scala-lang"
        },
        "name": {
          "value": "scala-library"
        },
        "attributes": {}
      },
      "version": "2.13.14",
      "configuration": {
        "value": "default(compile)"
      },
      "minimizedExclusions": {
        "data": "coursier.core.MinimizedExclusions.ExcludeNone"
      },
      "publication": {
        "name": "",
        "type": {
          "value": ""
        },
        "ext": {
          "value": ""
        },
        "classifier": {
          "value": ""
        }
      },
      "optional": false,
      "transitive": true
    },
    "force": true
  },
  "com.lihaoyi:mill-moduledefs_2.13:0.11.0-M2",
  "com.kohlschutter.junixsocket:junixsocket-core:2.10.0"
]
```

Note that `org.scala-lang:scala-library` is still using the old verbose
serialization, due to `.forceVersion()` which makes it unable to be
parsed/unparsed into a naked string literal as it needs a
`.forceVersion()` call. We could make the shorthand serialization
smarter to deal with this in future, but for now this is already a huge
improvement over the status quo
  • Loading branch information
@lihaoyi
lihaoyi authored Sep 9, 2024
1 parent d33b29f commit f582b7e
Show file tree
Hide file tree
Showing 3 changed files with 127 additions and 35 deletions.
69 changes: 37 additions & 32 deletions .github/workflows/run-tests.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ concurrency:
cancel-in-progress: true

jobs:
# Jobs are listed in rough order of priority: if multiple jobs fail, the first job
# in the list should be the one that's most worth looking into
build-linux:
uses: ./.github/workflows/run-mill-action.yml
with:
Expand All @@ -46,38 +48,6 @@ jobs:

- run: ./mill -i docs.githubPages

compiler-bridge:
needs: build-linux
uses: ./.github/workflows/run-mill-action.yml
with:
java-version: '11'
millargs: bridge.__.publishLocal
env-bridge-versions: 'essential'

lint-autofix:
needs: build-linux
uses: ./.github/workflows/run-mill-action.yml
with:
java-version: '11'
buildcmd: ./mill -i mill.scalalib.scalafmt.ScalafmtModule/checkFormatAll __.sources + __.mimaReportBinaryIssues + __.fix --check

itest:
needs: build-linux
strategy:
fail-fast: false
matrix:
include:
# bootstrap tests
- java-version: '11' # Have one job on oldest JVM
buildcmd: ci/test-mill-dev.sh && ci/test-mill-release.sh && ./mill -i -k __.ivyDepsTree && ./mill -i -k __.ivyDepsTree --withRuntime
- java-version: 17 # Have one job on default JVM
buildcmd: ci/test-mill-bootstrap.sh

uses: ./.github/workflows/run-mill-action.yml
with:
java-version: ${{ matrix.java-version }}
buildcmd: ${{ matrix.buildcmd }}

linux:
needs: build-linux
strategy:
Expand Down Expand Up @@ -148,3 +118,38 @@ jobs:
os: windows-latest
java-version: ${{ matrix.java-version }}
millargs: ${{ matrix.millargs }}

itest:
needs: build-linux
strategy:
fail-fast: false
matrix:
include:
# bootstrap tests
- java-version: '11' # Have one job on oldest JVM
buildcmd: ci/test-mill-dev.sh && ci/test-mill-release.sh && ./mill -i -k __.ivyDepsTree && ./mill -i -k __.ivyDepsTree --withRuntime
- java-version: 17 # Have one job on default JVM
buildcmd: ci/test-mill-bootstrap.sh

uses: ./.github/workflows/run-mill-action.yml
with:
java-version: ${{ matrix.java-version }}
buildcmd: ${{ matrix.buildcmd }}

# Rarely breaks so run it near the end
compiler-bridge:
needs: build-linux
uses: ./.github/workflows/run-mill-action.yml
with:
java-version: '11'
millargs: bridge.__.publishLocal
env-bridge-versions: 'essential'

# Scalafmt, Mima, and Scalafix job runs last because it's the least important:
# usually just a automated or mechanical manual fix to do before merging
lint-autofix:
needs: build-linux
uses: ./.github/workflows/run-mill-action.yml
with:
java-version: '11'
buildcmd: ./mill -i mill.scalalib.scalafmt.ScalafmtModule/checkFormatAll __.sources + __.mimaReportBinaryIssues + __.fix --check
73 changes: 70 additions & 3 deletions scalalib/src/mill/scalalib/Dep.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,53 @@ object Dep {
case _ => throw new Exception(s"Unable to parse signature: [$signature]")
}).configure(attributes = attributes)
}

@unused private implicit val depFormat: RW[Dependency] = mill.scalalib.JsonFormatters.depFormat

def unparse(dep: Dep): Option[String] = {
val org = dep.dep.module.organization.value
val mod = dep.dep.module.name.value
val ver = dep.dep.version

val classifierAttr = dep.dep.attributes.classifier.value match {
case "" => ""
case s => s";classifier=$s"
}

val typeAttr = dep.dep.attributes.`type`.value match {
case "" => ""
case s => s";type=$s"
}
val attrs = classifierAttr + typeAttr

val prospective = dep.cross match {
case CrossVersion.Constant("", false) => Some(s"$org:$mod:$ver$attrs")
case CrossVersion.Constant("", true) => Some(s"$org:$mod::$ver$attrs")
case CrossVersion.Binary(false) => Some(s"$org::$mod:$ver$attrs")
case CrossVersion.Binary(true) => Some(s"$org::$mod::$ver$attrs")
case CrossVersion.Full(false) => Some(s"$org:::$mod:$ver$attrs")
case CrossVersion.Full(true) => Some(s"$org:::$mod::$ver$attrs")
case CrossVersion.Constant(v, _) => None
}

prospective.filter(parse(_) == dep)
}
private val rw0: RW[Dep] = macroRW

// Use literal JSON strings for common cases so that files
// containing serialized dependencies can be easier to skim
implicit val rw: RW[Dep] = upickle.default.readwriter[ujson.Value].bimap[Dep](
(dep: Dep) =>
unparse(dep) match {
case Some(s) => ujson.Str(s)
case None => upickle.default.writeJs[Dep](dep)(rw0)
},
{
case s: ujson.Str => parse(s.value)
case v: ujson.Value => upickle.default.read[Dep](v)(rw0)
}
)

def apply(
org: String,
name: String,
Expand All @@ -140,8 +187,7 @@ object Dep {
force
)
}
@unused private implicit val depFormat: RW[Dependency] = mill.scalalib.JsonFormatters.depFormat
implicit def rw: RW[Dep] = macroRW

}

sealed trait CrossVersion {
Expand Down Expand Up @@ -213,5 +259,26 @@ case class BoundDep(

object BoundDep {
@unused private implicit val depFormat: RW[Dependency] = mill.scalalib.JsonFormatters.depFormat
implicit val jsonify: upickle.default.ReadWriter[BoundDep] = upickle.default.macroRW
private val jsonify0: upickle.default.ReadWriter[BoundDep] = upickle.default.macroRW

// Use literal JSON strings for common cases so that files
// containing serialized dependencies can be easier to skim
//
// `BoundDep` is basically a `Dep` with `cross=CrossVersion.Constant("", false)`,
// so we can re-use most of `Dep`'s serialization logic
implicit val jsonify: upickle.default.ReadWriter[BoundDep] =
upickle.default.readwriter[ujson.Value].bimap[BoundDep](
bdep => {
Dep.unparse(Dep(bdep.dep, CrossVersion.Constant("", false), bdep.force)) match {
case None => upickle.default.writeJs(bdep)(jsonify0)
case Some(s) => ujson.Str(s)
}
},
{
case ujson.Str(s) =>
val dep = Dep.parse(s)
BoundDep(dep.dep, dep.force)
case v => upickle.default.read[BoundDep](v)(jsonify0)
}
)
}
20 changes: 20 additions & 0 deletions scalalib/test/src/mill/scalalib/ResolveDepsTests.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,18 @@ object ResolveDepsTests extends TestSuite {
deps.map(Lib.depToBoundDep(_, scala212Version, ""))
)

def assertRoundTrip(deps: Agg[Dep], simplified: Boolean) = {
for (dep <- deps) {
val unparsed = Dep.unparse(dep)
if (simplified) {
assert(unparsed.nonEmpty)
assert(Dep.parse(unparsed.get) == dep)
} else {
assert(unparsed.isEmpty)
}
assert(upickle.default.read[Dep](upickle.default.write(dep)) == dep)
}
}
val tests = Tests {
test("resolveValidDeps") {
val deps = Agg(ivy"com.lihaoyi::pprint:0.5.3")
Expand All @@ -25,13 +37,15 @@ object ResolveDepsTests extends TestSuite {

test("resolveValidDepsWithClassifier") {
val deps = Agg(ivy"org.lwjgl:lwjgl:3.1.1;classifier=natives-macos")
assertRoundTrip(deps, simplified = true)
val Success(paths) = evalDeps(deps)
assert(paths.nonEmpty)
assert(paths.items.next().path.toString.contains("natives-macos"))
}

test("resolveTransitiveRuntimeDeps") {
val deps = Agg(ivy"org.mockito:mockito-core:2.7.22")
assertRoundTrip(deps, simplified = true)
val Success(paths) = evalDeps(deps)
assert(paths.nonEmpty)
assert(paths.exists(_.path.toString.contains("objenesis")))
Expand All @@ -40,12 +54,14 @@ object ResolveDepsTests extends TestSuite {

test("excludeTransitiveDeps") {
val deps = Agg(ivy"com.lihaoyi::pprint:0.5.3".exclude("com.lihaoyi" -> "fansi_2.12"))
assertRoundTrip(deps, simplified = false)
val Success(paths) = evalDeps(deps)
assert(!paths.exists(_.path.toString.contains("fansi_2.12")))
}

test("excludeTransitiveDepsByOrg") {
val deps = Agg(ivy"com.lihaoyi::pprint:0.5.3".excludeOrg("com.lihaoyi"))
assertRoundTrip(deps, simplified = false)
val Success(paths) = evalDeps(deps)
assert(!paths.exists(path =>
path.path.toString.contains("com/lihaoyi") && !path.path.toString.contains("pprint_2.12")
Expand All @@ -54,24 +70,28 @@ object ResolveDepsTests extends TestSuite {

test("excludeTransitiveDepsByName") {
val deps = Agg(ivy"com.lihaoyi::pprint:0.5.3".excludeName("fansi_2.12"))
assertRoundTrip(deps, simplified = false)
val Success(paths) = evalDeps(deps)
assert(!paths.exists(_.path.toString.contains("fansi_2.12")))
}

test("errOnInvalidOrgDeps") {
val deps = Agg(ivy"xxx.yyy.invalid::pprint:0.5.3")
assertRoundTrip(deps, simplified = true)
val Failure(errMsg, _) = evalDeps(deps)
assert(errMsg.contains("xxx.yyy.invalid"))
}

test("errOnInvalidVersionDeps") {
val deps = Agg(ivy"com.lihaoyi::pprint:invalid.version.num")
assertRoundTrip(deps, simplified = true)
val Failure(errMsg, _) = evalDeps(deps)
assert(errMsg.contains("invalid.version.num"))
}

test("errOnPartialSuccess") {
val deps = Agg(ivy"com.lihaoyi::pprint:0.5.3", ivy"fake::fake:fake")
assertRoundTrip(deps, simplified = true)
val Failure(errMsg, _) = evalDeps(deps)
assert(errMsg.contains("fake"))
}
Expand Down

0 comments on commit f582b7e

Please sign in to comment.