Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

IGN-10612 WIP DO NOT MERGE just for sharing code easily #57

Closed
wants to merge 7 commits into from
Closed

IGN-10612 WIP DO NOT MERGE just for sharing code easily #57

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
54b121e
deduplicate module.xml jar entries by combining scopes
benmusson Aug 6, 2024
44ee58a
IGN-10612 WIP refactor for clarity + code golf
brianeray Aug 14, 2024
06930b7
IGN-10612 WIP more code golf
brianeray Aug 14, 2024
6539bdb
IGN-10612 WIP Acid test the dedup filter
brianeray Aug 15, 2024
738a9f5
IGN-10612 WIP Placate Spotless linter
brianeray Aug 15, 2024
376f0a2
combine scope, then create pair
benmusson Aug 16, 2024
22946e2
IGN-10612 feedback from IA module devs
brianeray Aug 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 91 additions & 20 deletions ...-module-plugin/src/functionalTest/kotlin/io/ia/sdk/gradle/modl/task/WriteModuleXmlTest.kt
View file
Open in desktop
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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tricks everybody. Me. Others at IA. Probably everybody who has used it and the underlying JUnit call.

)
}

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
Open in desktop
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
Open in desktop
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"),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mirrors the same thing in the Ignition codebase.

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
Open in desktop
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.
Copy link
Collaborator Author

@brianeray brianeray Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I regression tested this not only via unit test coverage in WriteModuleXmlTest.kt, but by copying over that test class to a local master branch along with the test helper utils.kt and running it there.

The only thing that failed--as expected--was the new --foldJars test.

So I'm pretty sure the logic that follows is equivalent to that before these PRs. I think the biggest difference is just refactoring to do pure transforms instead of nested forEaches that have side effects (writing the output).

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 }
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Essentially, flipflop keys and values while flattening the original values.

)
}
}

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