From a312e301cf83c968d6a6b644ba922509f7c66445 Mon Sep 17 00:00:00 2001 From: Zac Sweers Date: Thu, 2 Nov 2023 13:56:38 -0400 Subject: [PATCH 1/7] Upstream MergeSarifReports --- .../slack/cli/sarif/MergeSarifReports.kt | 304 ++++++++++++++++++ 1 file changed, 304 insertions(+) create mode 100644 src/main/kotlin/slack/cli/sarif/MergeSarifReports.kt diff --git a/src/main/kotlin/slack/cli/sarif/MergeSarifReports.kt b/src/main/kotlin/slack/cli/sarif/MergeSarifReports.kt new file mode 100644 index 0000000..a3577b1 --- /dev/null +++ b/src/main/kotlin/slack/cli/sarif/MergeSarifReports.kt @@ -0,0 +1,304 @@ +package slack.cli.sarif + +import com.github.ajalt.clikt.core.CliktCommand +import com.github.ajalt.clikt.parameters.options.flag +import com.github.ajalt.clikt.parameters.options.option +import com.github.ajalt.clikt.parameters.options.required +import com.github.ajalt.clikt.parameters.types.file +import io.github.detekt.sarif4k.SarifSchema210 +import io.github.detekt.sarif4k.SarifSerializer +import java.io.File +import kotlin.io.path.absolutePathString +import kotlin.system.exitProcess +import slack.cli.projectDirOption +import slack.cli.skipBuildAndCacheDirs + +public class MergeSarifReports : + CliktCommand( + help = "Merges all matching sarif reports into a single file for ease of uploading." + ) { + + private val projectDir by projectDirOption() + private val outputFile: File by option("--output-file").file().required() + private val filePrefix by option("--file-prefix").required() + private val verbose by option("--verbose", "-v").flag() + private val remapSrcRoots by + option( + "--remap-src-roots", + help = + "When enabled, remaps uri roots to include the subproject path (relative to the root project)." + ) + .flag() + private val removeUriPrefixes by + option( + "--remove-uri-prefixes", + help = + "When enabled, removes the root project directory from location uris such that they are only relative to the root project dir." + ) + .flag() + + private fun log(message: String) { + if (verbose) { + echo(message) + } + } + + private fun prepareOutput() { + if (outputFile.exists()) { + outputFile.delete() + } + outputFile.parentFile?.mkdirs() + } + + private fun findBuildFiles(): List { + log("Finding build files in ${projectDir.toFile().canonicalFile}") + val buildFiles = + projectDir + .toFile() + .canonicalFile + .walkTopDown() + .skipBuildAndCacheDirs() + .filter { it.name == "build.gradle.kts" } + .toList() + log("${buildFiles.size} build files found") + return buildFiles + } + + private fun String.prefixPathWith(prefix: String) = "$prefix/$this" + + private fun findSarifFiles(): List { + // Find build files first, this gives us an easy hook to then go looking in build/reports dirs. + // Otherwise we don't have a way to easily exclude populated build dirs that would take forever. + val buildFiles = findBuildFiles() + + log("Finding sarif files") + return buildFiles + .asSequence() + .flatMap { buildFile -> + val reportsDir = File(buildFile.parentFile, "build/reports") + if (reportsDir.exists()) { + reportsDir.walkTopDown().filter { + it.isFile && it.extension == "sarif" && it.nameWithoutExtension.startsWith(filePrefix) + } + } else { + emptySequence() + } + } + .toList() + } + + /** + * Remaps the srcRoots in the sarif file to be relative to the _root_ project root. + * + * This is necessary because, when running lint on a standalone module, the source roots are + * relative to that _module_'s directory. However, we want the merged sarif to be relative to the + * root project directory. + * + * This function performs that in two steps: + * 1. Remap the `Run.originalUriBaseIds.%SRCROOT%` to be the root project dir ([projectDir]). + * 2. Remap the `Result.locations.physicalLocation.artifactLocation.uri` to be relative to the + * root project dir by prefixing the path with the module name. + * + * ## Example + * + * If we have a project with the following structure: + * ``` + * apps/app-core/src/main/java/com/example/app/MainActivity.kt + * libraries/lib/src/main/java/com/example/app/LibActivity.kt + * ``` + * + * The standard sarif report would denote paths like this: + * ``` + * src/main/java/com/example/app/MainActivity.kt + * src/main/java/com/example/app/LibActivity.kt + * ``` + * + * And what we actually want is this: + * ``` + * apps/app-core/src/main/java/com/example/app/MainActivity.kt + * libraries/lib/src/main/java/com/example/app/LibActivity.kt + * ``` + */ + private fun SarifSchema210.remapSrcRoots(sarifFile: File): SarifSchema210 { + // /─────────────────────────────────┐ + // build/─────────────────────┐ │ + // reports/──────┐ │ │ + // ▼ ▼ ▼ + val module = sarifFile.parentFile.parentFile.parentFile + check(File(module, "build.gradle.kts").exists()) { + "Expected to find build.gradle.kts in $module" + } + val modulePrefix = module.toRelativeString(projectDir.toFile()) + return copy( + runs = + runs.map { run -> + val originalUri = run.originalURIBaseIDS!!.getValue(SRC_ROOT) + run.copy( + // Remap "%SRCROOT%" to be the root project dir. + originalURIBaseIDS = + mapOf( + SRC_ROOT to + originalUri.copy( + uri = "file://${projectDir + .toFile().canonicalPath}/" + ) + ), + // Remap results to add the module prefix to the artifactLocation.uri. + results = + run.results?.map { result -> + result.copy( + locations = + result.locations?.map { location -> + location.copy( + physicalLocation = + location.physicalLocation?.copy( + artifactLocation = + location.physicalLocation + ?.artifactLocation + ?.copy( + uri = + location.physicalLocation + ?.artifactLocation + ?.uri + ?.prefixPathWith(modulePrefix) + ) + ) + ) + } + ) + } + ) + } + ) + } + + /** Removes the [projectDir] prefix from location URIs */ + private fun SarifSchema210.remapUris(): SarifSchema210 { + return copy( + runs = + runs.map { run -> + run.copy( + results = + run.results?.map { result -> + result.copy( + locations = + result.locations?.map { location -> + location.copy( + physicalLocation = + location.physicalLocation?.let { physicalLocation -> + physicalLocation.copy( + artifactLocation = + physicalLocation.artifactLocation?.let { artifactLocation -> + artifactLocation.copy( + uri = + artifactLocation.uri + ?.removePrefix(projectDir.absolutePathString()) + ?.removePrefix("/") + ) + } + ) + } + ) + } + ) + } + ) + } + ) + } + + private fun merge(inputs: List) { + log("Parsing ${inputs.size} sarif files") + val sarifs = + inputs.map { sarifFile -> + log("Parsing $sarifFile") + val parsed = SarifSerializer.fromJson(sarifFile.readText()) + if (parsed.runs.isEmpty() || parsed.runs[0].results.orEmpty().isEmpty()) { + return@map parsed + } + if (remapSrcRoots) { + parsed.remapSrcRoots(sarifFile) + } else if (removeUriPrefixes) { + parsed.remapUris() + } else { + parsed + } + } + + log("Merging ${inputs.size} sarif files") + val sortedMergedRules = + sarifs + .flatMap { it.runs.single().tool.driver.rules.orEmpty() } + .associateBy { it.id } + .toSortedMap() + val mergedResults = + sarifs + .flatMap { it.runs.single().results.orEmpty() } + // Some projects produce multiple reports for different variants, so we need to + // de-dupe. + .distinct() + .also { echo("Merged ${it.size} results") } + + // Update rule.ruleIndex to match the index in rulesToAdd + val ruleIndicesById = + sortedMergedRules.entries.withIndex().associate { (index, entry) -> entry.key to index } + val correctedResults = + mergedResults + .map { result -> + val ruleId = result.ruleID + val ruleIndex = ruleIndicesById.getValue(ruleId) + result.copy(ruleIndex = ruleIndex.toLong()) + } + .sortedWith( + compareBy( + { it.ruleIndex }, + { it.ruleID }, + { it.locations?.firstOrNull()?.physicalLocation?.artifactLocation?.uri }, + { it.locations?.firstOrNull()?.physicalLocation?.region?.startLine }, + { it.locations?.firstOrNull()?.physicalLocation?.region?.startColumn }, + { it.locations?.firstOrNull()?.physicalLocation?.region?.endLine }, + { it.locations?.firstOrNull()?.physicalLocation?.region?.endColumn }, + { it.message.text }, + ) + ) + + val sarifToUse = + if (removeUriPrefixes) { + // Just use the first if we don't care about originalUriBaseIDs + sarifs.first() + } else { + // Pick a sarif file to use as the base for the merged sarif file. We want one that has an + // `originalURIBaseIDS` too since parsing possibly uses this. + sarifs.find { it.runs.firstOrNull()?.originalURIBaseIDS?.isNotEmpty() == true } + ?: error("No sarif files had originalURIBaseIDS set, can't merge") + } + + // Note: we don't sort these results by anything currently (location, etc), but maybe some day + // we should if it matters for caching + val runToCopy = sarifToUse.runs.single() + val mergedTool = + runToCopy.tool.copy( + driver = runToCopy.tool.driver.copy(rules = sortedMergedRules.values.toList()) + ) + + val mergedSarif = + sarifToUse.copy(runs = listOf(runToCopy.copy(tool = mergedTool, results = correctedResults))) + + log("Writing merged sarif to $outputFile") + prepareOutput() + outputFile.writeText(SarifSerializer.toJson(mergedSarif)) + } + + override fun run() { + val sarifFiles = findSarifFiles() + if (sarifFiles.isEmpty()) { + log("No sarif files found! Did you run lint/detekt first?") + exitProcess(1) + } + merge(sarifFiles) + } + + private companion object { + private const val SRC_ROOT = "%SRCROOT%" + } +} \ No newline at end of file From 7782df6642e9d4f716a0336c086a26ad0fc4fbca Mon Sep 17 00:00:00 2001 From: Zac Sweers Date: Thu, 2 Nov 2023 14:32:34 -0400 Subject: [PATCH 2/7] Spotless --- .../slack/cli/sarif/MergeSarifReports.kt | 161 ++++++++++-------- 1 file changed, 88 insertions(+), 73 deletions(-) diff --git a/src/main/kotlin/slack/cli/sarif/MergeSarifReports.kt b/src/main/kotlin/slack/cli/sarif/MergeSarifReports.kt index a3577b1..eb877bb 100644 --- a/src/main/kotlin/slack/cli/sarif/MergeSarifReports.kt +++ b/src/main/kotlin/slack/cli/sarif/MergeSarifReports.kt @@ -1,3 +1,18 @@ +/* + * Copyright (C) 2023 Slack Technologies, LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ package slack.cli.sarif import com.github.ajalt.clikt.core.CliktCommand @@ -23,19 +38,19 @@ public class MergeSarifReports : private val filePrefix by option("--file-prefix").required() private val verbose by option("--verbose", "-v").flag() private val remapSrcRoots by - option( - "--remap-src-roots", - help = - "When enabled, remaps uri roots to include the subproject path (relative to the root project)." - ) - .flag() + option( + "--remap-src-roots", + help = + "When enabled, remaps uri roots to include the subproject path (relative to the root project)." + ) + .flag() private val removeUriPrefixes by - option( - "--remove-uri-prefixes", - help = - "When enabled, removes the root project directory from location uris such that they are only relative to the root project dir." - ) - .flag() + option( + "--remove-uri-prefixes", + help = + "When enabled, removes the root project directory from location uris such that they are only relative to the root project dir." + ) + .flag() private fun log(message: String) { if (verbose) { @@ -131,44 +146,44 @@ public class MergeSarifReports : val modulePrefix = module.toRelativeString(projectDir.toFile()) return copy( runs = - runs.map { run -> - val originalUri = run.originalURIBaseIDS!!.getValue(SRC_ROOT) - run.copy( - // Remap "%SRCROOT%" to be the root project dir. - originalURIBaseIDS = - mapOf( - SRC_ROOT to - originalUri.copy( - uri = "file://${projectDir + runs.map { run -> + val originalUri = run.originalURIBaseIDS!!.getValue(SRC_ROOT) + run.copy( + // Remap "%SRCROOT%" to be the root project dir. + originalURIBaseIDS = + mapOf( + SRC_ROOT to + originalUri.copy( + uri = "file://${projectDir .toFile().canonicalPath}/" - ) - ), - // Remap results to add the module prefix to the artifactLocation.uri. - results = - run.results?.map { result -> - result.copy( - locations = - result.locations?.map { location -> - location.copy( - physicalLocation = - location.physicalLocation?.copy( - artifactLocation = - location.physicalLocation - ?.artifactLocation - ?.copy( - uri = - location.physicalLocation - ?.artifactLocation - ?.uri - ?.prefixPathWith(modulePrefix) - ) ) + ), + // Remap results to add the module prefix to the artifactLocation.uri. + results = + run.results?.map { result -> + result.copy( + locations = + result.locations?.map { location -> + location.copy( + physicalLocation = + location.physicalLocation?.copy( + artifactLocation = + location.physicalLocation + ?.artifactLocation + ?.copy( + uri = + location.physicalLocation + ?.artifactLocation + ?.uri + ?.prefixPathWith(modulePrefix) + ) + ) + ) + } ) } - ) - } - ) - } + ) + } ) } @@ -176,34 +191,34 @@ public class MergeSarifReports : private fun SarifSchema210.remapUris(): SarifSchema210 { return copy( runs = - runs.map { run -> - run.copy( - results = - run.results?.map { result -> - result.copy( - locations = - result.locations?.map { location -> - location.copy( - physicalLocation = - location.physicalLocation?.let { physicalLocation -> - physicalLocation.copy( - artifactLocation = - physicalLocation.artifactLocation?.let { artifactLocation -> - artifactLocation.copy( - uri = - artifactLocation.uri - ?.removePrefix(projectDir.absolutePathString()) - ?.removePrefix("/") - ) - } - ) - } + runs.map { run -> + run.copy( + results = + run.results?.map { result -> + result.copy( + locations = + result.locations?.map { location -> + location.copy( + physicalLocation = + location.physicalLocation?.let { physicalLocation -> + physicalLocation.copy( + artifactLocation = + physicalLocation.artifactLocation?.let { artifactLocation -> + artifactLocation.copy( + uri = + artifactLocation.uri + ?.removePrefix(projectDir.absolutePathString()) + ?.removePrefix("/") + ) + } + ) + } + ) + } ) } - ) - } - ) - } + ) + } ) } @@ -301,4 +316,4 @@ public class MergeSarifReports : private companion object { private const val SRC_ROOT = "%SRCROOT%" } -} \ No newline at end of file +} From 4e999184e57a9406218549901914f2f5c454b3a1 Mon Sep 17 00:00:00 2001 From: Zac Sweers Date: Thu, 2 Nov 2023 14:38:03 -0400 Subject: [PATCH 3/7] Split function --- .../slack/cli/sarif/MergeSarifReports.kt | 33 ++++++++++--------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/src/main/kotlin/slack/cli/sarif/MergeSarifReports.kt b/src/main/kotlin/slack/cli/sarif/MergeSarifReports.kt index eb877bb..9d2a84b 100644 --- a/src/main/kotlin/slack/cli/sarif/MergeSarifReports.kt +++ b/src/main/kotlin/slack/cli/sarif/MergeSarifReports.kt @@ -222,23 +222,26 @@ public class MergeSarifReports : ) } + private fun loadSarifs(inputs: List): List { + return inputs.map { sarifFile -> + log("Parsing $sarifFile") + val parsed = SarifSerializer.fromJson(sarifFile.readText()) + if (parsed.runs.isEmpty() || parsed.runs[0].results.orEmpty().isEmpty()) { + return@map parsed + } + if (remapSrcRoots) { + parsed.remapSrcRoots(sarifFile) + } else if (removeUriPrefixes) { + parsed.remapUris() + } else { + parsed + } + } + } + private fun merge(inputs: List) { log("Parsing ${inputs.size} sarif files") - val sarifs = - inputs.map { sarifFile -> - log("Parsing $sarifFile") - val parsed = SarifSerializer.fromJson(sarifFile.readText()) - if (parsed.runs.isEmpty() || parsed.runs[0].results.orEmpty().isEmpty()) { - return@map parsed - } - if (remapSrcRoots) { - parsed.remapSrcRoots(sarifFile) - } else if (removeUriPrefixes) { - parsed.remapUris() - } else { - parsed - } - } + val sarifs = loadSarifs(inputs) log("Merging ${inputs.size} sarif files") val sortedMergedRules = From 72e5b1fd2f376cedb8784b76b601d2c95cd56be9 Mon Sep 17 00:00:00 2001 From: Zac Sweers Date: Thu, 2 Nov 2023 14:38:18 -0400 Subject: [PATCH 4/7] Detekt --- src/main/kotlin/slack/cli/sarif/MergeSarifReports.kt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/kotlin/slack/cli/sarif/MergeSarifReports.kt b/src/main/kotlin/slack/cli/sarif/MergeSarifReports.kt index 9d2a84b..71faa21 100644 --- a/src/main/kotlin/slack/cli/sarif/MergeSarifReports.kt +++ b/src/main/kotlin/slack/cli/sarif/MergeSarifReports.kt @@ -48,7 +48,8 @@ public class MergeSarifReports : option( "--remove-uri-prefixes", help = - "When enabled, removes the root project directory from location uris such that they are only relative to the root project dir." + "When enabled, removes the root project directory from location uris such that they are only " + + "relative to the root project dir." ) .flag() From ef48d6e07c709f8d5a0a5bd414e2bb17aa526797 Mon Sep 17 00:00:00 2001 From: Zac Sweers Date: Thu, 2 Nov 2023 14:44:02 -0400 Subject: [PATCH 5/7] API dump --- api/kotlin-cli-util.api | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/api/kotlin-cli-util.api b/api/kotlin-cli-util.api index 4ca7b42..8010207 100644 --- a/api/kotlin-cli-util.api +++ b/api/kotlin-cli-util.api @@ -42,6 +42,11 @@ public final class slack/cli/lint/LintBaselineMergerCli : com/github/ajalt/clikt public fun run ()V } +public final class slack/cli/sarif/MergeSarifReports : com/github/ajalt/clikt/core/CliktCommand { + public fun ()V + public fun run ()V +} + public final class slack/cli/shellsentry/AnalysisResult { public fun (Ljava/lang/String;Ljava/lang/String;Lslack/cli/shellsentry/RetrySignal;ILkotlin/jvm/functions/Function1;)V public final fun component1 ()Ljava/lang/String; From ffaefcd8489189bc9f3db842e1d5b2c18f1ed496 Mon Sep 17 00:00:00 2001 From: Zac Sweers Date: Thu, 2 Nov 2023 15:00:42 -0400 Subject: [PATCH 6/7] De-flake --- build.gradle.kts | 12 ++++++++++++ gradle/libs.versions.toml | 1 + 2 files changed, 13 insertions(+) diff --git a/build.gradle.kts b/build.gradle.kts index 4d6cdce..6c8f3af 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -27,6 +27,7 @@ plugins { alias(libs.plugins.binaryCompatibilityValidator) alias(libs.plugins.moshix) alias(libs.plugins.kotlin.serialization) + alias(libs.plugins.retry) } spotless { @@ -85,6 +86,17 @@ kotlin { moshi { enableSealed.set(true) } +// We have a couple flaky tests on CI right now +if (System.getenv("CI") != null) { + tasks.withType().configureEach { + retry { + maxRetries.set(2) + maxFailures.set(20) + failOnPassedAfterRetry.set(false) + } + } +} + dependencies { api(libs.clikt) implementation(libs.tikxml.htmlEscape) diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 529aebe..0404ee8 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -18,6 +18,7 @@ kotlin-jvm = { id = "org.jetbrains.kotlin.jvm", version.ref = "kotlin" } kotlin-serialization = { id = "org.jetbrains.kotlin.plugin.serialization", version.ref = "kotlin" } spotless = { id = "com.diffplug.spotless", version = "6.22.0" } binaryCompatibilityValidator = { id = "org.jetbrains.kotlinx.binary-compatibility-validator", version = "0.13.2" } +retry = { id = "org.gradle.test-retry", version = "1.5.6" } [libraries] bugsnag = "com.bugsnag:bugsnag:3.7.1" From ad983c10b927f760a5a7e53185594434e57744bc Mon Sep 17 00:00:00 2001 From: Zac Sweers Date: Thu, 2 Nov 2023 15:19:38 -0400 Subject: [PATCH 7/7] Add merge_group --- .github/workflows/ci.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3c9e7d9..5d31050 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -10,6 +10,7 @@ on: - '*.md' # Always run on PRs pull_request: + merge_group: jobs: build: