Skip to content

Commit 6022687

Browse files
LouisCADLouisCAD
committed
Make LintUpdater run in parallel and handle edge cases
It now runs while refreshVersions is trying to reach the artifact repositories, for minimal impact on the speed of the refreshVersions task. The code updating the lint.xml file now returns a list containing any encountered problems, that will be logged at the end of the refreshVersions task run. Analysis of the lint.xml file is now done semantically, using the javax.xml APIs (aka. DOM), so comments won't affect it. More test data has also been added to test the newly handled cases, successful and problematic.
1 parent 2c53c5c commit 6022687

File tree

35 files changed

+454
-49
lines changed

35 files changed

+454
-49
lines changed

‎plugins/core/src/main/kotlin/de/fayard/refreshVersions/core/RefreshVersionsTask.kt‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,20 +3,19 @@ package de.fayard.refreshVersions.core
33
import de.fayard.refreshVersions.core.internal.RefreshVersionsConfigHolder
44
import de.fayard.refreshVersions.core.internal.RefreshVersionsConfigHolder.settings
55
import de.fayard.refreshVersions.core.internal.SettingsPluginsUpdater
6-
import de.fayard.refreshVersions.core.internal.configureLintOnAndroidProjects
6+
import de.fayard.refreshVersions.core.internal.configureLintIfRunningOnAnAndroidProject
77
import de.fayard.refreshVersions.core.internal.legacy.LegacyBootstrapUpdater
88
import de.fayard.refreshVersions.core.internal.lookupVersionCandidates
9+
import de.fayard.refreshVersions.core.internal.problems.log
910
import de.fayard.refreshVersions.core.internal.versions.VersionsPropertiesModel
1011
import de.fayard.refreshVersions.core.internal.versions.writeWithNewVersions
1112
import kotlinx.coroutines.*
1213
import org.gradle.api.DefaultTask
1314
import org.gradle.api.artifacts.Dependency
14-
import org.gradle.api.logging.LogLevel
1515
import org.gradle.api.tasks.Input
1616
import org.gradle.api.tasks.Optional
1717
import org.gradle.api.tasks.TaskAction
1818
import org.gradle.api.tasks.options.Option
19-
import org.gradle.api.tasks.options.OptionValues
2019
import org.gradle.util.GradleVersion
2120

2221
/**
@@ -57,6 +56,9 @@ open class RefreshVersionsTask : DefaultTask() {
5756
// will reduce the number of repositories lookups, improving performance a little more.
5857

5958
runBlocking {
59+
val lintUpdatingProblemsAsync = async {
60+
configureLintIfRunningOnAnAndroidProject(settings, RefreshVersionsConfigHolder.readVersionsMap())
61+
}
6062
val result = lookupVersionCandidates(
6163
httpClient = RefreshVersionsConfigHolder.httpClient,
6264
project = project,
@@ -79,7 +81,9 @@ open class RefreshVersionsTask : DefaultTask() {
7981
warnAboutHardcodedVersionsIfAny(result.dependenciesWithHardcodedVersions)
8082
warnAboutDynamicVersionsIfAny(result.dependenciesWithDynamicVersions)
8183
warnAboutGradleUpdateAvailableIfAny(result.gradleUpdates)
82-
configureLintOnAndroidProjects(settings, RefreshVersionsConfigHolder.readVersionsMap())
84+
lintUpdatingProblemsAsync.await().forEach { problem ->
85+
logger.log(problem)
86+
}
8387
}
8488
}
8589

‎plugins/core/src/main/kotlin/de/fayard/refreshVersions/core/extensions/dom/NodeList.kt‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
package de.fayard.refreshVersions.core.extensions.dom
2+
3+
import org.w3c.dom.Node
4+
import org.w3c.dom.NodeList
5+
6+
internal fun NodeList.asList(): List<Node> = List(length) { item(it) }

‎plugins/core/src/main/kotlin/de/fayard/refreshVersions/core/extensions/text/String.kt‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,3 +37,12 @@ internal fun String.substringUpTo(delimiter: Char, missingDelimiterValue: String
3737
val index = indexOf(delimiter)
3838
return if (index == -1) missingDelimiterValue else substring(0, index + 1)
3939
}
40+
41+
/**
42+
* Returns a substring up to the first occurrence of [delimiter].
43+
* If the string does not contain the delimiter, returns [missingDelimiterValue] which defaults to the original string.
44+
*/
45+
internal fun String.substringUpTo(delimiter: String, missingDelimiterValue: String = this): String {
46+
val index = indexOf(delimiter)
47+
return if (index == -1) missingDelimiterValue else substring(0, index + delimiter.length)
48+
}

