Skip to content

Commit 8d6bbf5

Browse files
arturazarturaz
authored
Refactor publishing code to use a more appropriate data structure to represent the publishing data (#5524)
A part of #5425 extracted from it. The API has been changed from `Seq[(Contents, String)]` to `Map[os.SubPath, Contents]`. - `String` is replaced with `os.SubPath` to make sure the paths are relative without any `..` components. - `Seq` is replaced with `Map` to ensure multiple contents cannot be written to the same file path. Additionally, this turns on mima signature checking which ensures that generics are also checked. This has caught an issue in `defaultGpgArgs`, which has been fixed by this PR as well.
1 parent 788dd0f commit 8d6bbf5

File tree

14 files changed

+298
-144
lines changed

14 files changed

+298
-144
lines changed

‎contrib/artifactory/src/mill/contrib/artifactory/ArtifactoryPublisher.scala‎

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -14,42 +14,44 @@ class ArtifactoryPublisher(
1414
private val api =
1515
new ArtifactoryHttpApi(credentials, readTimeout = readTimeout, connectTimeout = connectTimeout)
1616

17-
def publish(fileMapping: Seq[(os.Path, String)], artifact: Artifact): Unit = {
17+
def publish(fileMapping: Map[os.SubPath, os.Path], artifact: Artifact): Unit = {
1818
publishAll(fileMapping -> artifact)
1919
}
2020

21-
def publishAll(artifacts: (Seq[(os.Path, String)], Artifact)*): Unit = {
21+
def publishAll(artifacts: (Map[os.SubPath, os.Path], Artifact)*): Unit = {
2222
log.info("arts: " + artifacts)
2323
val mappings = for ((fileMapping0, artifact) <- artifacts) yield {
24-
val publishPath = Seq(
24+
val publishPath = os.SubPath(Seq(
2525
artifact.group.replace(".", "/"),
2626
artifact.id,
2727
artifact.version
28-
).mkString("/")
29-
val fileMapping = fileMapping0.map { case (file, name) => (file, publishPath + "/" + name) }
30-
31-
artifact -> fileMapping.map {
32-
case (file, name) => name -> os.read.bytes(file)
28+
).mkString("/"))
29+
val fileMapping = fileMapping0.map { case (name, contents) =>
30+
publishPath / name -> os.read.bytes(contents)
3331
}
32+
33+
artifact -> fileMapping
3434
}
3535

3636
val (snapshots, releases) = mappings.partition(_._1.isSnapshot)
3737

38-
if (snapshots.nonEmpty) publishToRepo(snapshotUri, snapshots.flatMap(_._2), snapshots.map(_._1))
39-
if (releases.nonEmpty) publishToRepo(releaseUri, releases.flatMap(_._2), releases.map(_._1))
38+
if (snapshots.nonEmpty)
39+
publishToRepo(snapshotUri, snapshots.iterator.flatMap(_._2).toMap, snapshots.map(_._1))
40+
if (releases.nonEmpty)
41+
publishToRepo(releaseUri, releases.iterator.flatMap(_._2).toMap, releases.map(_._1))
4042
}
4143

4244
private def publishToRepo(
4345
repoUri: String,
44-
payloads: Seq[(String, Array[Byte])],
46+
payloads: Map[os.SubPath, Array[Byte]],
4547
artifacts: Seq[Artifact]
4648
): Unit = {
47-
val publishResults = payloads.map {
49+
val publishResults = payloads.iterator.map {
4850
case (fileName, data) =>
4951
log.info(s"Uploading $fileName")
5052
val resp = api.upload(s"$repoUri/$fileName", data)
5153
resp
52-
}
54+
}.toVector
5355
reportPublishResults(publishResults, artifacts)
5456
}
5557

‎contrib/codeartifact/src/mill/contrib/codeartifact/CodeArtifactPublisher.scala‎

Lines changed: 13 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -21,15 +21,15 @@ class CodeartifactPublisher(
2121
)
2222

2323
private def basePublishPath(artifact: Artifact) =
24-
Seq(
24+
os.SubPath(Vector(
2525
artifact.group.replace(".", "/"),
2626
artifact.id
27-
).mkString("/")
27+
))
2828

2929
private def versionPublishPath(artifact: Artifact) =
30-
s"${basePublishPath(artifact)}/${artifact.version}"
30+
basePublishPath(artifact) / artifact.version
3131

32-
def publish(fileMapping: Seq[(os.Path, String)], artifact: Artifact): Unit = {
32+
def publish(fileMapping: Map[os.SubPath, os.Path], artifact: Artifact): Unit = {
3333
publishAll(fileMapping -> artifact)
3434
}
3535

@@ -53,23 +53,21 @@ class CodeartifactPublisher(
5353
</versioning>
5454
</metadata>
5555

56-
def publishAll(artifacts: (Seq[(os.Path, String)], Artifact)*): Unit = {
56+
def publishAll(artifacts: (Map[os.SubPath, os.Path], Artifact)*): Unit = {
5757
log.info("arts: " + artifacts)
5858
val mappings = for ((fileMapping0, artifact) <- artifacts) yield {
5959
val publishPath = versionPublishPath(artifact)
60-
val fileMapping = fileMapping0.map {
61-
case (file, name) => (file, publishPath + "/" + name)
60+
val fileMapping = fileMapping0.map { case (name, contents) =>
61+
publishPath / name -> os.read.bytes(contents)
6262
}
6363

64-
artifact -> fileMapping.map {
65-
case (file, name) => name -> os.read.bytes(file)
66-
}
64+
artifact -> fileMapping
6765
}
6866

6967
val (snapshots, releases) = mappings.partition(_._1.isSnapshot)
7068

7169
if (snapshots.nonEmpty) {
72-
publishToRepo(snapshotUri, snapshots.flatMap(_._2), snapshots.map(_._1))
70+
publishToRepo(snapshotUri, snapshots.iterator.flatMap(_._2).toMap, snapshots.map(_._1))
7371

7472
val artifact = snapshots.head._1
7573
api.upload(
@@ -78,7 +76,7 @@ class CodeartifactPublisher(
7876
)
7977
}
8078
if (releases.nonEmpty) {
81-
publishToRepo(releaseUri, releases.flatMap(_._2), releases.map(_._1))
79+
publishToRepo(releaseUri, releases.iterator.flatMap(_._2).toMap, releases.map(_._1))
8280

8381
val artifact = releases.head._1
8482
api.upload(
@@ -90,15 +88,15 @@ class CodeartifactPublisher(
9088

9189
private def publishToRepo(
9290
repoUri: String,
93-
payloads: Seq[(String, Array[Byte])],
91+
payloads: Map[os.SubPath, Array[Byte]],
9492
artifacts: Seq[Artifact]
9593
): Unit = {
96-
val publishResults = payloads.map {
94+
val publishResults = payloads.iterator.map {
9795
case (fileName, data) =>
9896
log.info(s"Uploading $fileName")
9997
val resp = api.upload(s"$repoUri/$fileName", data)
10098
resp
101-
}
99+
}.toVector
102100
reportPublishResults(publishResults, artifacts)
103101
}
104102

‎contrib/gitlab/src/mill/contrib/gitlab/GitlabPublisher.scala‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,15 +10,15 @@ class GitlabPublisher(
1010
log: Logger
1111
) {
1212

13-
def publish(fileMapping: Seq[(os.Path, String)], artifact: Artifact): Unit =
13+
def publish(fileMapping: Map[os.SubPath, os.Path], artifact: Artifact): Unit =
1414
publishAll(fileMapping -> artifact)
1515

16-
def publishAll(artifacts: (Seq[(os.Path, String)], Artifact)*): Unit = {
16+
def publishAll(artifacts: (Map[os.SubPath, os.Path], Artifact)*): Unit = {
1717
log.info("Publishing artifacts: " + artifacts)
1818

1919
val uploadData = for {
2020
(items, artifact) <- artifacts
21-
files = items.map { case (path, name) => name -> os.read.bytes(path) }
21+
files = items.view.mapValues(os.read.bytes(_)).toMap
2222
} yield artifact -> files
2323

2424
uploadData
@@ -33,14 +33,14 @@ class GitlabPublisher(
3333
private def publishToRepo(
3434
repo: ProjectRepository,
3535
artifact: Artifact,
36-
payloads: Seq[(String, Array[Byte])]
36+
payloads: Map[os.SubPath, Array[Byte]]
3737
): (Artifact, Seq[Response]) = {
38-
val publishResults = payloads.map { case (fileName, data) =>
38+
val publishResults = payloads.iterator.map { case (fileName, data) =>
3939
log.info(s"Uploading $fileName")
4040
val uploadTarget = repo.uploadUrl(artifact)
4141
val resp = upload(s"$uploadTarget/$fileName", data)
4242
resp
43-
}
43+
}.toVector
4444
artifact -> publishResults
4545
}
4646

‎contrib/gitlab/test/src/mill/contrib/gitlab/GitlabTokenTests.scala‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,10 @@ object GitlabTokenTests extends TestSuite {
141141

142142
val artifact = Artifact("test.group", "id", "0.0.0")
143143

144-
publisher.publish(Seq(fakeFile -> "data.file"), artifact)
144+
publisher.publish(
145+
Map(os.SubPath("data.file") -> fakeFile),
146+
artifact
147+
)
145148

146149
os.remove(fakeFile)
147150

‎core/api/src/mill/api/JsonFormatters.scala‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@ trait JsonFormatters {
2929
implicit val relPathRW: RW[os.RelPath] = upickle.default.readwriter[String]
3030
.bimap[os.RelPath](_.toString, os.RelPath(_))
3131

32+
implicit def subPathRW: RW[os.SubPath] = JsonFormatters.Default.subPathRW
33+
3234
implicit val nioPathRW: RW[java.nio.file.Path] = upickle.default.readwriter[String]
3335
.bimap[java.nio.file.Path](
3436
_.toUri().toString(),
@@ -81,4 +83,9 @@ trait JsonFormatters {
8183
export upickle.implicits.namedTuples.default.given
8284
}
8385

84-
object JsonFormatters extends JsonFormatters
86+
object JsonFormatters extends JsonFormatters {
87+
object Default {
88+
val subPathRW: RW[os.SubPath] =
89+
upickle.default.readwriter[String].bimap[os.SubPath](_.toString, os.SubPath(_))
90+
}
91+
}

‎libs/androidlib/src/mill/androidlib/AndroidLibModule.scala‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -51,14 +51,14 @@ trait AndroidLibModule extends AndroidModule with PublishModule {
5151
*/
5252
override def publishArtifacts: T[PublishModule.PublishData] = {
5353
val baseNameTask: Task[String] = Task.Anon { s"${artifactId()}-${publishVersion()}" }
54-
val defaultPayloadTask: Task[Seq[(PathRef, String)]] = (pomPackagingType, this) match {
54+
val defaultPayloadTask = (pomPackagingType, this) match {
5555
case (PackagingType.Aar, androidLib: AndroidLibModule) => Task.Anon {
5656
val baseName = baseNameTask()
57-
Seq(
58-
androidLib.androidAar() -> s"$baseName.aar",
59-
sourceJar() -> s"$baseName-sources.jar",
60-
docJar() -> s"$baseName-javadoc.jar",
61-
pom() -> s"$baseName.pom"
57+
Map(
58+
os.SubPath(s"$baseName.aar") -> androidLib.androidAar(),
59+
os.SubPath(s"$baseName-sources.jar") -> sourceJar(),
60+
os.SubPath(s"$baseName-javadoc.jar") -> docJar(),
61+
os.SubPath(s"$baseName.pom") -> pom()
6262
)
6363
}
6464
case (otherPackagingType, otherModuleType) =>
@@ -71,7 +71,7 @@ trait AndroidLibModule extends AndroidModule with PublishModule {
7171
PublishModule.PublishData(
7272
meta = artifactMetadata(),
7373
payload = defaultPayloadTask() ++ extraPublish().map(p =>
74-
(p.file, s"$baseName${p.classifierPart}.${p.ext}")
74+
os.SubPath(s"$baseName${p.classifierPart}.${p.ext}") -> p.file
7575
)
7676
)
7777
}

‎libs/javalib/src/mill/javalib/PublishModule.scala‎

Lines changed: 35 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -451,20 +451,20 @@ trait PublishModule extends JavaModule { outer =>
451451

452452
def publishArtifacts: T[PublishModule.PublishData] = {
453453
val baseNameTask: Task[String] = Task.Anon { s"${artifactId()}-${publishVersion()}" }
454-
val defaultPayloadTask: Task[Seq[(PathRef, String)]] = (pomPackagingType, this) match {
454+
val defaultPayloadTask = (pomPackagingType, this) match {
455455
case (PackagingType.Pom, _) => Task.Anon {
456456
val baseName = baseNameTask()
457-
Seq(
458-
pom() -> s"$baseName.pom"
457+
Map(
458+
os.SubPath(s"$baseName.pom") -> pom()
459459
)
460460
}
461461
case (PackagingType.Jar, _) | _ => Task.Anon {
462462
val baseName = baseNameTask()
463-
Seq(
464-
jar() -> s"$baseName.jar",
465-
sourceJar() -> s"$baseName-sources.jar",
466-
docJar() -> s"$baseName-javadoc.jar",
467-
pom() -> s"$baseName.pom"
463+
Map(
464+
os.SubPath(s"$baseName.jar") -> jar(),
465+
os.SubPath(s"$baseName-sources.jar") -> sourceJar(),
466+
os.SubPath(s"$baseName-javadoc.jar") -> docJar(),
467+
os.SubPath(s"$baseName.pom") -> pom()
468468
)
469469
}
470470
}
@@ -473,7 +473,7 @@ trait PublishModule extends JavaModule { outer =>
473473
PublishModule.PublishData(
474474
meta = artifactMetadata(),
475475
payload = defaultPayloadTask() ++ extraPublish().map(p =>
476-
(p.file, s"$baseName${p.classifierPart}.${p.ext}")
476+
os.SubPath(s"$baseName${p.classifierPart}.${p.ext}") -> p.file
477477
)
478478
)
479479
}
@@ -541,8 +541,7 @@ trait PublishModule extends JavaModule { outer =>
541541
object PublishModule extends ExternalModule with DefaultTaskModule {
542542
def defaultTask(): String = "publishAll"
543543

544-
val defaultGpgArgs: Seq[PossiblySecret[String]] =
545-
internal.PublishModule.defaultGpgArgsForKey(key = None)
544+
val defaultGpgArgs: Seq[String] = internal.PublishModule.defaultGpgArgs
546545

547546
@deprecated("This API should have been internal and is not guaranteed to stay.", "1.0.1")
548547
def pgpImportSecretIfProvided(env: Map[String, String]): Unit =
@@ -567,17 +566,37 @@ object PublishModule extends ExternalModule with DefaultTaskModule {
567566
def sonatypeCentralSnapshotUri: String =
568567
"https://central.sonatype.com/repository/maven-snapshots/"
569568

570-
case class PublishData(meta: Artifact, payload: Seq[(PathRef, String)]) {
569+
case class PublishData(
570+
meta: Artifact,
571+
// Unfortunately we can't change the signature of this to `Map[os.SubPath, PathRef]` because
572+
// we need to keep the binary compatibility and case classes are notoriously difficult to change
573+
// while preserving binary compatibility.
574+
//
575+
// So instead we convert back and forth.
576+
payload: Seq[(PathRef, String)]
577+
) {
578+
def payloadAsMap: Map[os.SubPath, PathRef] = PublishData.seqToMap(payload)
571579

572580
/**
573581
* Maps the path reference to an actual path so that it can be used in publishAll signatures
574582
*/
575-
private[mill] def withConcretePath: (Seq[(Path, String)], Artifact) =
576-
(payload.map { case (p, f) => (p.path, f) }, meta)
583+
private[mill] def withConcretePath: (Map[os.SubPath, os.Path], Artifact) =
584+
(payloadAsMap.view.mapValues(_.path).toMap, meta)
577585
}
578586
object PublishData {
579-
import mill.javalib.publish.JsonFormatters.artifactFormat
580-
implicit def jsonify: upickle.default.ReadWriter[PublishData] = upickle.default.macroRW
587+
implicit def jsonify: upickle.default.ReadWriter[PublishData] = {
588+
import mill.javalib.publish.JsonFormatters.artifactFormat
589+
upickle.default.macroRW
590+
}
591+
592+
def apply(meta: Artifact, payload: Map[os.SubPath, PathRef]): PublishData =
593+
apply(meta, mapToSeq(payload))
594+
595+
private def seqToMap(payload: Seq[(PathRef, String)]): Map[os.SubPath, PathRef] =
596+
payload.iterator.map { case (pathRef, name) => os.SubPath(name) -> pathRef }.toMap
597+
598+
private def mapToSeq(payload: Map[os.SubPath, PathRef]): Seq[(PathRef, String)] =
599+
payload.iterator.map { case (name, pathRef) => pathRef -> name.toString }.toSeq
581600
}
582601

583602
/**

0 commit comments

Comments
 (0)