diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4ff43be9..961df0c3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -40,6 +40,21 @@ jobs: id: spotless-check run: ./gradlew spotlessCheck + # Detekt had never run in ANY workflow - it was a local-only, advisory tool, and with + # `ignoreFailures = true` it could not fail even there. Both halves are fixed now, so it + # belongs on the lane that already has Gradle set up and always runs. + # + # `success() || failure()` rather than plain ordering: a Spotless failure must not hide a + # Detekt failure, or fixing one just uncovers the other on the next push. + # + # Added as a step on an existing job, deliberately. "CI Gate" lists jobs, not steps, so this + # needs no branch-protection change (AGENTS.md: add new lanes to ci-gate needs, not to the + # protection rule). + - name: Run Detekt + id: detekt-check + if: success() || failure() + run: ./gradlew detekt + # Lives here because this lane always runs and already has Gradle set up. Guards the one # verification gap CI is otherwise blind to: artifacts only Android Studio's sync resolves. # Without it a stale ledger is invisible until a human hits it in the IDE (ADR 0007). @@ -74,6 +89,32 @@ jobs: echo "" >> $GITHUB_STEP_SUMMARY echo "Or trigger it manually from the Actions tab (pick this branch in the \"Run workflow\" dropdown)." >> $GITHUB_STEP_SUMMARY + # Deliberately worded differently from the Format Fix hint above. Spotless output is a + # mechanical transform, so "run the fixer" is always the right advice. A Detekt baseline is a + # suppression, so the right advice is "fix the code", and the workflow is the exception. + - name: Detekt Hint + if: always() && steps.detekt-check.outcome == 'failure' + run: | + BRANCH_NAME="${{ github.head_ref || github.ref_name }}" + echo "### ๐Ÿ” Detekt Found New Issues" >> $GITHUB_STEP_SUMMARY + echo "" >> $GITHUB_STEP_SUMMARY + echo "The backlog that existed when this gate was adopted is already baselined, so a failure here means **new** findings. Fix them." >> $GITHUB_STEP_SUMMARY + echo "" >> $GITHUB_STEP_SUMMARY + echo "Reproduce locally:" >> $GITHUB_STEP_SUMMARY + echo "" >> $GITHUB_STEP_SUMMARY + echo "\`\`\`bash" >> $GITHUB_STEP_SUMMARY + echo "make lint" >> $GITHUB_STEP_SUMMARY + echo "\`\`\`" >> $GITHUB_STEP_SUMMARY + echo "" >> $GITHUB_STEP_SUMMARY + echo "If โ€” and only if โ€” the finding is pre-existing or a deliberate exception, re-baseline it. **This suppresses the rule, so the diff is the review**:" >> $GITHUB_STEP_SUMMARY + echo "" >> $GITHUB_STEP_SUMMARY + echo "\`\`\`bash" >> $GITHUB_STEP_SUMMARY + echo "make detekt-baseline MODULE=:the:module # locally, then commit the diff" >> $GITHUB_STEP_SUMMARY + echo "gh workflow run detekt_baseline.yml --ref $BRANCH_NAME # or on the branch" >> $GITHUB_STEP_SUMMARY + echo "\`\`\`" >> $GITHUB_STEP_SUMMARY + echo "" >> $GITHUB_STEP_SUMMARY + echo "Never re-baseline to silence a finding you just introduced โ€” that is how the gate stops working." >> $GITHUB_STEP_SUMMARY + # Change-scope detector with per-lane verdict adoption. The heavy test lanes (unit / screenshot # / instrumented) key their `if:` off one output each. Two levels: # diff --git a/.github/workflows/detekt_baseline.yml b/.github/workflows/detekt_baseline.yml new file mode 100644 index 00000000..4c54425d --- /dev/null +++ b/.github/workflows/detekt_baseline.yml @@ -0,0 +1,118 @@ +name: Detekt Baseline + +# Dispatch against a branch to regenerate the Detekt baselines and commit them to that same branch. +# The sibling of format_fix.yml, for the gate that replaced `ignoreFailures = true`. +# +# IT IS DELIBERATELY NOT AUTOMATIC, and that is the whole design note. spotlessApply is a +# deterministic mechanical transform, so format_fix.yml can run and commit with nothing for a +# reviewer to approve. A Detekt baseline is the opposite: it is a *suppression list*. Running this +# on every push, or on a red PR, would mean any new finding silences itself and the gate never +# fails again - the exact state this replaced, reached automatically instead of by a config flag. +# +# So: manual dispatch only, never on `push` or `pull_request`, and it prints what it suppressed +# into the job summary. The baseline files are committed, so the PR diff is the real review surface +# - a reviewer can see precisely which rules were added and object. +# +# When to dispatch it: +# - onboarding a module that had no baseline +# - after genuinely FIXING findings, to shrink a baseline +# - a deliberate, reviewed decision to grandfather something +# +# When NOT to: CI went red on a finding you just introduced. Fix the code. That is the point. +# +# Push uses the write deploy key, not GITHUB_TOKEN, for the reason in ADR 0007: events created with +# GITHUB_TOKEN do not trigger workflow runs, so the commit would move an open PR's head to a SHA CI +# never runs on, leaving "CI Gate" pending forever and `gh pr merge --auto` hanging silently. + +on: + workflow_dispatch: {} + +permissions: + contents: read + +concurrency: + group: detekt-baseline-${{ github.ref_name }} + cancel-in-progress: false + +jobs: + baseline: + name: Regenerate Detekt baselines on the branch + runs-on: ubuntu-latest + steps: + - name: Refuse to run on the default branch + # Baselines suppress findings; doing that on master without review is precisely the failure + # this workflow's design is trying to avoid. Branch protection would reject the push anyway, + # so fail here with a message that says why. + if: github.ref_name == github.event.repository.default_branch + run: | + echo "::error::Dispatch this against a feature branch, not ${{ github.ref_name }}." + echo "It commits directly to the branch it runs on, and a baseline is a suppression." + exit 1 + + - name: Checkout branch + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + ref: ${{ github.ref_name }} + ssh-key: ${{ secrets.VERIFICATION_METADATA_DEPLOY_KEY }} + + - name: Set up JDK 23 + uses: actions/setup-java@03ad4de0992f5dab5e18fcb136590ce7c4a0ac95 # v5 + with: + java-version: '23' + distribution: 'temurin' + + - name: Setup Gradle + uses: gradle/actions/setup-gradle@3f131e8634966bd73d06cc69884922b02e6faf92 # v6 + with: + gradle-version: wrapper + + - name: Make gradlew executable + run: chmod +x gradlew + + - name: Regenerate Detekt baselines + run: ./gradlew detektBaseline + + - name: Summarise what changed, then commit + run: | + git config user.name "github-actions[bot]" + git config user.email "github-actions[bot]@users.noreply.github.com" + + # -u, not -A: only files already tracked. The repo deliberately carries untracked + # local-only content (planning notes) that must never be committed. + git add -u + # Newly created baselines are untracked, so add those explicitly by name. + git ls-files --others --exclude-standard -- '**/detekt-baseline.xml' \ + | xargs -r git add -- + + if git diff --cached --quiet; then + echo "No baseline changes - Detekt findings already match the committed baselines." + echo "No baseline changes." >> "$GITHUB_STEP_SUMMARY" + exit 0 + fi + + # Make the suppression visible in the run itself, not only in the eventual diff. Counting + # added lines per file is enough to see at a glance whether this quietly grew. + { + echo "## Detekt baseline changes" + echo "" + echo "A baseline is a suppression list. Review these before merging." + echo "" + echo "| file | rules added | rules removed |" + echo "|---|---|---|" + for f in $(git diff --cached --name-only -- '**/detekt-baseline.xml'); do + added=$(git diff --cached -- "$f" | grep -c '^+.*' || true) + removed=$(git diff --cached -- "$f" | grep -c '^-.*' || true) + echo "| \`$f\` | $added | $removed |" + done + echo "" + echo "Rules newly suppressed:" + echo "" + echo '```' + git diff --cached -- '**/detekt-baseline.xml' \ + | grep '^+.*' | sed 's/^+\s*//' | sort | uniq -c | sort -rn || true + echo '```' + } >> "$GITHUB_STEP_SUMMARY" + + git commit -m "chore: regenerate Detekt baselines" + git push origin HEAD:"$GITHUB_REF_NAME" + echo "Pushed a baseline commit to $GITHUB_REF_NAME - review the diff before merging." diff --git a/AGENTS.md b/AGENTS.md index 00d229ef..81f9a49f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -150,6 +150,7 @@ left `make konsist` green at "3 up-to-date". The whole gate, not one rule. | **Non-goals** โ€” auth, cert pinning, encrypted storage, integrity/anti-tamper, push, server-side `available` sync, a shipped analytics/crash SDK, consent flows, automatic retry, multi-process. All declined because the premise is absent (the backend isn't ours, is read-only, and is unauthenticated), each with its reopen trigger. Read it before "adding what a real app has" โ€” and delete a row in the same PR that builds it | `docs/adr/0010` | | Dispatchers are chosen where the work is, not where the coroutine starts. ViewModels use a bare `viewModelScope.launch { }` โ€” Room and Retrofit suspend calls are already main-safe, so an IO hop only moves state assignment off Main. Inject `CoroutineDispatcherProvider` only where a class actually calls `withContext`/`flowOn` (a blocking SDK, CPU work). StrictMode in debug is the detector that makes this safe | `docs/adr/0012` | | Build time is budgeted in `config/build-time-budget.txt` and measured **locally** by `make build-budget`, never in CI โ€” a CI wall-clock number measures which runner the job drew. Clean build 37s cold / 4s warm; a deep ABI change costs only 1.11x a leaf one, so **per-build overhead dominates, not compilation** โ€” do not justify a new module by build speed | `docs/adr/0011` | +| Convention plugins stay **precompiled script plugins**; the project moved to them from binary plugins on purpose. Do not propose converting back on the usual grounds โ€” precompiled scripts *can* share helper functions (see `AndroidCommon.kt`), and the configuration-time argument is largely neutralised by the configuration cache. Converting is a *measurement* task with a written method, not a judgement call. Declarative Gradle sits on top of this, not against it | `docs/adr/0013` | Also settled, without an ADR: @@ -177,10 +178,18 @@ declaring done. 4. **Screenshots** โ€” `make screenshot-verify` (record with `make screenshot-record` and inspect the PNGs; they are your eyes on the UI) 5. **Lint / format** โ€” `make lint`, `make format`, and **`make android-lint` whenever resources - change**. `make lint` is Detekt only; the Android Lint gate CI runs is `make android-lint` - (`:app:lintDebug`, checkDependencies across the whole graph). A string added to - `values/strings.xml` without its `values-fr` / `values-es` siblings passes every other rung and - fails CI with `MissingTranslation` โ€” that is how PR #140 broke master. + change**. These are three separate gates and all three now fail: + - `make lint` is **Detekt**, and since the gate was adopted it **fails on new findings** and runs + in CI on the `format-check` job. The backlog present at adoption is frozen in per-module + `detekt-baseline.xml`; burn those down, and **never regenerate one to bury a new finding** + (same rule as `lint-baseline.xml`). To grandfather something deliberately: + `make detekt-baseline MODULE=:foo`, or dispatch `detekt_baseline.yml` against the branch โ€” the + committed diff is the review. Test and `androidTest` sources are scanned too. + - `make android-lint` is **Android Lint** (`:app:lintDebug`, checkDependencies across the whole + graph), gated by `app/lint-baseline.xml`. A string added to `values/strings.xml` without its + `values-fr` / `values-es` siblings passes every other rung and fails CI with + `MissingTranslation` โ€” that is how PR #140 broke master. + - `make format` is Spotless; CI runs `spotlessCheck` and `format_fix.yml` can apply it for you. 6. **Device** โ€” instrumented tests, install, logcat: use the `billionbeers-android` skill, not ad-hoc `adb` diff --git a/Makefile b/Makefile index 9ccf75c1..143dbf36 100644 --- a/Makefile +++ b/Makefile @@ -17,7 +17,7 @@ UI_TEST_PREFIX = $(if $(MODULE_TRIMMED),$(MODULE_TRIMMED):,:app:) # Wrapper for Gradle to support build-brief (bb) or rtk if available GRADLE_RUNNER := $(shell if command -v bb >/dev/null 2>&1; then echo "bb ./gradlew"; elif command -v build-brief >/dev/null 2>&1; then echo "build-brief --gradle ./gradlew"; elif command -v rtk >/dev/null 2>&1; then echo "rtk ./gradlew"; else echo "./gradlew"; fi) -.PHONY: help setup setup-ai-tools update-android-skills build bundle-release install clean test konsist compose-metrics ui-test screenshot-record screenshot-verify screenshot-clean lint android-lint format check check-duplicates check-unused-deps dependency-guard dependency-guard-baseline verification-metadata health benchmark-micro benchmark-macro benchmark-check generate-baseline gradle-benchmark build-budget build-budget-check jacoco-report coverage-check install-profiler install-diffuse new-feature-module new-dev-app play-listing-check play-listing-capture play-listing-reset store-frames +.PHONY: detekt-baseline help setup setup-ai-tools update-android-skills build bundle-release install clean test konsist compose-metrics ui-test screenshot-record screenshot-verify screenshot-clean lint android-lint format check check-duplicates check-unused-deps dependency-guard dependency-guard-baseline verification-metadata health benchmark-micro benchmark-macro benchmark-check generate-baseline gradle-benchmark build-budget build-budget-check jacoco-report coverage-check install-profiler install-diffuse new-feature-module new-dev-app play-listing-check play-listing-capture play-listing-reset store-frames help: ## Show this help message. @echo "\n๐Ÿ“Š BillionBeers Makefile Help" @@ -144,7 +144,7 @@ lint: ## Run static analysis (Detekt). android-lint: ## Run Android Lint over the app and its whole library graph (checkDependencies), gated by app/lint-baseline.xml. $(GRADLE_RUNNER) :app:lintDebug -detekt-baseline: ## Update Detekt baselines for all modules. +detekt-baseline: ## Re-baseline Detekt (all modules, or MODULE=:foo). ALWAYS review the diff - a baseline is a suppression, and regenerating one to silence a NEW finding buries it. $(GRADLE_RUNNER) $(MODULE_PREFIX)detektBaseline format: ## Apply code formatting (Spotless). diff --git a/app/detekt-baseline.xml b/app/detekt-baseline.xml index 48f844ce..eb5785bf 100644 --- a/app/detekt-baseline.xml +++ b/app/detekt-baseline.xml @@ -2,8 +2,7 @@ - ForbiddenComment:MainCoroutineScopeRule.kt$// TODO: refactor - TopLevelPropertyNaming:TestingConstants.kt$const val fakeErrorName = "Error getting list of beers" - WildcardImport:MainCoroutineScopeRule.kt$import kotlinx.coroutines.test.* + EmptyFunctionBlock:FakeSplitInstallManager.kt$FakeSplitInstallManager${} + MatchingDeclarationName:LocalDataSourceTest2.kt$LocalDataSourceTest diff --git a/beer_data/detekt-baseline.xml b/beer_data/detekt-baseline.xml index f27b1d1c..786adc31 100644 --- a/beer_data/detekt-baseline.xml +++ b/beer_data/detekt-baseline.xml @@ -2,7 +2,7 @@ - SwallowedException:BeersRepositoryImpl.kt$BeersRepositoryImpl$e: Exception - TooGenericExceptionCaught:BeersRepositoryImpl.kt$BeersRepositoryImpl$e: Exception + CyclomaticComplexMethod:BeersMapper.kt$BeersMapper$fun fromBeersApiResponseItemToBeer(response: BeersApiResponseItem?): Beer + TooManyFunctions:BeersRepositoryImpl.kt$BeersRepositoryImpl : BeersRepository diff --git a/beer_database/detekt-baseline.xml b/beer_database/detekt-baseline.xml index ec31a94c..0c06ecee 100644 --- a/beer_database/detekt-baseline.xml +++ b/beer_database/detekt-baseline.xml @@ -2,7 +2,9 @@ - LongParameterList:BeersDao.kt$BeersDao$( id: String, name: String, tagline: String, description: String, imageUrl: String, abv: Double, ibu: Double, foodPairing: String, ) + LongParameterList:BeersDao.kt$BeersDao$( id: String, name: String, tagline: String, description: String, imageUrl: String, abv: Double, ibu: Double, foodPairing: String, styleName: String, breweryName: String, srm: Int?, releasedYear: Int?, minServingTemperature: Int?, maxServingTemperature: Int?, fermentationMethod: String, ingredients: String, recommendedGlasses: String, ) + MagicNumber:Migrations.kt$<no name provided>$3 MatchingDeclarationName:Converter.kt$Converters + TooManyFunctions:BeersDao.kt$BeersDao diff --git a/beer_network/detekt-baseline.xml b/beer_network/detekt-baseline.xml new file mode 100644 index 00000000..edd8e04b --- /dev/null +++ b/beer_network/detekt-baseline.xml @@ -0,0 +1,7 @@ + + + + + LongParameterList:BeersService.kt$BeersService$( @Query("_page") page: Int, @Query("_limit") perPage: Int = DEFAULT_ITEMS_PER_PAGE, @Query("translations.language.code") languageCode: String = DEFAULT_LANGUAGE_CODE, // Filters; null omits the param entirely, so the catalog's unfiltered fetch stays byte- // identical on the wire. The server ANDs whichever are present. @Query("q") search: String? = null, @Query("typology.id") typologyId: String? = null, @Query("brewery.id") breweryId: String? = null, ) + + diff --git a/beerdomain/api/detekt-baseline.xml b/beerdomain/api/detekt-baseline.xml new file mode 100644 index 00000000..1fa7c171 --- /dev/null +++ b/beerdomain/api/detekt-baseline.xml @@ -0,0 +1,7 @@ + + + + + TooManyFunctions:BeersRepository.kt$BeersRepository + + diff --git a/beerdomain/fakes/detekt-baseline.xml b/beerdomain/fakes/detekt-baseline.xml index 1e519738..0d2a531c 100644 --- a/beerdomain/fakes/detekt-baseline.xml +++ b/beerdomain/fakes/detekt-baseline.xml @@ -2,6 +2,7 @@ - EmptyFunctionBlock:FakeBeersRepository.kt$FakeBeersRepository${} + TooManyFunctions:FakeBeersRepository.kt$FakeBeersRepository : BeersRepository + TopLevelPropertyNaming:BeerFixtures.kt$const val fakeErrorName = "Error getting list of beers" diff --git a/build-logic/convention/src/main/kotlin/billionbeers.detekt.gradle.kts b/build-logic/convention/src/main/kotlin/billionbeers.detekt.gradle.kts index 5f7439a3..25b6d1ba 100644 --- a/build-logic/convention/src/main/kotlin/billionbeers.detekt.gradle.kts +++ b/build-logic/convention/src/main/kotlin/billionbeers.detekt.gradle.kts @@ -9,12 +9,34 @@ val libs = the() configure { toolVersion = libs.versions.detekt.get() - source.setFrom(files("src/main/java", "src/main/kotlin")) + // Test sources are scanned too. They were excluded, so a third of the repo's Kotlin - the part + // that decides whether the rest is correct - was invisible to static analysis even in the + // advisory mode this used to run in. Measured cost of adding them: 13 findings across 6 + // modules, all grandfathered into the per-module baselines at adoption. + source.setFrom( + files( + "src/main/java", + "src/main/kotlin", + "src/test/java", + "src/test/kotlin", + "src/androidTest/java", + "src/androidTest/kotlin", + ) + ) config.setFrom(files("$rootDir/config/detekt/detekt.yml")) baseline = file("detekt-baseline.xml").takeIf { it.exists() } buildUponDefaultConfig = true autoCorrect = false - ignoreFailures = true + // A NEW finding fails the build; the backlog present at adoption does not, because it is + // frozen in each module's detekt-baseline.xml. This is the same shape as the Android Lint gate + // (`abortOnError = true` over `app/lint-baseline.xml`), and AGENTS.md ยง5 already advertises + // `make lint` as a rung of the verification ladder - with ignoreFailures it was output, not a + // gate, and could not fail on anything however bad. + // + // Burn the baselines down over time. Never regenerate one to bury a regression: that is the + // rule AGENTS.md states for lint-baseline.xml and it applies identically here. Regenerate only + // when the finding is genuinely fixed, and only for the module you fixed. + ignoreFailures = false } // Detekt supports max java 22 for now @@ -24,4 +46,10 @@ tasks.withType().configureEach { tasks.withType().configureEach { jvmTarget = DETEKT_JAVA_VERSION + // The extension's `baseline` is null until the file exists, which is correct for the *check* + // task - a missing baseline should mean "no exemptions", not an error. But it left the task + // that CREATES the baseline with nowhere to write, so `detektBaseline` failed with "property + // 'baseline' doesn't have a configured value" on precisely the modules that needed one. Give + // the create task the path unconditionally; it is an output, not an input. + baseline.set(layout.projectDirectory.file("detekt-baseline.xml")) } diff --git a/catalog-processor/detekt-baseline.xml b/catalog-processor/detekt-baseline.xml new file mode 100644 index 00000000..0ab6758a --- /dev/null +++ b/catalog-processor/detekt-baseline.xml @@ -0,0 +1,12 @@ + + + + + CyclomaticComplexMethod:CatalogProcessor.kt$CatalogProcessor$private fun generateWrapper( func: KSFunctionDeclaration, wrapperName: String, demoContainer: String, ) + LongMethod:CatalogProcessor.kt$CatalogProcessor$private fun generateWrapper( func: KSFunctionDeclaration, wrapperName: String, demoContainer: String, ) + MaxLineLength:CatalogProcessor.kt$CatalogProcessor$"TextField(value = $paramName.toString(), onValueChange = { $paramName = it.toIntOrNull() ?: 0 }, label = { Text(%S) }, keyboardOptions = KeyboardOptions(keyboardType = KeyboardType.Number))" + WildcardImport:CatalogProcessor.kt$import com.google.devtools.ksp.processing.* + WildcardImport:CatalogProcessor.kt$import com.google.devtools.ksp.symbol.* + WildcardImport:CatalogProcessor.kt$import com.squareup.kotlinpoet.* + + diff --git a/core-common/detekt-baseline.xml b/core-common/detekt-baseline.xml new file mode 100644 index 00000000..663623ea --- /dev/null +++ b/core-common/detekt-baseline.xml @@ -0,0 +1,13 @@ + + + + + CyclomaticComplexMethod:PagedListReducer.kt$PagedListReducer$fun reduce(items: List<T>, state: PagingState<E>): CommonUiState<PagedListUiModel<T>> + EmptyFunctionBlock:AnalyticsTracker.kt$NoOpAnalyticsTracker${} + EmptyFunctionBlock:Logger.kt$NoOpLogger${} + ReturnCount:PagingMediator.kt$PagingMediator$@Suppress("TooGenericExceptionCaught") override suspend fun loadNextPage() + TooGenericExceptionThrown:PagingMediatorTest.kt$PagingMediatorTest$throw RuntimeException("count failed") + TooGenericExceptionThrown:PagingMediatorTest.kt$PagingMediatorTest.Harness$throw RuntimeException("fetch $key failed") + TooGenericExceptionThrown:PagingMediatorTest.kt$PagingMediatorTest.RecordingStorage$throw RuntimeException("write failed") + + diff --git a/core/designsystem/detekt-baseline.xml b/core/designsystem/detekt-baseline.xml index 3b17b23d..9bacb7c6 100644 --- a/core/designsystem/detekt-baseline.xml +++ b/core/designsystem/detekt-baseline.xml @@ -21,7 +21,7 @@ MagicNumber:DialogWithProgressBar.kt$0.01f MagicNumber:DialogWithProgressBar.kt$50 MagicNumber:DialogWithProgressBar.kt$500 - MatchingDeclarationName:DialogWithProgressBar.kt$CatalogSettings + MagicNumber:DialogWithProgressBar.kt$DialogProgressProvider$0.5f MatchingDeclarationName:Spacing.kt$BillionBeersSpacing SwallowedException:ComposeExtensions.kt$e: Exception TooGenericExceptionCaught:ComposeExtensions.kt$e: Exception diff --git a/docs/adr/0013-convention-plugin-form.md b/docs/adr/0013-convention-plugin-form.md new file mode 100644 index 00000000..325cc3a5 --- /dev/null +++ b/docs/adr/0013-convention-plugin-form.md @@ -0,0 +1,91 @@ +# 0013: Convention plugins stay precompiled script plugins, and the alternative gets measured before it gets adopted + +## Status + +Accepted. + +## Context + +The build's conventions live in `build-logic/convention/src/main/kotlin` as **precompiled script +plugins** โ€” sixteen `billionbeers.*.gradle.kts` files whose plugin id is their filename. Two +**binary plugins** (`DuplicateClassesPlugin`, `UnusedDependenciesPlugin`) sit beside them as +ordinary `Plugin` classes, so the build already runs both forms and the wiring for either +is proven. + +This is not the original shape. The project moved *from* binary plugins *to* precompiled scripts +deliberately, on the grounds that they are simpler, easier to maintain, and faster. That history is +the reason this ADR exists: without it written down, the reverse migration looks like an obvious +improvement to anyone reading the code fresh, and it was in fact proposed on exactly that basis in +August 2026. + +The case for converting back is real but narrower than it first appears: + +- **The Gradle 9 accessor workaround would disappear.** Six conventions apply their siblings with + `apply(plugin = "billionbeers.โ€ฆ")` instead of a `plugins { }` block, to route around an + accessor-generation bug. Binary plugins generate no accessors, so the bug cannot apply and the + workaround could be deleted. +- **Build logic would become unit-testable** with `ProjectBuilder`/TestKit. In a repo that + mechanically enforces thirteen architecture invariants on application code, the build logic is + the only substantial untested part. +- **Configuration time.** Gradle generates a precompiled script plugin's class at configuration + time, and the widely-cited Now in Android figure is 12.7s โ†’ 0.76s on conversion. + +Two things weaken that third point specifically, which is the one usually used to justify the +migration: + +1. This build has `org.gradle.configuration-cache=true`. On a cache *hit* the configuration phase + is not executed at all, so the cost is not paid. It surfaces only on cache misses โ€” CI cold + runs, build-script edits, and `build-logic` changes. +2. The measured numbers here are not consistent with a 12-second configuration phase. ADR 0011 + records a 4s warm clean build, and `make build-budget` on 2026-08-09 measured a 3.3s warm clean + build and 1.6s incremental. Whatever configuration costs in this project, it is a small fraction + of that. + +Against the migration: the precompiled form is what the team chose for readability and maintenance, +and one commonly-cited disadvantage of it turns out to be false. Precompiled script plugins **can** +share helper functions โ€” a top-level declaration in a plain `.kt` file in the same source set is +visible to every script, which is how `Versions.kt` and `AndroidCommon.kt` already work. So the +"you must go binary to factor out duplication" argument does not hold, and the two largest reuse +cleanups (a shared `configureBillionBeersAndroid`, and a `billionbeers.android.testing` plugin) +were both delivered in the precompiled form with no migration at all. + +## Decision + +**Conventions stay precompiled script plugins.** No migration, opportunistic or otherwise. + +The two existing binary plugins stay binary โ€” they are task-and-extension implementations rather +than conventions, which is what that form is good at. + +**The comparison is a measurement task, not a judgement call.** Before this is reopened, someone +should measure it on this repo rather than argue from another project's numbers: + +1. Pick one convention โ€” `billionbeers.android.testing` is the smallest and has no accessor use. +2. Convert it to a `Plugin` on a branch, registered via `gradlePlugin { plugins { โ€ฆ } }`. + Consumers do not change; the plugin id is the same. +3. Measure configuration time on a **configuration-cache miss** both ways, since that is the only + state where the difference can appear: + `./gradlew help --no-configuration-cache` and a `build-logic`-touching run, several iterations. +4. Compare against `config/build-time-budget.txt` methodology โ€” medians, not single runs, and on a + machine doing nothing else (ADR 0011). + +## Consequences + +- The `apply(plugin = "billionbeers.โ€ฆ")` workaround stays, and stays commented, until either the + Gradle bug is confirmed fixed or the measurement above justifies conversion. Flipping one back to + a `plugins { }` block and running `./gradlew help` is the cheap way to test the former. +- Build logic remains untested. That is a real cost and the strongest argument on the other side; + it is accepted for now because the conventions are small and heavily commented, and because the + invariants in `:konsist` cover the *outcomes* the conventions produce. +- **Reopen triggers:** a measured configuration-time difference that matters at this repo's scale; + a decision to publish a convention (Gradle's docs recommend converting to binary before + publishing โ€” note the two artifacts currently earmarked for extraction are already binary); or a + Gradle release that makes precompiled scripts materially worse. + +## Related + +- Declarative Gradle (`.gradle.dcl`) is a separate question and further out. It replaces the + *consumer* build files, not the build logic, and requires binary plugins registering software + types underneath โ€” so it sits on top of this decision rather than competing with it. It remains + experimental, with prototype-grade AGP support, and has no expression for several things this + build does (the baseline-profile `finalizeDsl` hook, the Paparazzi source generation, the + auto-discovering `settings.gradle.kts`). Not before a stable release. diff --git a/feature/beerbrowse/detekt-baseline.xml b/feature/beerbrowse/detekt-baseline.xml new file mode 100644 index 00000000..6d56e2b3 --- /dev/null +++ b/feature/beerbrowse/detekt-baseline.xml @@ -0,0 +1,10 @@ + + + + + EmptyFunctionBlock:BrowseBeersViewModelTest.kt$BrowseBeersViewModelTest${} + LongParameterList:BrowseBeersScreen.kt$( title: String, viewState: CommonUiState<PagedListUiModel<Beer>>, onBack: () -> Unit, onBeerClick: (Beer) -> Unit, onScrollToBottom: () -> Unit, onRetryLoadMore: () -> Unit, // Serves both first-page reloads: the full-screen error retry and the pull-to-refresh gesture. onRetryFirstPage: () -> Unit, ) + LongParameterList:BrowseHomeScreen.kt$( styles: CommonUiState<List<BeerStyle>>, breweries: CommonUiState<List<Brewery>>, selectedTab: Int, onTabSelected: (Int) -> Unit, onStyleClick: (BeerStyle) -> Unit, onBreweryClick: (Brewery) -> Unit, onBack: () -> Unit, onRetryStyles: () -> Unit, onRetryBreweries: () -> Unit, ) + MatchingDeclarationName:BeerBrowseScreenImpl.kt$BrowseSelection + + diff --git a/feature/beersearch/detekt-baseline.xml b/feature/beersearch/detekt-baseline.xml new file mode 100644 index 00000000..c7612e7d --- /dev/null +++ b/feature/beersearch/detekt-baseline.xml @@ -0,0 +1,8 @@ + + + + + EmptyFunctionBlock:BeersSearchViewModelTest.kt$BeersSearchViewModelTest${} + LongParameterList:BeersSearchScreen.kt$( viewState: CommonUiState<PagedListUiModel<Beer>>, query: String, onQueryChange: (String) -> Unit, onBeerClick: (Beer) -> Unit, onBack: () -> Unit, onScrollToBottom: () -> Unit, onRetryLoadMore: () -> Unit, onRetrySearch: () -> Unit, autoFocus: Boolean = true, ) + + diff --git a/feature/beerslist/detekt-baseline.xml b/feature/beerslist/detekt-baseline.xml index 6fc2e5b7..ed22813b 100644 --- a/feature/beerslist/detekt-baseline.xml +++ b/feature/beerslist/detekt-baseline.xml @@ -2,6 +2,6 @@ - MagicNumber:BeersListScreen.kt$300 + LongParameterList:BeersListScreen.kt$( viewState: CommonUiState<PagedListUiModel<Beer>>, onBeerClick: (Beer) -> Unit, onSearchClick: () -> Unit, onBrowseClick: () -> Unit, onScrollToBottom: () -> Unit, onRefresh: () -> Unit, onRetry: () -> Unit, onRetryLoadMore: () -> Unit, ) diff --git a/konsist/detekt-baseline.xml b/konsist/detekt-baseline.xml new file mode 100644 index 00000000..f8f43e03 --- /dev/null +++ b/konsist/detekt-baseline.xml @@ -0,0 +1,7 @@ + + + + + ForbiddenComment:TestFixturesPluginBoundaryTest.kt$TestFixturesPluginBoundaryTest$* Backs ADR 0001 (docs/adr/0001-test-fixtures-via-sibling-modules.md): test fixtures live in * sibling `:module:fakes` / `:module:fixtures` modules, never Gradle's `java-test-fixtures` plugin. * That was decided on measured build-time cost, not taste - the plugin adds a variant to every * consumer and the ADR records what that did to build times here. * * The decision was drifting back in on its own: `beerdomain/api/build.gradle.kts` carried a "TODO: * try testFixtures instead" with commented-out `testFixturesImplementation` lines, which is how a * settled ADR quietly becomes a suggestion. Deleting the TODO fixes today; this rule fixes the next * time. * * Matching is on the plugin id and deliberately ignores comment lines - see [uncommentedText]. + + diff --git a/presentation_utils/detekt-baseline.xml b/presentation_utils/detekt-baseline.xml new file mode 100644 index 00000000..864dc8d9 --- /dev/null +++ b/presentation_utils/detekt-baseline.xml @@ -0,0 +1,8 @@ + + + + + LongParameterList:DynamicFeatureInstallerTest.kt$DynamicFeatureInstallerTest$( status: Int, sessionId: Int = SESSION_ID, errorCode: Int = SplitInstallErrorCode.NO_ERROR, downloaded: Long = 0, total: Long = 0, modules: List<String> = listOf(MODULE), ) + MatchingDeclarationName:InfiniteListHandler.kt$ListPosition + + diff --git a/snapshot-processor/detekt-baseline.xml b/snapshot-processor/detekt-baseline.xml new file mode 100644 index 00000000..a5475fe0 --- /dev/null +++ b/snapshot-processor/detekt-baseline.xml @@ -0,0 +1,15 @@ + + + + + ReturnCount:SnapshotProcessor.kt$SnapshotProcessor$private fun isValidPreview(func: KSFunctionDeclaration): Boolean + SpreadOperator:SnapshotProcessor.kt$SnapshotProcessor$(true, *resolver.getAllFiles().toList().toTypedArray()) + SwallowedException:SnapshotProcessor.kt$SnapshotProcessor$e: Exception + SwallowedException:SnapshotProcessor.kt$SnapshotProcessor$e: FileAlreadyExistsException + TooGenericExceptionCaught:SnapshotProcessor.kt$SnapshotProcessor$e: Exception + UnusedPrivateProperty:SnapshotProcessor.kt$SnapshotProcessor$val previewSymbols = resolver.getSymbolsWithAnnotation("androidx.compose.ui.tooling.preview.Preview") + WildcardImport:SnapshotProcessor.kt$import com.google.devtools.ksp.processing.* + WildcardImport:SnapshotProcessor.kt$import com.google.devtools.ksp.symbol.* + WildcardImport:SnapshotProcessor.kt$import com.squareup.kotlinpoet.* + + diff --git a/testing-utils-android/detekt-baseline.xml b/testing-utils-android/detekt-baseline.xml new file mode 100644 index 00000000..65727533 --- /dev/null +++ b/testing-utils-android/detekt-baseline.xml @@ -0,0 +1,7 @@ + + + + + TooManyFunctions:BaseTestRobot.kt$BaseTestRobot + +