From f582b7ec8769d568f3a931f56ac6084965399890 Mon Sep 17 00:00:00 2001 From: Li Haoyi Date: Mon, 9 Sep 2024 14:37:58 +0800 Subject: [PATCH] Shorten `Dep` serialization format in common cases (#3490) Fixes https://github.com/com-lihaoyi/mill/issues/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 --- .github/workflows/run-tests.yml | 69 ++++++++++-------- scalalib/src/mill/scalalib/Dep.scala | 73 ++++++++++++++++++- .../src/mill/scalalib/ResolveDepsTests.scala | 20 +++++ 3 files changed, 127 insertions(+), 35 deletions(-) diff --git a/.github/workflows/run-tests.yml b/.github/workflows/run-tests.yml index 818f787a2b5..ed862f35a90 100644 --- a/.github/workflows/run-tests.yml +++ b/.github/workflows/run-tests.yml @@ -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: @@ -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: @@ -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 diff --git a/scalalib/src/mill/scalalib/Dep.scala b/scalalib/src/mill/scalalib/Dep.scala index 2e37bae96bb..e00e5a74b21 100644 --- a/scalalib/src/mill/scalalib/Dep.scala +++ b/scalalib/src/mill/scalalib/Dep.scala @@ -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, @@ -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 { @@ -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) + } + ) } diff --git a/scalalib/test/src/mill/scalalib/ResolveDepsTests.scala b/scalalib/test/src/mill/scalalib/ResolveDepsTests.scala index 586e0d49895..98be5c2efba 100644 --- a/scalalib/test/src/mill/scalalib/ResolveDepsTests.scala +++ b/scalalib/test/src/mill/scalalib/ResolveDepsTests.scala @@ -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") @@ -25,6 +37,7 @@ 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")) @@ -32,6 +45,7 @@ object ResolveDepsTests extends TestSuite { 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"))) @@ -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") @@ -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")) }