‎plugins/core/src/main/kotlin/de/fayard/refreshVersions/core/internal/LintUpdater.kt‎

Lines changed: 148 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,40 +1,158 @@
11
package de.fayard.refreshVersions.core.internal
22

3+
import de.fayard.refreshVersions.core.extensions.dom.asList
4+
import de.fayard.refreshVersions.core.extensions.text.substringUpTo
5+
import de.fayard.refreshVersions.core.internal.problems.Problem
36
import org.gradle.api.initialization.Settings
7+
import org.w3c.dom.Document
8+
import org.w3c.dom.Element
9+
import org.w3c.dom.Node
10+
import java.io.File
11+
import javax.xml.parsers.DocumentBuilderFactory
412

5-
// Disable a warning in Android Studio that AGP has version "_"
6-
fun configureLintOnAndroidProjects(settings: Settings, versionsMap: Map<String, String>) {
7-
if ("plugin.android" !in versionsMap) return
8-
val file = settings.rootDir.resolve("lint.xml")
9-
val currentLintXml = when {
10-
file.canRead() -> file.readText()
13+
/**
14+
* Android projects have Android lint enabled that will warn when the version of a dependency
15+
* is set to the version placeholder (`_`). This function will edit the `lint.xml` config file
16+
* if needed to add ignore rules for these lint warnings/errors.
17+
*
18+
* If these lint "issues" are already set to a non ignore value, they will not be edited,
19+
* but problems will be returned in the list instead.
20+
*/
21+
internal fun configureLintIfRunningOnAnAndroidProject(
22+
settings: Settings,
23+
versionsMap: Map<String, String>
24+
): List<Problem<LintUpdatingIssue>> {
25+
if ("plugin.android" !in versionsMap) return emptyList()
26+
val lintFile = settings.rootDir.resolve("lint.xml")
27+
val (newXml, problems) = attemptGettingLintXmlWithMissingRules(lintFile = lintFile)
28+
newXml?.let {
29+
lintFile.writeText(it)
30+
}
31+
return problems
32+
}
33+
34+
internal sealed class LintUpdatingIssue {
35+
36+
class UnexpectedSeverity(
37+
val issueId: String,
38+
val actualSeverity: String?
39+
) : LintUpdatingIssue() {
40+
override fun toString() = """
41+
|Expected the severity of $issueId to be "ignore" but found $actualSeverity instead.
42+
|If it's intentional, you can ignore this message, otherwise,
43+
|we recommend to correct the severity manually.
44+
""".trimMargin()
45+
}
46+
47+
class ParsingFailure(
48+
val throwable: Throwable? = null,
49+
val errorMessage: String = throwable?.message
50+
?: throwable?.toString()
51+
?: error("Expected a throwable or an explicit message")
52+
) : LintUpdatingIssue() {
53+
override fun toString() = errorMessage
54+
}
55+
56+
abstract override fun toString(): String
57+
}
58+
59+
@Suppress("SuspiciousCollectionReassignment")
60+
internal fun attemptGettingLintXmlWithMissingRules(
61+
lintFile: File? = null,
62+
lintXmlContent: String = when {
63+
lintFile == null -> error("Need lintFile or lintXmlContent to be set")
64+
lintFile.exists() -> lintFile.readText()
1165
else -> ""
1266
}
13-
val newLintXml = updateLintXml(currentLintXml)
14-
if (newLintXml !== currentLintXml) {
15-
file.writeText(newLintXml)
67+
): Pair<String?, List<Problem<LintUpdatingIssue>>> {
68+
69+
fun nonFatalError(issue: LintUpdatingIssue): Problem<LintUpdatingIssue> {
70+
return Problem(level = Problem.Level.Error, issue = issue, affectedFile = lintFile)
71+
}
72+
73+
if (lintXmlContent.isBlank()) {
74+
return """
75+
|<?xml version="1.0" encoding="UTF-8"?>
76+
|<lint>
77+
| <!-- Generated by `./gradlew refreshVersions` to avoid errors when using _ as a version. -->
78+
| <issue id="GradlePluginVersion" severity="ignore" />
79+
| <issue id="GradleDependency" severity="ignore" />
80+
|</lint>
81+
""".trimMargin() to emptyList()
82+
}
83+
84+
val document: Document = runCatching {
85+
DocumentBuilderFactory.newInstance().newDocumentBuilder().parse(lintXmlContent.byteInputStream())
86+
}.getOrElse {
87+
return null to listOf(nonFatalError(issue = LintUpdatingIssue.ParsingFailure(throwable = it)))
1688
}
89+
90+
if (document.documentElement.tagName != "lint") {
91+
return null to listOf(
92+
nonFatalError(
93+
issue = LintUpdatingIssue.ParsingFailure(
94+
errorMessage = "Couldn't find the root tag named \"lint\""
95+
)
96+
)
97+
)
98+
}
99+
100+
var problems = emptyList<Problem<LintUpdatingIssue>>()
101+
102+
val nodes = document.documentElement.childNodes.asList()
103+
104+
fun findSuppressNode(issueId: String): Node? = nodes.firstOrNull {
105+
it is Element && it.isIssue(id = issueId)
106+
}?.also {
107+
check(it is Element)
108+
if (it.hasAttribute("severity", expectedValue = "ignore").not()) {
109+
problems += Problem(
110+
level = Problem.Level.Warning, issue = LintUpdatingIssue.UnexpectedSeverity(
111+
issueId = issueId,
112+
actualSeverity = it.attributes.getNamedItem("severity")?.nodeValue
113+
),
114+
affectedFile = lintFile
115+
)
116+
}
117+
}
118+
// Note: "AGP" stands for "Android Gradle Plugin"
119+
val agpErrorSuppressNodeOrNull = findSuppressNode("GradlePluginVersion")
120+
val libsVersionsWarningSuppressNode = findSuppressNode("GradleDependency")
121+
122+
123+
if (agpErrorSuppressNodeOrNull != null && libsVersionsWarningSuppressNode != null) return null to problems
124+
125+
// We are not using the javax.xml API to edit the xml file because it messes-up the formatting by adding
126+
// unwanted empty lines after every tag, and working around that issue is overly complicated:
127+
// it requires feeding a schema, which doesn't seemed to exist, and we are not going to reverse-engineer it.
128+
// This text insertion logic is good enough
129+
val newLintXmlFile = buildString {
130+
val commentText = "Generated by `./gradlew refreshVersions` to avoid errors when using _ as a version."
131+
val firstPart = lintXmlContent.substringUpTo("<lint>")
132+
val secondPart = lintXmlContent.substringAfter("<lint>")
133+
append(firstPart)
134+
appendln()
135+
appendln(" <!-- $commentText -->")
136+
if (agpErrorSuppressNodeOrNull == null) {
137+
append(""" <issue id="GradlePluginVersion" severity="ignore" />""")
138+
}
139+
if (libsVersionsWarningSuppressNode == null) {
140+
if (agpErrorSuppressNodeOrNull == null) appendln()
141+
append(""" <issue id="GradleDependency" severity="ignore" />""")
142+
}
143+
144+
append(secondPart)
145+
}
146+
return newLintXmlFile to problems
17147
}
18148

19-
fun updateLintXml(input: String): String {
20-
val alreadyAdded = input.contains("GradleDependency") && input.contains("GradlePluginVersion")
21-
if (alreadyAdded) return input
22-
val ignoreRules = """
23-
| <issue id="GradleDependency" severity="ignore" />
24-
| <issue id="GradlePluginVersion" severity="ignore" />
25-
""".trimMargin()
26-
27-
val prefix = when {
28-
input.isBlank() -> """
29-
<?xml version="1.0" encoding="UTF-8"?>
30-
<lint>
31-
""".trimIndent()
32-
else -> input.substringBefore("</lint>").trimEnd()
33-
}
34-
val postfix = "</lint>"
35-
return """
36-
|$prefix
37-
|$ignoreRules
38-
|$postfix
39-
""".trimMargin()
149+
private fun Element.isIssue(id: String): Boolean {
150+
return tagName == "issue" && hasAttribute(name = "id", expectedValue = id)
151+
}
152+
153+
private fun Element.hasAttribute(name: String, expectedValue: String): Boolean {
154+
@Suppress("RedundantLambdaArrow") // Used to specify that the type is nullable.
155+
return attributes.getNamedItem(name).let { it: Node? ->
156+
it != null && (expectedValue == it.nodeValue)
157+
}
40158
}

‎plugins/core/src/main/kotlin/de/fayard/refreshVersions/core/internal/problems/Problem.kt‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
package de.fayard.refreshVersions.core.internal.problems
2+
3+
import java.io.File
4+
5+
internal data class Problem<out T>(
6+
val level: Level,
7+
val issue: T,
8+
val errorMessage: String = issue.toString(),
9+
val affectedFile: File? = null
10+
) {
11+
12+
sealed class Level {
13+
object Warning : Level()
14+
sealed class Error : Level() {
15+
companion object : Error()
16+
object Fatal : Error()
17+
}
18+
}
19+
}

‎plugins/core/src/main/kotlin/de/fayard/refreshVersions/core/internal/problems/ProblemLogging.kt‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
package de.fayard.refreshVersions.core.internal.problems
2+
3+
import org.gradle.api.logging.LogLevel
4+
import org.gradle.api.logging.Logger
5+
6+
internal fun Logger.log(problem: Problem<Any>) {
7+
val logLevel = when (problem.level) {
8+
Problem.Level.Warning -> LogLevel.WARN
9+
Problem.Level.Error -> LogLevel.ERROR
10+
Problem.Level.Error.Fatal -> LogLevel.ERROR
11+
}
12+
val levelLetter = when (problem.level) {
13+
Problem.Level.Warning -> 'w'
14+
is Problem.Level.Error -> 'e'
15+
}
16+
val affectedFile = problem.affectedFile
17+
val message = problem.errorMessage
18+
when (affectedFile) {
19+
null -> log(logLevel, message)
20+
else -> {
21+
// This log level format is recognized by IntelliJ IDEA.
22+
log(logLevel, "$levelLetter: ${affectedFile.path}:\n$message")
23+
}
24+
}
25+
}

‎plugins/core/src/test/kotlin/de/fayard/refreshVersions/core/LintUpdaterTest.kt‎

Lines changed: 66 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,29 +1,84 @@
11
package de.fayard.refreshVersions.core
22

3-
import de.fayard.refreshVersions.core.internal.updateLintXml
3+
import de.fayard.refreshVersions.core.internal.LintUpdatingIssue
4+
import de.fayard.refreshVersions.core.internal.attemptGettingLintXmlWithMissingRules
45
import io.kotest.matchers.shouldBe
56
import org.junit.jupiter.api.TestFactory
67
import testutils.junit.mapDynamicTest
7-
import java.io.File
8+
import kotlin.test.Test
9+
import kotlin.test.assertTrue
810

911
class LintUpdaterTest {
1012

1113
private val testDataDir = testResources.resolve("lint-updater")
1214

13-
private val dirs = testDataDir.listFiles { file ->
15+
@TestFactory
16+
fun `test lint updater`() = testDataDir.resolve("no-problem").listFiles { file ->
1417
file.isDirectory
15-
}!!.asList()
18+
}!!.asList().mapDynamicTest { directory ->
19+
require(directory.isDirectory)
20+
val input = directory.resolve("input.xml").readText()
21+
val expected = directory.resolve("expected.xml").readText()
22+
val (updatedXml, _) = attemptGettingLintXmlWithMissingRules(lintXmlContent = input)
23+
(updatedXml ?: input).trim() shouldBe expected.trim()
24+
}
1625

1726
@TestFactory
18-
fun `lint updater`() = dirs.mapDynamicTest { dirOfSample ->
19-
`test lint updater`(dirOfSample)
27+
fun `test updating lint files with one wrong severity`() =
28+
testDataDir.resolve("one-wrong-severity").listFiles { file ->
29+
file.isDirectory
30+
}!!.asList().mapDynamicTest { directory ->
31+
require(directory.isDirectory)
32+
val input = directory.resolve("input.xml").readText()
33+
val expected = directory.resolve("expected.xml").readText()
34+
val (updatedXml, problems) = attemptGettingLintXmlWithMissingRules(lintXmlContent = input)
35+
(updatedXml ?: input).trim() shouldBe expected.trim()
36+
problems.size shouldBe 1
37+
assertTrue(problems.single().issue is LintUpdatingIssue.UnexpectedSeverity)
38+
}
39+
40+
@TestFactory
41+
fun `test updating lint files with two wrong severity`() =
42+
testDataDir.resolve("two-wrong-severity").listFiles { file ->
43+
file.isDirectory
44+
}!!.asList().mapDynamicTest { directory ->
45+
require(directory.isDirectory)
46+
val input = directory.resolve("input.xml").readText()
47+
val expected = directory.resolve("expected.xml").readText()
48+
val (updatedXml, problems) = attemptGettingLintXmlWithMissingRules(lintXmlContent = input)
49+
(updatedXml ?: input).trim() shouldBe expected.trim()
50+
problems.size shouldBe 2
51+
problems.forEach {
52+
assertTrue(it.issue is LintUpdatingIssue.UnexpectedSeverity)
53+
}
54+
}
55+
56+
@Test
57+
fun `wrong root tag returns a problem`() {
58+
val input = """
59+
|<?xml version="1.0" encoding="UTF-8"?>
60+
|<mint>
61+
| <!-- Generated by `./gradlew refreshVersions` to avoid errors when using _ as a version. -->
62+
| <issue id="GradlePluginVersion" severity="ignore" />
63+
| <issue id="GradleDependency" severity="ignore" />
64+
|</mint>
65+
""".trimMargin()
66+
val (updatedXml, problems) = attemptGettingLintXmlWithMissingRules(lintXmlContent = input)
67+
updatedXml shouldBe null
68+
problems.size shouldBe 1
69+
assertTrue(problems.single().issue is LintUpdatingIssue.ParsingFailure)
2070
}
2171

22-
private fun `test lint updater`(directory: File) {
23-
require(directory.isDirectory)
24-
val input = directory.resolve("input.xml").readText()
25-
val expected = directory.resolve("expected.xml").readText()
26-
updateLintXml(input).trim() shouldBe expected.trim()
72+
@Test
73+
fun `improper xml lint file returns a problem`() {
74+
val input = """
75+
|<?xml version="1.0" encoding="UTF-8"?>
76+
|<lint>
77+
""".trimMargin()
78+
val (updatedXml, problems) = attemptGettingLintXmlWithMissingRules(lintXmlContent = input)
79+
updatedXml shouldBe null
80+
problems.size shouldBe 1
81+
assertTrue(problems.single().issue is LintUpdatingIssue.ParsingFailure)
2782
}
2883
}
2984

0 commit comments

Comments
 (0)