Skip to content

Commit

Permalink
IGN-10612 feedback from IA module devs
Browse files Browse the repository at this point in the history 
Add `--foldJars` task option to preserve the old behavior by default,
it's a breaking change and maybe the dups are useful to some.

The option if passed does the scope folding and the sorting as before
and another sort of folding for `CDG`-scoped jars, turning them into
the `A`[ll]-scoped equivalent.

Plus noticed that the tests fall into the `assertEquals` API trap where
you think the argument order is `actual, expected` but it's really
`expected, actual`. Completely nonintuitive and sort of asymmetric to
other `assert*` APIs but when `assertEquals` fails having them in the
wrong order makes things harder to troubleshoot.
  • Loading branch information
@brianeray
brianeray committed Aug 20, 2024
1 parent 376f0a2 commit 22946e2
Show file tree
Hide file tree
Showing 4 changed files with 196 additions and 40 deletions.
111 changes: 91 additions & 20 deletions ...-module-plugin/src/functionalTest/kotlin/io/ia/sdk/gradle/modl/task/WriteModuleXmlTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import io.ia.ignition.module.generator.api.ProjectScope
import io.ia.ignition.module.generator.util.replacePlaceholders
import io.ia.sdk.gradle.modl.BaseTest
import io.ia.sdk.gradle.modl.util.collapseXmlToOneLine
import io.ia.sdk.gradle.modl.util.splitXmlNodesToList
import org.gradle.testkit.runner.BuildResult
import org.gradle.testkit.runner.TaskOutcome
import java.io.File
Expand Down Expand Up @@ -48,8 +49,8 @@ class WriteModuleXmlTest : BaseTest() {
"""<depends scope="GCD" required="false">io.ia.modl</depends>"""
)
assertEquals(
1,
Regex(DEPENDS).findAll(oneLineXml).toList().size,
1
)
}

Expand Down Expand Up @@ -86,8 +87,8 @@ class WriteModuleXmlTest : BaseTest() {
"""<depends scope="G" required="true">io.ia.otherModl</depends>"""
)
assertEquals(
2,
Regex(DEPENDS).findAll(oneLineXml).toList().size,
2
)
}

Expand Down Expand Up @@ -125,7 +126,6 @@ class WriteModuleXmlTest : BaseTest() {
val oneLineXml = generateXml(
dirName,
replacements,
// true,
)

assertContains(
Expand All @@ -137,8 +137,8 @@ class WriteModuleXmlTest : BaseTest() {
"""<depends scope="G" required="true">io.ia.otherModl</depends>"""
)
assertEquals(
2,
Regex(DEPENDS).findAll(oneLineXml).toList().size,
2
)
}

Expand All @@ -159,14 +159,14 @@ class WriteModuleXmlTest : BaseTest() {
"""<depends scope="GCD">io.ia.modl</depends>"""
)
assertEquals(
1,
Regex(DEPENDS).findAll(oneLineXml).toList().size,
1
)
}

@Test
// @Tag ("IGN-10612")
fun `jars are included, de-duplicated, and sorted`() {
fun `jars are de-duplicated and sorted with --foldJars option`() {
val dirName = currentMethodName()
val dependencies = mapOf<String, String>(
// JLA-1.5 pulls in commons-math3-3.5 as transitive dep
Expand All @@ -179,33 +179,98 @@ class WriteModuleXmlTest : BaseTest() {
// Pulls in commons-pool-1.5.4 as transitive dep
"DG" to "modlApi 'commons-dbcp:commons-dbcp:1.4'",
"CD" to "modlApi 'args4j:args4j:2.0.8'",
// Translates to shorthand A[ll]
"CDG" to "modlApi 'com.inductiveautomation.ignition:ia-gson:2.10.1'",
)

val oneLineXml = generateXml(dirName, emptyMap(), dependencies)
val oneLineXml = generateXml(
dirName = dirName,
replacements = emptyMap(),
dependencies = dependencies,
foldJars = true,
)

// Split to list on the whitespace between nodes, extract <jar/>s.
val jars =
oneLineXml.split(Regex("""(?<=>)\s+(?=<)"""))
.filter { node -> node.startsWith("<jar") }
val jars = splitXmlNodesToList(oneLineXml, listOf("jar"))

assertEquals(
jars,
listOf(
"""<jar scope="CDG">common-0.0.1-SNAPSHOT.jar</jar>""",
"""<jar scope="CDG">ia-gson-2.10.1.jar</jar>""",
"""<jar scope="CDG">javassist-3.12.1.GA.jar</jar>""",
"""<jar scope="CD">args4j-2.0.8.jar</jar>""",
"""<jar scope="CD">client-0.0.1-SNAPSHOT.jar</jar>""",
"""<jar scope="CD">jline-2.12.jar</jar>""",

"""<jar scope="DG">commons-dbcp-1.4.jar</jar>""",
"""<jar scope="DG">commons-pool-1.5.4.jar</jar>""",

"""<jar scope="A">common-0.0.1-SNAPSHOT.jar</jar>""",
"""<jar scope="A">ia-gson-2.10.1.jar</jar>""",
"""<jar scope="A">javassist-3.12.1.GA.jar</jar>""",

"""<jar scope="D">designer-0.0.1-SNAPSHOT.jar</jar>""",
"""<jar scope="D">duckdb_jdbc-0.9.2.jar</jar>""",

"""<jar scope="G">JLargeArrays-1.5.jar</jar>""",
"""<jar scope="G">commons-math3-3.5.jar</jar>""",
"""<jar scope="G">gateway-0.0.1-SNAPSHOT.jar</jar>""",
)
),
jars,
)
}

@Test
// @Tag ("IGN-10612")
fun `jars are written largely as-is by default`() {
val dirName = currentMethodName()
val dependencies = mapOf<String, String>(
// JLA-1.5 pulls in commons-math3-3.5 as transitive dep
"G" to "modlApi 'pl.edu.icm:JLargeArrays:1.5'",
"D" to "modlApi 'org.duckdb:duckdb_jdbc:0.9.2'",
// C[lient] implies D[esigner], so here C -> CD
"C" to "modlApi 'jline:jline:2.12'",
// Similarly, here CG -> CD and separately G
"CG" to "modlApi 'javassist:javassist:3.12.1.GA'",
// Pulls in commons-pool-1.5.4 as transitive dep
"DG" to "modlApi 'commons-dbcp:commons-dbcp:1.4'",
"CD" to "modlApi 'args4j:args4j:2.0.8'",
"CDG" to "modlApi 'com.inductiveautomation.ignition:ia-gson:2.10.1'",
)

val oneLineXml = generateXml(
dirName = dirName,
replacements = emptyMap(),
dependencies = dependencies,
// default is foldJars = false,
)

// Split to list on the whitespace between nodes, extract <jar/>s.
val jars = splitXmlNodesToList(oneLineXml, listOf("jar"))

assertEquals(
listOf(
"""<jar scope="CD">ia-gson-2.10.1.jar</jar>""",
"""<jar scope="CD">args4j-2.0.8.jar</jar>""",
"""<jar scope="CD">javassist-3.12.1.GA.jar</jar>""",
"""<jar scope="CD">jline-2.12.jar</jar>""",
"""<jar scope="CD">client-0.0.1-SNAPSHOT.jar</jar>""",

"""<jar scope="CDG">common-0.0.1-SNAPSHOT.jar</jar>""",

"""<jar scope="D">ia-gson-2.10.1.jar</jar>""",
"""<jar scope="D">args4j-2.0.8.jar</jar>""",
"""<jar scope="D">commons-dbcp-1.4.jar</jar>""",
"""<jar scope="D">duckdb_jdbc-0.9.2.jar</jar>""",
"""<jar scope="D">commons-pool-1.5.4.jar</jar>""",
"""<jar scope="D">designer-0.0.1-SNAPSHOT.jar</jar>""",

"""<jar scope="G">ia-gson-2.10.1.jar</jar>""",
"""<jar scope="G">commons-dbcp-1.4.jar</jar>""",
"""<jar scope="G">javassist-3.12.1.GA.jar</jar>""",
"""<jar scope="G">JLargeArrays-1.5.jar</jar>""",
"""<jar scope="G">commons-pool-1.5.4.jar</jar>""",
"""<jar scope="G">commons-math3-3.5.jar</jar>""",
"""<jar scope="G">gateway-0.0.1-SNAPSHOT.jar</jar>""",
),
jars,
)
}

Expand Down Expand Up @@ -236,6 +301,7 @@ class WriteModuleXmlTest : BaseTest() {
dirName: String,
replacements: Map<String, String> = mapOf(),
dependencies: Map<String, String> = mapOf(),
foldJars: Boolean = false,
dumpBuildScript: Boolean = false,
): String {
val projectDir = generateModule(
Expand All @@ -260,16 +326,21 @@ class WriteModuleXmlTest : BaseTest() {
println(projectDir.resolve("build.gradle").readText())
}

val args = mutableListOf(
// "--stacktrace",
"writeModuleXml",
)
if (foldJars) {
args.add("--foldJars")
}

val result: BuildResult = runTask(
projectDir.toFile(),
listOf(
"writeModuleXml",
"--stacktrace",
)
args,
)

val task = result.task(":writeModuleXml")
assertEquals(task?.outcome, TaskOutcome.SUCCESS)
assertEquals(TaskOutcome.SUCCESS, task?.outcome)

// We could do real XML parsing here but this is just a test,
// quick-and-dirty should be fine.
Expand Down
13 changes: 13 additions & 0 deletions gradle-module-plugin/src/functionalTest/kotlin/io/ia/sdk/gradle/modl/util/utils.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,16 @@ fun nameToDirName(moduleName: String): String {
// tags together in one long line by knocking out indentation and newlines.
fun collapseXmlToOneLine(xml: String): String =
xml.replace(Regex("""^\s+"""), "").replace(Regex("""\R"""), "")

// Likewise, for when it's useful to break XML nodes out to a list of node
// names. With optional inclusive filter.
fun splitXmlNodesToList(
xml: String,
nodeFilter: List<String> = listOf<String>(),
): List<String> =
xml.split(Regex("""(?<=>)\s+(?=<)"""))
.filter { n ->
// if no filter, include; else look for BOL with matching tag
nodeFilter.isEmpty() ||
nodeFilter.any { nf -> n.startsWith("<$nf") }
}
15 changes: 14 additions & 1 deletion gradle-module-plugin/src/main/kotlin/io/ia/sdk/gradle/modl/model/Models.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,11 @@ enum class IgnitionScope(val code: String) {
GATEWAY_DESIGNER("DG"),
GATEWAY_DESIGNER_VISION_CLIENT("CDG"),
GATEWAY_VISION_CLIENT("CG"),
ALL("A"),
NONE("");

companion object {
private val VALID_SCOPES = Regex("^[GCD]+\$")
private val VALID_SCOPES = Regex("^[AGCD]+\$")

/**
* Applies simplistic validation to the string and returns the corresponding scope if it can be determined.
Expand Down Expand Up @@ -60,8 +61,20 @@ enum class IgnitionScope(val code: String) {
"CDG" -> GATEWAY_DESIGNER_VISION_CLIENT
"DG" -> GATEWAY_DESIGNER
"CG" -> GATEWAY_VISION_CLIENT
"A" -> ALL
else -> throw Exception("Could not determine IgnitionScope for shorthand '$code'")
}
}

/**
* Performs 'CDG'-to-'A' transform when applicable.
*/
@JvmStatic
@Throws(ModuleConfigException::class)
fun promoteToAllWhenImplied(scopes: String): IgnitionScope =
forShorthand(scopes).let { scope ->
if (scope == GATEWAY_DESIGNER_VISION_CLIENT)
ALL else scope
}
}
}
97 changes: 78 additions & 19 deletions gradle-module-plugin/src/main/kotlin/io/ia/sdk/gradle/modl/task/WriteModuleXml.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package io.ia.sdk.gradle.modl.task

import io.ia.sdk.gradle.modl.PLUGIN_TASK_GROUP
import io.ia.sdk.gradle.modl.extension.ModuleDependencySpec
import io.ia.sdk.gradle.modl.model.ArtifactManifest
import io.ia.sdk.gradle.modl.model.IgnitionScope
import io.ia.sdk.gradle.modl.model.artifactManifestFromJson
import org.gradle.api.DefaultTask
import org.gradle.api.file.RegularFile
Expand All @@ -14,6 +16,7 @@ import org.gradle.api.tasks.InputFiles
import org.gradle.api.tasks.Optional
import org.gradle.api.tasks.OutputFile
import org.gradle.api.tasks.TaskAction
import org.gradle.api.tasks.options.Option
import org.redundent.kotlin.xml.PrintOptions
import org.redundent.kotlin.xml.xml
import java.io.File
Expand Down Expand Up @@ -98,6 +101,23 @@ open class WriteModuleXml @Inject constructor(_objects: ObjectFactory) : Default
@get:Optional
val docIndexPath: Property<String> = _objects.property(String::class.java)

/**
* Whether to fold and sort duplicate jars dependencies when the same appear
* in more than one scope.
*
* Also converts implicit all scope string 'CDG' to 'A' and sorts jars by
* scope string.
*
* Default is `false`.
*/
@get:Input
@get:Optional
@Option(
description = "Folds jars from multiple scopes into a single jar entry."
)
val foldJars: Property<Boolean> = _objects.property(Boolean::class.java)
.convention(false)

@OutputFile
fun getModuleXmlFile(): File {
return project.file("${project.buildDir}/moduleContent/module.xml")
Expand Down Expand Up @@ -214,31 +234,70 @@ open class WriteModuleXml @Inject constructor(_objects: ObjectFactory) : Default
return false
}

// Manifests' artifacts, collect + sort by distinct scopes to de-dup them
// Manifests' artifacts
private fun manifests(): List<Pair<String, String>> =
artifactManifests.get().map { manifest ->
artifactManifestFromJson(manifest.asFile.readText(Charsets.UTF_8))
}.let { manifests ->
manifests
.flatMap { mani -> mani.artifacts }
.groupBy { arti -> arti.jarName }
.map { (jar, artifacts) ->
val combinedScope = artifacts.fold(setOf<Char>()) { scope, arti ->
scope.union(
manifests
.filter { mani -> arti in mani.artifacts }
.flatMap { mani -> mani.scope.toList() }
)
}.joinToString("")

jar to combinedScope
}.sortedWith(
compareByDescending<Pair<String, String>> { (_, scope) -> scope.length }
.thenBy { (_, scope) -> scope }
.thenBy { (jar, _) -> jar }
)
if (foldJars.get())
// more compact or else legacy dup-prone
deduplicatedJars(manifests) else rawScopedJars(manifests)
}

// Collapse duplicate artifacts in different scopes to single scope string.
//
// IGN-10168 is backlogged to handle at least some of this upstream,
// probably in or near `collectModlDependencies`.
private fun deduplicatedJars(
manifests: List<ArtifactManifest>
): List<Pair<String, String>> =
manifests
.flatMap { mani -> mani.artifacts }
.groupBy { arti -> arti.jarName }
.map { (jar, artifacts) ->
val combinedScope = artifacts.fold(setOf<Char>()) { scope, arti ->
scope.union(
manifests
.filter { mani -> arti in mani.artifacts }
.flatMap { mani -> mani.scope.toList() }
)
}.joinToString("")
.let { scope ->
// CDG > A formalized b/c we'll want it for IGN-10168
IgnitionScope.promoteToAllWhenImplied(scope).code
}

jar to combinedScope
}.sortedWith(
compareByDescending<Pair<String, String>> { (_, scope) -> scope.length }
.thenBy { (_, scope) -> scope }
.thenBy { (jar, _) -> jar }
)

// Leave duplicate artifacts largely as-is, even if present in 2+ scopes.
// This is legacy behavior.
private fun rawScopedJars(
manifests: List<ArtifactManifest>
): List<Pair<String, String>> =
manifests
.groupBy { mani -> mani.scope }
.map { (scope, manis) ->
val distinctJars =
manis
.flatMap { mani -> mani.artifacts }
.fold(mutableSetOf<String>()) { jars, arti ->
jars.apply { add(arti.jarName) }
}

scope to distinctJars
}.fold(mutableListOf<Pair<String, String>>()) { lst, (scope, jars) ->
lst.apply {
addAll(
jars.map { jar -> jar to scope }
)
}
}

fun writeXml(outputFile: File, moduleXml: String) {
outputFile.writeText(moduleXml)
}
Expand Down

0 comments on commit 22946e2

Please sign in to comment.