Skip to content

Upgrade to Gradle 9.6.1 and Android Gradle plugin 9#66

Open
oschwald wants to merge 15 commits into
mainfrom
greg/upgrade-gradle
Open

Upgrade to Gradle 9.6.1 and Android Gradle plugin 9#66
oschwald wants to merge 15 commits into
mainfrom
greg/upgrade-gradle

Conversation

@oschwald

@oschwald oschwald commented Jul 24, 2026

Copy link
Copy Markdown
Member

Linear: STF-1171

Summary

Gradle 9.6.0 removed several internal Problems API classes, including org.gradle.api.problems.internal.InternalProblems, which AGP 8.x binds to. AGP 8.13.1 therefore fails at plugin application on Gradle 9.6, so the wrapper bump and the AGP 9 upgrade have to land together.

  1. Declare a minCompileSdk of 30 in the AAR metadata.
  2. Upgrade to Gradle 9.6.1 and AGP 9.3.1 — AGP 9 ships built-in Kotlin support and rejects org.jetbrains.kotlin.android, so that plugin is removed from both modules, the root build file, and the version catalog. The three buildfeatures properties AGP 9 removed (buildconfig, aidl, renderscript) are dropped; both modules already enable buildConfig in their own buildFeatures block.
  3. Register the japicmp pipeline tasks without the deprecated delegate — Gradle 9.6 deprecated val name by tasks.registering { } for removal in Gradle 10, and all three uses were in the japicmp pipeline.
  4. Retry the distribution download twiceretries=0 is the upstream 9.6.1 default rather than an accidental disabling; this opts in deliberately.
  5. Run R8 and compile instrumented tests in CI, and assert the AAR's consumer metadata in the api-compat workflow.
  6. Fix stale dokka and JaCoCo references in the docs (pre-existing).

Supersedes #59, which bumps AGP to 9.3.1 without the Gradle wrapper bump it needs.

Why minCompileSdk 30

AGP 9 defaults the AAR's minCompileSdk to the library's own compileSdk. Left alone it would publish 36 and re-introduce exactly the consumer floor that 0.3.1 removed for #63.

The floor was measured by building a consumer app against every platform from 27 to 36. The outcome depends on the consumer's configuration, so the full matrix:

Consumer compileSdk Java 9+ sourceCompatibility Java 8 + minify Java 8, no minify
30 and above pass pass pass
27, 28, 29 fail (AGP Java 9 rule) fail (R8 missing class) pass

Two independent mechanisms already required 30 before this change:

  • InstallationInfoHelper references android.content.pm.InstallSourceInfo, added in API 30. The call is SDK_INT guarded, which keeps it safe at runtime but does not help R8, which reports Missing class android.content.pm.InstallSourceInfo.
  • AGP refuses to create the JavaCompile task at all when sourceCompatibility > 8 and compileSdk < 30 — a configuration-time error independent of which APIs the source touches.

For a consumer that neither minifies nor sets Java 9+ source compatibility, the floor is a new requirement, as the last column shows. That case is the reason the changelog says so plainly rather than claiming the floor was purely pre-existing. The tradeoff is deliberate: it converts an obscure R8 failure into a clear Gradle error for the mainstream configurations.

The harness was validated with a negative control — a consumer referencing an API-35-only constant passes at 35/36 and fails with Unresolved reference at 34 and below — so these are real compilations against each platform's android.jar rather than false greens.

The published ABI is unchanged

AGP 8 derived the Kotlin module name per variant (device-sdk_release); AGP 9's built-in Kotlin uses the project name (device-sdk). That name is baked into kotlin.Metadata and into the mangled JVM names of internal members, so switching it rewrote metadata on 74 classes and renamed symbols such as collectDeviceData$device_sdk_release() — enough to fail japicmp against released 0.3.1 despite no genuine API change. Pinning the module name to the AGP 8 value keeps the artifact binary compatible and the api-compat workflow green with no baseline reset.

Verified on the published artifact: javap across all 112 classes is identical to an AGP 8.13.1 build, the POM is byte-identical, minSdk is still 27, and minAndroidGradlePluginVersion stays 1.0.0, so consumers are not forced onto AGP 9.

New CI guards

  • R8 ran in no job. ci.yml built only :sample:assembleDebug, and both debug variants set isMinifyEnabled = false, as does the SDK's own release variant — so the one code path that justifies minCompileSdk had no coverage. :sample:assembleRelease closes that.
  • japicmp cannot see the AAR metadata, since it compares classes.jar only. The api-compat workflow now asserts minCompileSdk, minAndroidGradlePluginVersion and the manifest's minSdkVersion, which is precisely the regression class this PR exists to prevent. Verified the guard fires: building with minCompileSdk 29 fails the assertion, restoring 30 passes it.
  • androidTest was never compiled by CI, and a toolchain upgrade is the likeliest thing to break it.

One deprecation remains that we cannot fix from here: detekt 1.23.8 calls the deprecated ReportingExtension.file(String) from DetektPlugin.apply, so the build still prints Gradle's "incompatible with Gradle 10" summary line. Ours are all gone.

Behavior change worth noting

AGP 9 no longer registers testReleaseUnitTest, so ./gradlew :device-sdk:test now runs the 164 unit tests once instead of twice. Test executions halved; behavioral coverage did not. device-sdk has no src/release, sets isMinifyEnabled = false, declares both BuildConfig fields in defaultConfig, and has no variant-specific dependencies, so testReleaseUnitTest was a duplicate run with essentially no incremental signal — and AGP unit tests never see R8 output regardless. Keeping AGP 9's default is correct here; there is no need to re-add an androidComponents opt-in.

Verification

From-scratch run with the build cache disabled, 317 tasks executed:

  • ./gradlew :device-sdk:build — 164 tests, 0 failures
  • ./gradlew :device-sdk:japicmp — passes against the released 0.3.1 AAR
  • ./gradlew detekt ktlintCheck — pass
  • ./gradlew :sample:assembleDebug and :sample:assembleRelease — pass, including R8 minification
  • ./gradlew :device-sdk:assembleDebugAndroidTest — passes
  • ./gradlew :device-sdk:dokkaGenerate — passes (dokkaHtml is a disabled V1 task under Dokka 2, which is why the docs were fixed)
  • AAR consumer-metadata assertion — passes, and verified to fail when the floor changes
  • Consumer app built against SDK platforms 27 through 36 across Java 8/9+ and minify on/off, plus a negative control to confirm the harness detects violations

Not verified: publishing end to end. publishToMavenLocal fails in my environment on gpg exit 2 because there is no signing key available; the signing configuration is untouched by this branch.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Compatibility
    • Updated the published AAR consumer metadata to require compileSdk 30+ (effective from version 0.4.0) to prevent build/minification issues; verified public API/ABI compatibility.
  • Build Updates
    • Upgraded to Gradle 9.6.1 and Android Gradle Plugin 9.3.1.
    • Improved Gradle wrapper setup with stronger download/retry behavior.
    • Refined build feature defaults to be explicit per module.
  • CI / Quality
    • Added pull-request validation for AAR consumer metadata and manifest requirements.
    • Expanded CI builds to include instrumented debug tests and a minified sample release.
  • Documentation
    • Updated Gradle/Dokka command guidance and tooling setup/upgrade notes.

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The project upgrades to Gradle 9.6.1 and AGP 9.3.1, removes the Kotlin Android plugin alias, updates wrapper scripts, declares minCompileSdk = 30 for the AAR, pins the published Kotlin module name, and expands CI validation.

Changes

Build compatibility upgrade

Layer / File(s) Summary
Gradle and AGP toolchain upgrade
gradle/libs.versions.toml, gradle/wrapper/*, gradle.properties, gradlew*, build.gradle.kts, device-sdk/build.gradle.kts
The build toolchain, wrapper launch behavior, AGP feature settings, and API compatibility task registration are updated for AGP 9.3.1 and Gradle 9.6.1.
Kotlin Android plugin alias cleanup
gradle/libs.versions.toml, build.gradle.kts, device-sdk/build.gradle.kts, sample/build.gradle.kts, CLAUDE.md
The Kotlin Android plugin alias is removed from the catalog and build scripts, and the documentation example is updated.
AAR metadata and module-name compatibility
device-sdk/build.gradle.kts, CHANGELOG.md, README.md, .github/workflows/api-compat.yml
The AAR declares minCompileSdk = 30, the Kotlin module name is pinned to device-sdk_release, compatibility requirements are documented, and packaged metadata is validated.
Build validation and developer instructions
.github/workflows/ci.yml, CLAUDE.md, README.md, SETUP.md, README.dev.md
CI validates wrappers and additional build variants, while documentation updates commands, Java setup, testing guidance, and Gradle upgrade instructions.

Estimated code review effort: 3 (Moderate) | ~30 minutes

Possibly related PRs

  • maxmind/device-android#42: Changes fallback logic in the InstallationInfoHelper code referenced by this AAR compatibility update.

Poem

A rabbit hops through Gradle’s den,
AGP grows strong from eight to nine again.
The AAR wears compile-SDK cheer,
Kotlin’s name stays crisp and clear.
CI checks each build just right—
Wrapper scripts take flight.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change set: upgrading to Gradle 9.6.1 and AGP 9.x.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch greg/upgrade-gradle

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@CHANGELOG.md`:
- Line 10: Update the changelog entry to remove the claim that compiling Java 9
or later source requires compileSdk 30, while preserving any accurate compileSdk
guidance that remains.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 5158bfa1-ca35-4964-9530-2d67e4f03a6d

📥 Commits

Reviewing files that changed from the base of the PR and between ffe781c and b207d96.

⛔ Files ignored due to path filters (1)
  • gradle/wrapper/gradle-wrapper.jar is excluded by !**/*.jar
📒 Files selected for processing (10)
  • CHANGELOG.md
  • CLAUDE.md
  • build.gradle.kts
  • device-sdk/build.gradle.kts
  • gradle.properties
  • gradle/libs.versions.toml
  • gradle/wrapper/gradle-wrapper.properties
  • gradlew
  • gradlew.bat
  • sample/build.gradle.kts
💤 Files with no reviewable changes (2)
  • sample/build.gradle.kts
  • build.gradle.kts

Comment thread CHANGELOG.md Outdated
@oschwald
oschwald force-pushed the greg/upgrade-gradle branch from 6ece20c to d8345c0 Compare July 24, 2026 21:31
oschwald and others added 4 commits July 24, 2026 22:18
InstallationInfoHelper references android.content.pm.InstallSourceInfo,
which was added in API 30. A runtime SDK_INT guard keeps that safe at
execution time, but it does not help R8: a consumer minifying against
compileSdk 29 or lower fails with "Missing class
android.content.pm.InstallSourceInfo". Declaring the floor turns that
confusing R8 failure into a clear Gradle error.

Verified by building a consumer app against every platform from 27 to 36:
compileSdk 30 and above succeed for both debug and minified release
builds, while 29 and below fail. Consumers compiling Java 9+ source
already needed compileSdk 30, so this only makes an existing requirement
explicit rather than adding one.

Declaring the value explicitly also keeps AGP from defaulting it to our
own compileSdk, which would otherwise force consumers up to 36.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Gradle 9.6.0 removed several internal Problems API classes, including
org.gradle.api.problems.internal.InternalProblems, which AGP 8.x binds
to. AGP 8.13.1 therefore fails at plugin application on Gradle 9.6, so
the two upgrades have to happen together.

AGP 9 ships built-in Kotlin support and rejects the separate
org.jetbrains.kotlin.android plugin, so that plugin is removed from both
modules, from the root build file, and from the version catalog. The
kotlin version reference is still used by kotlin-stdlib and the
serialization plugin.

The buildconfig, aidl, and renderscript buildFeatures properties were
removed in AGP 9 and now warn that they have no effect. Both modules
already enable buildConfig explicitly in their own buildFeatures blocks.

The Kotlin module name is pinned to its AGP 8 value so the published ABI
does not change. See the comment in device-sdk/build.gradle.kts.

One behavior change to note: AGP 9 no longer registers
testReleaseUnitTest, so ./gradlew :device-sdk:test now runs the 164 unit
tests once against the debug variant instead of twice. All of them pass,
as do detekt, ktlintCheck, japicmp, dokkaGenerate, and both sample app
variants.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Gradle 9.6 deprecated the `val name by tasks.registering { }` property
delegate syntax, scheduled for removal in Gradle 10. All three uses were
in the japicmp pipeline, which is the only automated guard on the Kotlin
module name pin, so it is the last thing that should break on a future
Gradle upgrade.

Switched to the `tasks.register(...)` form already used by the japicmp
task itself just below. TaskProvider semantics are unchanged, so the
existing dependsOn references still work.

This does not silence the build's Gradle 10 warning entirely: detekt
1.23.8 also calls the deprecated ReportingExtension.file(String) from
DetektPlugin.apply, which we cannot fix from here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Gradle 9.6.1's wrapper task writes retries and retryBackOffMs into
gradle-wrapper.properties, and its default is retries=0. Gradle 9.3.1's
wrapper had no retry support at all, so the upgrade did not change
behavior, it only made the setting visible.

Set retries=2 so a transient failure fetching the distribution retries
instead of failing the job outright. The 500ms backoff default is kept.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@oschwald
oschwald force-pushed the greg/upgrade-gradle branch from d8345c0 to 54b499e Compare July 24, 2026 22:18

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@device-sdk/build.gradle.kts`:
- Around line 37-39: Update the aarMetadata configuration to use AGP 9.3’s
non-deprecated minCompileSdk DSL, replacing the direct assignment with the
nested version configuration and release(30) value while preserving the minimum
compile SDK requirement.

In `@gradle/wrapper/gradle-wrapper.properties`:
- Line 3: Update the Gradle wrapper configuration alongside distributionUrl by
adding the official distributionSha256Sum for the Gradle 9.6.1 binary ZIP, using
the checksum published by Gradle so downloaded distributions are validated.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b2974025-c73e-49e6-b30a-26b325a5892c

📥 Commits

Reviewing files that changed from the base of the PR and between b207d96 and 54b499e.

⛔ Files ignored due to path filters (1)
  • gradle/wrapper/gradle-wrapper.jar is excluded by !**/*.jar
📒 Files selected for processing (14)
  • .github/workflows/api-compat.yml
  • .github/workflows/ci.yml
  • CHANGELOG.md
  • CLAUDE.md
  • README.md
  • SETUP.md
  • build.gradle.kts
  • device-sdk/build.gradle.kts
  • gradle.properties
  • gradle/libs.versions.toml
  • gradle/wrapper/gradle-wrapper.properties
  • gradlew
  • gradlew.bat
  • sample/build.gradle.kts
💤 Files with no reviewable changes (2)
  • sample/build.gradle.kts
  • build.gradle.kts

Comment on lines +37 to +39
aarMetadata {
minCompileSdk = 30
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

rg -n -A5 -B2 'aarMetadata\s*\{|minCompileSdk' device-sdk/build.gradle.kts

Repository: maxmind/device-android

Length of output: 590


🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

printf 'Relevant gradle and AGP declarations:\n'
rg -n 'android\s*\{|agp|android-gradle-plugin|google' -S build.gradle.kts settings.gradle.kts gradle/libs.versions.toml device-sdk/build.gradle.kts || true

printf '\nVersion catalog entries:\n'
if [ -f gradle/libs.versions.toml ]; then
  sed -n '1,220p' gradle/libs.versions.toml
fi

Repository: maxmind/device-android

Length of output: 4739


🌐 Web query:

AGP 9.3.1 AarMetadata minCompileSdk property deprecation minCompileSdk DSL version release(30)

💡 Result:

In Android Gradle Plugin (AGP) 9.3.0 and later, the AarMetadata.minCompileSdk property has been deprecated in favor of a new DSL-based approach using minCompileSdk() [1][2]. As of AGP 9.3.0, you should no longer set the minCompileSdk property directly [1]. Instead, you should use the new minCompileSdk block, which accepts an action to configure the value using a MinCompileSdkSpec [1][2]. This change is part of a broader shift in AGP to standardize configuration patterns across the DSL [3]. To update your configuration, you should transition from the old property assignment to the new method syntax [1]: // Deprecated in AGP 9.3+ aarMetadata { minCompileSdk = 30 } // Recommended approach in AGP 9.3+ aarMetadata { minCompileSdk { version = 30 } } The minCompileSdk setting continues to serve the same purpose: specifying the minimum compileSdkVersion required by any module consuming your library [2][4]. Setting this value ensures that consuming projects are alerted if they attempt to use your library with an incompatible (lower) compile SDK version, preventing potential runtime errors such as 'resource not found' [2][4]. For more details on your specific implementation, refer to the official Android Gradle Plugin API reference for AarMetadata version 9.3 [1][2].

Citations:


🌐 Web query:

Android Gradle Plugin 9.3 AarMetadata.minCompileSdk deprecated minCompileSdk DSL release(30)

💡 Result:

In Android Gradle Plugin (AGP) 9.3, the previous property-based DSL for setting minCompileSdk within AarMetadata has been deprecated [1][2]. You should now use the minCompileSdk function, which takes a configuration block (using MinCompileSdkSpec) to specify the compile SDK requirements [1][2][3]. ### Migration Instead of directly setting the property (e.g., minCompileSdk = 30), you should use the new function-based API [1][2]. According to the AGP 9.3 API reference, this structure allows for more granular configuration, such as setting the version using a release API level [1][3]. Example usage (Kotlin DSL): android { defaultConfig { aarMetadata { minCompileSdk { version = release(30) } } } } The MinCompileSdkSpec allows you to compute the CompileSdkVersion using helpers like release(int) [3]. This change is part of an ongoing effort in AGP to standardize the DSL interfaces by moving away from legacy property assignments toward functional configuration blocks [4]. You should avoid setting the deprecated minCompileSdk property directly, as it is marked for removal in future versions [1][4].

Citations:


Use AGP 9.3’s non-deprecated AAR metadata DSL.

AGP 9.3.1 deprecates minCompileSdk = 30; migrate to minCompileSdk { version = release(30) } to avoid carrying a deprecated API through the AGP upgrade.

Proposed fix
         aarMetadata {
-            minCompileSdk = 30
+            minCompileSdk {
+                version = release(30)
+            }
         }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
aarMetadata {
minCompileSdk = 30
}
aarMetadata {
minCompileSdk {
version = release(30)
}
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@device-sdk/build.gradle.kts` around lines 37 - 39, Update the aarMetadata
configuration to use AGP 9.3’s non-deprecated minCompileSdk DSL, replacing the
direct assignment with the nested version configuration and release(30) value
while preserving the minimum compile SDK requirement.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Claude here, replying on behalf of @oschwald.

Not changing this: AarMetadata.minCompileSdk is not deprecated in AGP 9.3.1.

The block form does exist — com.android.build.api.dsl.AarMetadata declares minCompileSdk(Function1<MinCompileSdkSpec, Unit>) alongside the property — but its existence is not a deprecation of the property. Checked the shipped interface in gradle-api-9.3.1.jar:

public interface com.android.build.api.dsl.AarMetadata {
  public abstract java.lang.Integer getMinCompileSdk();
  public abstract void setMinCompileSdk(java.lang.Integer);
  public abstract java.lang.Integer getMinCompileSdkExtension();
  public abstract void setMinCompileSdkExtension(java.lang.Integer);
  public abstract void minCompileSdk(kotlin.jvm.functions.Function1<? super com.android.build.api.dsl.MinCompileSdkSpec, kotlin.Unit>);
  public abstract java.lang.String getMinAgpVersion();
  public abstract void setMinAgpVersion(java.lang.String);
}

javap -v over that class yields 0 occurrences of Deprecated, and the raw class file contains no deprecat string. As a control that the check is meaningful, com.android.build.api.dsl.CommonExtension from the same jar yields 15. The build also compiles device-sdk/build.gradle.kts with --warning-mode all and reports no deprecation for this usage — the only remaining deprecation in the build comes from detekt's ReportingExtension.file(String).

The linked reference page describes the newer spec-based form, which is real; the "deprecated in AGP 9.3+" framing does not match the shipped bytecode.

Comment thread gradle/wrapper/gradle-wrapper.properties Outdated
oschwald and others added 4 commits July 24, 2026 22:29
Two verification gaps that the minCompileSdk work made pointed.

R8 ran in no CI job. ci.yml built only :sample:assembleDebug, and both
debug variants set isMinifyEnabled = false, as does the SDK's own release
variant. The AAR's minCompileSdk floor is justified entirely by an R8
missing-class failure, so the one code path that justifies it had no
coverage. :sample:assembleRelease closes that.

The androidTest sources were never even compiled by CI, and a toolchain
upgrade is the likeliest thing to break them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
japicmp compares classes.jar only, so the parts of the AAR that decide
who can consume the library had no automated guard: minCompileSdk,
minAndroidGradlePluginVersion, and the manifest's minSdkVersion.

That is precisely the regression class worth guarding. AGP 9 defaults
minCompileSdk to the library's own compileSdk, so a future compileSdk
bump would silently raise the consumer floor to match, which is the
breakage 0.3.1 removed for GitHub #63. Pinning the expected values in CI
means changing them takes a deliberate edit here as well.

Verified the check fails as intended: building with minCompileSdk 29
fails the assertion, and restoring 30 passes it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Under Dokka 2 the V1 tasks are disabled: `:device-sdk:tasks --all` lists
dokkaHtml as "[V1 tasks disabled]", so every documented invocation of it
fails. The working task is dokkaGenerate. Fixed in CLAUDE.md, README.md
and SETUP.md, and the SETUP.md one is now module-qualified like the
others.

JaCoCo appears in no build file: there is no plugin, no jacocoTestReport
task, and no coverage measurement of any kind. Removed the command and
the "Test coverage with JaCoCo" claim from CLAUDE.md rather than leaving
instructions that cannot work.

Both are pre-existing rather than introduced by the toolchain upgrade.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
validateDistributionUrl only checks that the distribution URL looks like
an official Gradle host; it does not verify the bytes that come back.
distributionSha256Sum makes the wrapper reject a corrupted or tampered
distribution before executing it.

The value is the published checksum from
https://services.gradle.org/distributions/gradle-9.6.1-bin.zip.sha256.

Verified both directions: a fresh download into an empty
GRADLE_USER_HOME validates and runs, and substituting a wrong hash fails
with "Verification of Gradle distribution failed!", so the check is
active rather than silently ignored.

Note for future wrapper upgrades: this value has to be updated alongside
distributionUrl or the build will refuse to start.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Upgrades the project’s build toolchain to Gradle 9.6.1 + Android Gradle Plugin (AGP) 9.3.1 while keeping the published SDK artifact ABI stable and adding CI guards to prevent accidental consumer-facing metadata regressions.

Changes:

  • Upgraded Gradle wrapper to 9.6.1 and AGP to 9.3.1; removed the now-rejected org.jetbrains.kotlin.android plugin alias/usages.
  • Pinned published AAR consumer constraints by explicitly setting aarMetadata.minCompileSdk = 30 and documenting the new compileSdk requirement.
  • Strengthened CI by running an R8-minified sample build, compiling androidTest sources, and asserting AAR consumer metadata in the API compatibility workflow.

Reviewed changes

Copilot reviewed 13 out of 15 changed files in this pull request and generated no comments.

Show a summary per file
File Description
SETUP.md Updates documentation generation command to :device-sdk:dokkaGenerate.
sample/build.gradle.kts Removes Kotlin Android plugin application to align with AGP 9 behavior.
README.md Documents compileSdk 30+ requirement and updates Dokka command.
gradlew.bat Updates Windows Gradle wrapper script as generated for Gradle 9.6.1 (incl. improved error handling).
gradlew Updates POSIX Gradle wrapper script for Gradle 9.6.1 (incl. -jar invocation).
gradle/wrapper/gradle-wrapper.properties Bumps wrapper distribution to 9.6.1, adds SHA-256, and enables download retries.
gradle/libs.versions.toml Upgrades AGP version and removes Kotlin Android plugin alias from the catalog.
gradle.properties Removes AGP-9-removed buildFeatures defaults and documents remaining supported defaults.
device-sdk/build.gradle.kts Sets aarMetadata.minCompileSdk = 30, pins Kotlin module name to preserve ABI, and updates task registration style.
CLAUDE.md Removes stale JaCoCo reference, updates Dokka command, and refreshes guidance around release/R8.
CHANGELOG.md Adds 0.4.0 (TBD) notes covering minCompileSdk=30 and the toolchain upgrade.
build.gradle.kts Removes Kotlin Android plugin from root plugin declarations.
.github/workflows/ci.yml Adds :sample:assembleRelease (R8) and :device-sdk:assembleDebugAndroidTest compilation to CI.
.github/workflows/api-compat.yml Adds assertions for AAR consumer metadata (minCompileSdk/AGP floor/manifest minSdk).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@horgh horgh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Per-commit review

Reviewed all 8 commits independently, plus three whole-branch passes (comment accuracy, test-coverage gaps, security). 46 findings: 3 major, 24 minor, 19 nits. No blockers.

Bottom line

Mergeable, but three things deserve a decision first. Nothing here breaks the build or the published artifact today — the upgrade itself checks out. What the review found is that several of the new guards are weaker than their commit messages claim, and that the wrapper hardening is one ./gradlew wrapper away from silently vanishing.

eb8a550 came back clean — verified byte-for-byte faithful (whitespace-stripped diff of the changed region is identical apart from the three declaration forms), task names / Copy type / statement order / TaskProvider wiring all preserved, deprecation premise confirmed, and no delegate syntax remains anywhere in the repo.

The three majors

  1. The AAR metadata check has a silent false-pass holeapi-compat.yml:41-50. It asserts four keys are present and never constrains the rest of the file. AGP 9.3.1 emits up to nine keys; the published 0.3.1 AAR carries six. minCompileSdkExtension, minCompileMinorSdk and coreLibraryDesugaringEnabled gate consumers exactly like minCompileSdk does and are all unasserted. Inline comment has the detail.
  2. Nothing verifies that 30 is still the correct floordevice-sdk/build.gradle.kts:37-39. The assertion pins the literal; the R8 step builds at compileSdk 36 and can't reproduce the failure. NewApi lint can't help, because a guarded call is precisely the case that still puts the type reference in bytecode.
  3. The floor may be broader than the problem requires-dontwarn android.content.pm.InstallSourceInfo in consumer-rules.pro fixes the R8 error for minifying consumers without touching the AAR's consumer contract at all. Product call, not a bug.

Verified against upstream (all good)

Reviewers disassembled the checked-in wrapper jar, downloaded AGP 9.3.1, and pulled the published 0.3.1 AAR rather than reasoning from memory:

  • Committed gradle-wrapper.jar sha256 497c8c2a… matches Gradle's published gradle-9.6.1-wrapper.jar.sha256. gradlew / gradlew.bat are byte-identical to upstream v9.6.1 except the stock DEFAULT_JVM_OPTS line.
  • distributionSha256Sum 9c0f7fae… matches the published gradle-9.6.1-bin.zip.sha256, and differs from the -all.zip and wrapper-jar sums — right artifact pinned. Property name and semantics confirmed against WrapperExecutor.
  • retries=2 really is "retry twice": WrapperConfiguration defaults are retries=0 / retryBackOffMs=500, forceFetch does attempts = retries + 1, and the 9.3.1 jar on main contains neither string — so the upgrade genuinely didn't change behaviour, it only made the setting visible.
  • The four pinned AAR metadata values are right: AAR_FORMAT_VERSION="1.0", AAR_METADATA_VERSION="1.0", DEFAULT_MIN_AGP_VERSION="1.0.0", writer emits bare key=value with a hard-coded \n. AAR manifest is plain text with <uses-sdk android:minSdkVersion="27" />, and ProcessLibraryManifest still injects minSdk under AGP 9. The shell is sound too — set -e vs if !, line continuations, here-strings and unzip exit codes all behave.
  • 30 is the right floor today: every android.* type reachable from the shipped sources is API ≤29 except InstallSourceInfo in InstallationInfoHelper.kt:35.
  • defaultConfig.aarMetadata is valid on both AGP 8.13.1 and 9.x, so 5d7cfba is not a bisect hazard.
  • CI security posture is clean: all actions SHA-pinned, least-privilege permissions ({} / contents: read), persist-credentials: false, no ${{ }} interpolation in the new run: block, :sample:assembleRelease has no signingConfig (unsigned APK, no keystore in CI) and no mapping file or APK is uploaded.

Cross-cutting themes

The wrapper hardening is regeneration-fragile. retries=2 and distributionSha256Sum are hand edits to a machine-generated file, with no Wrapper task configuration in the build scripts and no upgrade runbook in README.dev.md. 4461a1b demonstrates the hazard directly — that regeneration is what emitted retries=0. Gradle hard-fails a version bump when distributionSha256Sum is present without --gradle-distribution-sha256-sum, so the checksum can't be lost silently, but retries two lines below can be.

Every new guard runs only on pull_request — never on the bytes that ship. dev-bin/release.sh rewrites version and baselineVersion, runs only :device-sdk:test, then publishes; per README.dev.md step 12 the release PR is opened after publishing. So japicmp and the new metadata assertion never run against the released commit. Making the metadata check a Gradle task wired into check would fix this and make it runnable locally.

Findings that couldn't be anchored inline (outside the diff)

  • ci.yml:24 (security, nit) — this branch replaces gradle-wrapper.jar, but the only integrity check is gradle/actions/setup-gradle's validate-wrappers defaulting to true — invisible in the workflow files, and it would disappear silently on a major bump or an action swap. Worth setting explicitly on all three setup-gradle steps. (The committed jar itself is authentic — verified above.)
  • ci.yml:33-34 (test gaps, minor):device-sdk:test is a lifecycle task that succeeds when it runs zero tests, and nothing asserts a count. This branch is itself the proof: AGP 9 silently stopped registering testReleaseUnitTest, the suite went from running twice to once, and CI stayed green — a human noticed only by reading the task list.
  • CLAUDE.md:237,254 (docs, minor) — says Java 21 is configured via org.gradle.java.home in gradle.properties. That key isn't there and never has been (git log -S finds nothing); the JDK is pinned by mise.toml (temurin-21). The surrounding /home/greg/... paths are another developer's machine. This is the file agents read first.
  • CLAUDE.md:36 (docs, nit)./gradlew assembleDebug is still annotated "skips minification issues", which contradicts the rewritten Release Build MinifyEnabled section 240 lines below.
  • Commit orderingf8c68d4's comment references the metadata assertion that e8acd1e adds next. Harmless once merged, but the two are ordered backwards relative to the reference.
  • device-sdk/proguard-rules.pro (pre-existing, nit) — wired via proguardFiles on a release variant with isMinifyEnabled = false, so it is applied to nothing, ever. Its Ktor keeps and -dontwarn rules never reach consumers, who only get consumer-rules.pro.

Needs a human decision

  1. Hard minCompileSdk floor vs. -dontwarn (major 3) — who are you willing to break?
  2. Which is true, the commit message or the CHANGELOG? 5d7cfba's message says it "only makes an existing requirement explicit"; the CHANGELOG says it "is a new requirement rather than a pre-existing one". The reviewer judged the CHANGELOG correct.
  3. Should the 0.4.0 entry be marked **Breaking:**? The 0.2.0 entries use that convention.

Comment on lines +31 to +39
// The lowest compileSdk consumers can build against.
// InstallationInfoHelper references android.content.pm.InstallSourceInfo
// (API 30), so R8 reports it as a missing class for consumers minifying
// below that, and declaring the floor turns that into a clear Gradle
// error. Setting it explicitly also pins the value rather than
// inheriting whatever the Android Gradle plugin defaults to.
aarMetadata {
minCompileSdk = 30
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

major — the floor is a broader fix than the problem requires.

The R8 failure only affects consumers that minify, but this hard-fails the build of every consumer below compileSdk 30, including those that don't minify and never had a problem.

The narrower fix — -dontwarn android.content.pm.InstallSourceInfo in device-sdk/consumer-rules.pro, which is already wired up via consumerProguardFiles and is the exact remedy AGP's own missing_rules.txt suggests — resolves the R8 error for minifying consumers without changing the AAR's consumer contract at all. The CHANGELOG concedes as much: for consumers that neither minify nor use Java 9+ source compatibility, this is "a new requirement rather than a pre-existing one".

This is precisely the regression class 0.3.1 just removed for #63 (something silently raising the consumer compileSdk/AGP floor). Practical blast radius in 2026 is small — Play's targetSdk policy already pushes apps well past compileSdk 30 — but the SDK is choosing to break a group of consumers' builds to fix a problem those consumers never had, when a strictly narrower fix exists.

Either ship the -dontwarn and drop the floor, or record why the hard floor was preferred (e.g. "a clear Gradle error beats a silent unresolved reference") so the trade-off is a decision rather than an oversight.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Claude here, replying on behalf of @oschwald.

Keeping the hard floor, and recording the rationale in f6457e2 as you asked. The reasoning, so it is a decision rather than an oversight:

compileSdk is the integrating app's build-time SDK, not the device API level. To be affected by the floor a consumer has to build against API <= 29 and pin Java 8 source compatibility and minify. Anything with Java 9+ sourceCompatibility already cannot use compileSdk < 30 — AGP refuses to configure Java compilation there, as this branch's earlier thread established — and Play's targetSdk policy (35+ for updates, 36 from this August) means an app on compileSdk 29 cannot ship an update at all, since targetSdk cannot exceed compileSdk. That intersection is empty in practice, and minSdk is unchanged, so support for API 27–29 devices is untouched.

On the #63 comparison specifically: I don't think the classes match. #63 forced consumers up to compileSdk 36/37 — the bleeding edge, immediate work for everyone on the current release. A floor of 30 sits five levels below where everyone shipping already is. Same mechanism, opposite blast radius.

Given that, -dontwarn android.content.pm.InstallSourceInfo would add a permanent suppression line to every consumer's R8 configuration to serve a population that cannot ship. For the consumers who do hit R8 today, a clear CheckAarMetadata error beats a suppressed warning and a silently unresolved type.

The CHANGELOG now carries the remedy for anyone who does hit it (ed1ab49), naming the consumer's own compileSdk explicitly so it is not read as dropping device support. Not marked **Breaking:**, per the separate thread.

// error. Setting it explicitly also pins the value rather than
// inheriting whatever the Android Gradle plugin defaults to.
aarMetadata {
minCompileSdk = 30

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

major — nothing verifies that 30 remains the correct floor.

The api-compat assertion added in e8acd1e pins only the declared literal, and the new :sample:assembleRelease step builds at compileSdk 36 so it cannot reproduce the missing-class failure (the comment at ci.yml:42-46 says so itself). The evidence for 30 is the one-time manual sweep described in the commit message — "building a consumer app against every platform from 27 to 36" — which nothing repeats.

NewApi lint can't backstop this: it only flags unguarded calls, and a guarded call (exactly like getInstallSourceInfo behind its SDK_INT >= R check) is precisely the case that still puts an API-31+ type reference into the bytecode and makes R8 emit Missing class … for a consumer compiling below that level.

So the moment a new collector touches e.g. android.credentials.* (API 34), the published AAR keeps declaring 30, CI stays green, and minifying consumers at compileSdk 30–33 hit the exact cryptic R8 failure this branch exists to fix. The inverse — the floor becoming needlessly high after the API-30 call is removed, locking out consumers for no reason — is equally undetected.

Options: a CI job that reproduces the documented verification (make the sample's compileSdk overridable, -PconsumerCompileSdk=30, and run :sample:assembleRelease with it — needs platforms;android-30 added to mise run setup, which installs only android-36 today). Cheaper: scan the release classes.jar for referenced android.* types and fail if any is absent from the API-30 android.jar.

Smaller related point: the pinned values are now duplicated across four places — here, libs.versions.toml (minSdk), README.md:7-8, and api-compat.yml — but only the workflow knows about the coupling. The moduleName comment 40 lines below establishes the opposite convention ("japicmp enforces this, so deleting the line fails api-compat"); worth a reciprocal one-liner here, and naming the file to edit in the ::error:: message.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Claude here, replying on behalf of @oschwald.

Agreed on the gap, and on why NewApi lint cannot backstop it — a guarded call is exactly the case that still leaves the type reference in bytecode.

Not building the CI machinery in this PR. Both remedies need real infrastructure: the consumer-per-platform approach needs platforms;android-30 added to mise run setup (which installs only android-36 today) plus a -PconsumerCompileSdk override on the sample, and the classes.jar scan needs the API-30 android.jar present in CI plus a bytecode scanner. That is a disproportionate amount of new surface to add to a toolchain-upgrade PR.

Tracked as item 1 of STF-1190, with both options recorded.

What did land, in f6457e2, is the cheap half — the declaration now says out loud that raising the floor is manual:

// If a collector ever references an android.* type newer than API 30,
// this value has to be raised to match; nothing verifies that for us.

On the duplication point: after d260d8e the workflow no longer just pins the same literal in a second place — it compares the entire aar-metadata.properties file, so it now also catches the inverse case you describe, where the floor becomes needlessly high (or any other key changes) without anyone noticing.

Comment thread device-sdk/build.gradle.kts Outdated
Comment on lines +76 to +87
// Pins the published ABI -- do not remove. The Kotlin module name is
// baked into kotlin.Metadata and into the mangled JVM names of internal
// members, so changing it rewrites the ABI of every class without any
// source change. "device-sdk_release" is the name 0.3.1 and earlier
// published; AGP 9's built-in Kotlin would otherwise use the project
// name. japicmp enforces this, so deleting the line fails api-compat.
//
// Setting it in the shared compilerOptions block gives every compilation
// the same name instead of AGP 8's per-variant names. That is harmless
// (.kotlin_module files are stripped from APKs) and means the unit tests
// exercise the same mangled names that ship in the release AAR.
moduleName.set("device-sdk_release")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The pin is worth keeping, but two claims here are overstated, and the guard is more fragile than the comment implies.

Accuracy: "baked into kotlin.Metadata" is only indirectly true — it appears via the mangled JVM signatures in the metadata's d2 array and as the META-INF/<name>.kotlin_module file name, not as a metadata field. And "changing it rewrites the ABI of every class" is wrong: only the handful of classes declaring internal members change (DeviceTracker, DeviceApiClient, and the companions with internal members). The consumer-visible public API is byte-for-byte unaffected, since no consumer can call a mangled internal and the compiler reads whatever *.kotlin_module files it finds regardless of their names. A maintainer weighing whether to keep this line will be reasoning from a premise that implies consumer breakage that wouldn't actually occur.

Fragility: "japicmp enforces this" is true today, but only incidentally. It holds solely because public class DeviceTracker still has internal fun collectDeviceData() and internal suspend fun sendDeviceData(), which compile to ACC_PUBLIC mangled names (collectDeviceData$device_sdk_release — verified present in the published 0.3.1 classes.jar). Make those two private, or move them into an internal class — an ordinary refactor a reviewer would wave through — and japicmp no longer sees a single module-name-dependent symbol. The ABI pin silently loses its only guard, with no failing check to announce it.

The AAR already carries a direct witness. One line in the "Check AAR consumer metadata" step, which already unzips the AAR, pins it regardless of what the source looks like:

unzip -p "$aar" classes.jar > /tmp/classes.jar
unzip -l /tmp/classes.jar | grep -q 'META-INF/device-sdk_release.kotlin_module'

Suggested rewording for the comment: the module name determines the JVM name mangling of internal members and the .kotlin_module entry name, so changing it produces a large no-op diff in api-compat; pinning it to the AGP 8 value keeps japicmp meaningful across the toolchain change.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Claude here, replying on behalf of @oschwald.

Both points taken, in f6457e2 and d260d8e.

On accuracy — you're right that "baked into kotlin.Metadata" and "rewrites the ABI of every class" overstate it. The comment now says the name reaches the artifact as the META-INF/<name>.kotlin_module filename and through mangled internal member names (which is how it shows up in d2), and states that the consumer-visible public API is byte-for-byte unaffected. That matters for exactly the reason you give: a maintainer weighing whether to keep the line should not be reasoning from an implied consumer break that would not occur.

On fragility — this was the sharper half. d260d8e adds the direct witness you suggested to the step that already unzips the AAR:

unzip -p "$aar" classes.jar >"$tmp/classes.jar"
entries=$(unzip -l "$tmp/classes.jar")
if ! grep -q 'META-INF/device-sdk_release\.kotlin_module' <<<"$entries"; then

Verified it fires: deleting moduleName.set(...) and rebuilding produces META-INF/device-sdk.kotlin_module, and the step fails and prints the actual entry. So the pin no longer depends on DeviceTracker keeping internal members that japicmp happens to see, and the comment now points at this assertion rather than at japicmp.

Comment on lines +198 to 213
val downloadBaselineAar =
tasks.register("downloadBaselineAar") {
val outputFile = layout.buildDirectory.file("japicmp/baseline.aar")
outputs.file(outputFile)
doLast {
val url = "https://repo1.maven.org/maven2/com/maxmind/device/device-sdk/$baselineVersion/device-sdk-$baselineVersion.aar"
val destFile = outputFile.get().asFile
destFile.parentFile.mkdirs()
URI(url).toURL().openStream().use { input ->
destFile.outputStream().use { output ->
input.copyTo(output)
}
}
logger.lifecycle("Downloaded baseline AAR from $url")
}
logger.lifecycle("Downloaded baseline AAR from $url")
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

security, minor — pre-existing, but after 51f9dbc this is the one remaining unverified network fetch in the build.

This commit only changed by tasks.registeringtasks.register, so it isn't a regression. Flagging it because the branch's stated goal is closing artifact-integrity gaps in the toolchain, and this one stays open: URI(url).toURL().openStream() with no checksum, no signature, no connect/read timeout, and outside Gradle's dependency-resolution machinery — so gradle/verification-metadata.xml can never cover it. The fetched bytes then feed zipTree and japicmp/javassist class parsing on every pull_request run.

If the baseline artifact were ever swapped (Maven Central or MaxMind publishing-account compromise, or an HTTPS_PROXY / internal-mirror redirect on a developer or CI machine), CI would silently consume attacker-controlled archive bytes into zip and bytecode parsers with no integrity signal. The unbounded read with no timeout can also hang the api-compat job until GitHub's 6h limit.

Fix: resolve the baseline through a detached configuration on mavenCentral() so dependency verification covers it, or minimally pin an expected sha256 and set connect/read timeouts on the stream.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Claude here, replying on behalf of @oschwald.

Took the timeouts in 04ebfc3 — 30s connect, 60s read. The hang-until-the-six-hour-limit failure mode was the concrete part of this.

Declining the checksum. The baseline is a different artifact every release, so dev-bin/release.sh would have to rewrite an expected sha256 alongside baselineVersion, which adds a release-time failure mode to protect a CI-side comparison whose bytes arrive over HTTPS from Maven Central. If this is worth hardening further, your first suggestion is the better one: resolve the baseline through a detached configuration on mavenCentral() so gradle/verification-metadata.xml covers it properly, rather than hand-rolling a second integrity mechanism. Recorded that way in STF-1190.

Comment thread .github/workflows/api-compat.yml Outdated
Comment on lines +41 to +50
for expected in \
minCompileSdk=30 \
minAndroidGradlePluginVersion=1.0.0 \
aarFormatVersion=1.0 \
aarMetadataVersion=1.0; do
if ! grep -qxF "$expected" <<<"$metadata"; then
echo "::error::expected $expected in the AAR's aar-metadata.properties"
exit 1
fi
done

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

major — this loop only asserts four lines are present; it never constrains the rest of the file, and the gap is in exactly the regression class the step exists to close.

Verified against the real AGP 9.3.1 jar (AarMetadataTaskKt.writeAarMetadataFile): the writer emits up to nine keys — aarFormatVersion, aarMetadataVersion, minCompileSdk, minCompileMinorSdk, minCompileSdkExtension, minAndroidGradlePluginVersion, forceCompileSdkPreview, coreLibraryDesugaringEnabled, desugarJdkLib. The published 0.3.1 AAR carries six of them. Four of the unasserted ones gate consumers exactly like minCompileSdk does:

  • minCompileMinorSdk and minCompileSdkExtension are defaulted from the library's own compileSdk by the same mechanism the comment above calls out (BooleanOption.DEFAULT_MIN_COMPILE_SDK_IN_AAR_METADATA, default true; AarMetadataUtil.createAndInitialize sets minCompileSdkMinor = compileSdk.minorApiLevel ?: -1 and minCompileSdkExtension = compileSdk.sdkExtension ?: 0, independently of the explicit minCompileSdk = 30).
  • coreLibraryDesugaringEnabled=true would force every consumer to enable core-library desugaring, and can be flipped by a transitive dependency or a future AGP default without anyone editing this repo.

So bumping to compileSdk = "36.1", or an SDK-extension / preview target like android-Baklava, writes minCompileMinorSdk=1 / minCompileSdkExtension=N / forceCompileSdkPreview=Baklava into the AAR and raises the consumer floor — while all four greps still succeed and CI stays green. That ships a #63-style consumer break to Maven Central.

Assert the whole file instead, so any new or changed key has to be acknowledged deliberately — which is what the commit message says it wants:

diff <(sort <<<"$metadata") <(sort .github/expected-aar-metadata.properties)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Claude here, replying on behalf of @oschwald.

Fixed in d260d8e — this was the most valuable finding in the review. The step now compares the whole file rather than asserting four keys are present:

expected=$(printf '%s\n' \
  'aarFormatVersion=1.0' \
  'aarMetadataVersion=1.0' \
  'coreLibraryDesugaringEnabled=false' \
  'minAndroidGradlePluginVersion=1.0.0' \
  'minCompileSdk=30' \
  'minCompileSdkExtension=0')
if ! diff <(sort <<<"$expected") <(sort <<<"$metadata"); then

So any added, removed or changed key has to be acknowledged here, which is what the commit message claimed it wanted and did not deliver.

Verified both failure modes, since catching additions is the whole point and the old version could not:

Scenario Result
baseline pass
minCompileSdk rebuilt as 29 fails, < minCompileSdk=30 / > minCompileSdk=29
file gains an unexpected key fails, diff names the key
moduleName pin removed fails on the new .kotlin_module assertion

Three other things in the same commit, from your other threads on this file: the step now runs :device-sdk:bundleReleaseAar itself rather than depending on japicmp's task graph and runs unless the job is cancelled; it asserts META-INF/device-sdk_release.kotlin_module inside classes.jar as a direct witness for the module-name pin; and it asserts proguard.txt is present and non-empty. It also prints the manifest before failing.

Comment thread CLAUDE.md Outdated
Comment on lines +277 to +281
The sample app has `isMinifyEnabled = true` for release builds, so
`:sample:assembleRelease` is what exercises R8 against the SDK and its consumer
ProGuard rules. CI runs it on every push, so it is expected to pass; if it
starts failing, treat that as a real R8 or keep-rule problem rather than an
expected quirk of the sample.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

minor — "CI runs it on every push" is not accurate, and this is a doc-accuracy commit.

.github/workflows/ci.yml triggers on push: branches: [main] plus pull_request: branches: ['**']. So :sample:assembleRelease runs on pull requests and on pushes to main — a push to a feature branch with no open PR runs this job not at all, and no other workflow builds a minified variant (codeql.yml does assembleDebug).

This section exists to tell a future developer or agent how much R8 coverage they can assume, so overstating it means someone pushes a branch, sees no failure, and believes R8 was exercised against the SDK's consumer rules when nothing ran. Suggested: "CI runs it on every pull request and on pushes to main".

Also worth tempering "is what exercises R8 against the SDK and its consumer ProGuard rules" — see the comment on the ci.yml step: a green assembleRelease proves R8 completed, not that the keep rules preserved what kotlinx.serialization needs.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Claude here, replying on behalf of @oschwald.

Fixed in 40d7e8a — now "on every pull request and on pushes to main". You're right that it matters in a doc-accuracy commit and that the failure mode is specific: someone pushes a feature branch with no PR, sees nothing fail, and concludes R8 was exercised.

Also tempered the R8 claim in the same section: it now says a green run proves R8 completed, not that consumer-rules.pro still keeps what kotlinx.serialization needs, and that such a regression surfaces as a runtime SerializationException in a consumer app rather than as a build failure. Asserting the serializers survive is item 2 of STF-1190.

Comment thread CLAUDE.md Outdated
Comment on lines 266 to 268
**"Resource mipmap/ic_launcher not found"**

- Sample app has no launcher icons (intentional for simplicity)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit — the commit removed the false half of this entry but left the entry itself.

Nothing in the repo references @mipmap/ic_launchersample/src/main/AndroidManifest.xml has no android:icon attribute — so the documented error "Resource mipmap/ic_launcher not found" cannot occur in either variant. A "Common Issue" no build can produce sends future readers hunting for a non-problem, and it's the leftover of exactly the stale claim this commit removed (the icon story was the justification for "may fail on release"). Drop the whole entry.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Claude here, replying on behalf of @oschwald.

Dropped the whole entry in 40d7e8a. Verified your premise first: nothing in the repo references @mipmap/ic_launcher and sample/src/main/AndroidManifest.xml has no android:icon, so the documented error cannot occur in either variant. Leaving half of a stale entry standing was the wrong call in the earlier commit — the icon story was the whole justification for the "may fail on release" claim it removed.

Comment thread CLAUDE.md
Comment on lines 51 to 54

# Run specific test class
./gradlew :device-sdk:test --tests "com.maxmind.device.DeviceTrackerTest"

# Run tests with coverage (JaCoCo)
./gradlew :device-sdk:testDebugUnitTest jacocoTestReport
```

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit — AGP 9 halved what this command runs, and that's recorded only in 4461a1b's commit message.

AGP 9 no longer registers testReleaseUnitTest, so ./gradlew :device-sdk:test and :device-sdk:build now run the suite once against debug instead of twice. SETUP.md presents the same commands unqualified, and dev-bin/release.sh gates the release on ./gradlew :device-sdk:test.

Practical risk is low — the SDK's release variant sets isMinifyEnabled = false and unit tests never see R8 — but the release gate is now weaker than the person running it believes, and there's no in-tree record that release-variant unit tests were dropped deliberately rather than lost by accident. A later reader diffing coverage against 0.3.1 has no way to tell. A one-line note here would do it; alternatively re-enable the release unit-test component explicitly via androidComponents { beforeVariants { … } } if that coverage is wanted.

Separately, CLAUDE.md:36 still annotates ./gradlew assembleDebug with "skips minification issues" — the same "minification is a known quirk, avoid it" framing this branch deliberately replaced 240 lines below. The two now contradict each other.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Claude here, replying on behalf of @oschwald.

Added the note in 40d7e8a, right on the command:

# Run unit tests for SDK. Under AGP 9 this runs the suite once, against the
# debug variant: AGP 9 no longer registers testReleaseUnitTest.

Took the note rather than re-enabling the release unit-test component: with no src/release, isMinifyEnabled = false, both BuildConfig fields in defaultConfig and no variant-specific dependencies, testReleaseUnitTest was a duplicate run, and AGP unit tests never see R8 output regardless. Your underlying point stands though — the release gate is now weaker than the person running it believes, and asserting a non-zero test count is item 6 of STF-1190.

Also fixed CLAUDE.md:36's "skips minification issues" annotation in the same commit.

Comment thread SETUP.md
./gradlew :device-sdk:dokkaGenerate
```

## Publishing to Maven Central

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

minor — same stale-Gradle-task bug class this commit set out to fix, five lines below this one, in a file the commit already had open.

SETUP.md:104 documents ./gradlew :device-sdk:publishReleasePublicationToMavenCentralRepository, and SETUP.md:110-112 documents signing.keyId / signing.password / signing.secretKeyRingFile in local.properties.

Publishing is done by the Vanniktech plugin: dev-bin/release.sh:173 and CLAUDE.md:91 both use :device-sdk:publishAndReleaseToMavenCentral, and device-sdk/build.gradle.kts:188-190 uses signing { useGpgCmd() }, so those keyring properties are never read. The Vanniktech Central Portal repository is also only configured when mavenCentralUsername is non-empty (device-sdk/build.gradle.kts:141-146), so the documented …ToMavenCentralRepository task doesn't exist for a developer following this file — they get a task-not-found error and configure signing properties that have no effect.

Point this section at README.dev.md / publishAndReleaseToMavenCentral, and drop the signing.* keyring block in favour of the gpg-command setup already documented in CLAUDE.md.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Claude here, replying on behalf of @oschwald.

Fixed in 40d7e8a. Confirmed both halves before changing anything: the Vanniktech plugin's task is publishAndReleaseToMavenCentral, and signing { useGpgCmd() } means signing.keyId / secretKeyRingFile are never read — so a developer following that file got a task-not-found error and then configured properties with no effect. It now points at README.dev.md and documents the credentials that are actually used.

While in the file I also swept the same class of staleness in its Troubleshooting section, which told people to point JAVA_HOME at Android Studio's JDK; the version is pinned by mise.toml.

Comment thread README.md Outdated
## Requirements

- Android API 27+ (Android 8.1+)
- `compileSdk` 30+

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit — this bullet silently switches subject from the previous one.

"Android API 27+" is about the end user's device (minSdk); "compileSdk 30+" is about the integrating app's build configuration. Readers routinely conflate the two, and an unqualified compileSdk 30+ directly beneath API 27+ invites the reading that the SDK dropped support for API 27–29 devices, which it did not.

The requirement is also stated unconditionally while the install snippets below still pin 0.3.1, which doesn't declare the floor. Suggested: "Your app must compile against compileSdk 30 or later (the AAR declares minCompileSdk = 30)", noting it applies from 0.4.0.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Claude here, replying on behalf of @oschwald.

Reworded in 40d7e8a:

  • Android API 27+ (Android 8.1+) on the device
  • Your app must compile against compileSdk 30 or later, from 0.4.0 onwards (the AAR declares minCompileSdk = 30)

That covers both halves of your point — the subject switch from device to build configuration is now explicit, and the requirement is scoped to 0.4.0 rather than stated unconditionally above install snippets that still pin 0.3.1.

@horgh

horgh commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

I didn't look at this yet, but Claude had quite a lot to say (above), so I thought I'd just post it, given I don't think I'll get to it this afternoon.

oschwald and others added 7 commits July 24, 2026 23:57
The check asserted that four keys were present and constrained nothing
else, which leaves the hole it was written to close. AGP writes up to
nine keys and derives minCompileSdk, minCompileSdkExtension and
minCompileMinorSdk from the library's own compileSdk, so a compileSdk
bump to something like "36.1" or an SDK-extension target could raise the
consumer floor while all four greps still passed. The whole file is now
compared against an expected set, so any added or changed key has to be
acknowledged here.

Three other gaps in the same step:

- It consumed an AAR it did not build. The artifact existed only because
  :device-sdk:japicmp pulls in bundleReleaseAar through its own task
  graph, and the step was skipped whenever japicmp failed, including for
  unrelated reasons like the baseline download being unavailable. It now
  builds the AAR itself and runs unless the job is cancelled.
- Nothing witnessed the pinned Kotlin module name directly. japicmp only
  catches a change to it while DeviceTracker still has internal members
  whose mangled names reach the ABI; making those private would remove
  the guard with no failing check. The step now asserts
  META-INF/device-sdk_release.kotlin_module inside classes.jar.
- Nothing checked that consumer ProGuard rules ship at all, so
  proguard.txt is asserted present and non-empty.

The manifest branch now prints the manifest before failing, matching the
properties branch.

Verified all four failure modes: a changed value, an unexpected extra
key, a removed moduleName pin (which produces
META-INF/device-sdk.kotlin_module), and the passing baseline.

Reported by horgh.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four inaccuracies, all in comments added or rewritten on this branch.

ci.yml claimed "the debug variants both set isMinifyEnabled = false".
device-sdk has no debug block at all; the only two explicit settings are
device-sdk's release block and sample's debug block. The conclusion was
right but someone grepping for a debug setting would not find one. Also
noted that AGP wires lintVitalRelease into the release assemble, so a
failure there is not necessarily an R8 problem.

The moduleName comment said the name is "baked into kotlin.Metadata" and
that changing it "rewrites the ABI of every class". Both overstate it:
the name reaches the artifact as the .kotlin_module file name and through
mangled internal member names, and the consumer-visible public API is
byte-for-byte unaffected. A maintainer weighing whether to keep the line
would have been reasoning from an implied consumer breakage that does not
occur. It now also points at the .kotlin_module assertion rather than
japicmp, which only guards it incidentally.

gradle.properties said the remaining two properties are "still read by
AGP 9", which is true but flattens them: shaders is stable, resvalues is
deprecated for removal in AGP 10.

The aarMetadata comment now records why the hard floor was chosen over
-dontwarn in consumer-rules.pro, so the trade-off reads as a decision,
and notes that raising the floor is manual if a collector ever
references a newer android.* type.

Reported by horgh.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The entry described the new requirement but not the remedy, and did not
distinguish the consumer's compileSdk from device support, which readers
routinely conflate. Adds both.

Not marked "**Breaking:**" despite the 0.2.0 convention: a consumer has
to build against compileSdk 29 or lower, pin Java 8 source compatibility
and minify to be affected at all, and Play's targetSdk policy means such
an app cannot ship an update regardless. Labelling it would imply action
for the overwhelming majority who need none.

This also corrects 5d7cfba's commit message, which claimed the floor
"only makes an existing requirement explicit rather than adding one" and
cited compileSdk 29 and below failing in debug builds too. That is wrong:
a Java 8 consumer that does not minify builds fine at 27, 28 and 29, so
for them the floor is genuinely new. The earlier evidence came from a
test consumer configured with Java 17 source compatibility, which hits
AGP's separate Java 9 rule at the same threshold and masked the
difference. The CHANGELOG is the accurate record; the commit message is
not being rewritten because those commits have already been reviewed.

Reported by horgh.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
retries=2 and distributionSha256Sum are hand edits to a generated file,
with no Wrapper task configuration anywhere and no upgrade runbook. The
checksum is safe by accident: Gradle refuses to regenerate the wrapper
for a new version while it is set unless
--gradle-distribution-sha256-sum is passed. retries was not safe at all.

Confirmed both directions by regenerating the wrapper. Without a Wrapper
task configuration, `./gradlew wrapper --gradle-version 9.6.1` silently
rewrote retries=2 back to retries=0. With the configuration added to the
root build file, regeneration reproduces the file byte for byte. 4461a1b
on this branch is where the original loss happened: regenerating for the
9.6.1 upgrade is what emitted retries=0 in the first place.

Also moved distributionSha256Sum above distributionUrl. Gradle writes
these keys alphabetically, so the previous position would have produced
an unrelated reorder hunk on the next regeneration.

README.dev.md gains an "Upgrading Gradle" section, because the failure a
maintainer hits is a checksum GradleException with no repo-side guidance,
and the tempting fix is to delete the line that is protecting them.

Reported by horgh.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The two steps added in b259f44 close independent verification gaps, but
ran serially with no condition, so an R8 failure meant never learning
whether the androidTest sources still compile. On a toolchain upgrade,
which is what those steps are for, that turns one red run into several.
Both now run unless the job is cancelled, and the cheap androidTest
compile goes first.

The artifact upload collected only test reports, so when the R8 step
fails the files that explain it -- missing_rules.txt, configuration.txt,
mapping.txt and any lint report -- were discarded with the runner,
leaving whatever R8 truncated into the console log. Missing-class errors
in particular print long reference lists.

Also set validate-wrappers explicitly on setup-gradle. It defaults to
true, but this branch replaces gradle-wrapper.jar and the only integrity
check for that was an invisible default that an action upgrade could
change without a diff.

Reported by horgh.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The baseline fetch used URI(url).toURL().openStream() with no timeouts,
so a stalled connection to Maven Central hangs the api-compat job until
GitHub's six-hour limit instead of failing. Adds a 30s connect and 60s
read timeout.

Not pinning a checksum for the baseline: it is a different artifact every
release, so dev-bin/release.sh would have to maintain the expected value,
and the bytes are fetched over HTTPS from Maven Central and only feed a
CI-side comparison. Resolving the baseline through a real Gradle
configuration so that dependency verification covers it would be the
better fix if this is worth hardening further.

Reported by horgh.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
All pre-existing, none introduced by the upgrade, but CLAUDE.md is the
file agents read first so wrong instructions there are expensive.

CLAUDE.md:

- The manual setup told developers to set org.gradle.java.home in
  gradle.properties. That key is not there and never has been; the JDK is
  pinned by mise.toml. The surrounding paths were another developer's home
  directory.
- The "Resource mipmap/ic_launcher not found" common issue describes an
  error no build can produce: nothing references @mipmap/ic_launcher and
  the sample manifest has no android:icon. Removed rather than half-fixed.
- "CI runs it on every push" was wrong: ci.yml is push to main plus pull
  request, so a feature-branch push with no PR runs nothing. Someone
  relying on that would believe R8 had been exercised when it had not.
- Tempered the claim that assembleRelease "exercises R8 against the SDK
  and its consumer ProGuard rules": a green run proves R8 completed, not
  that the keep rules still preserve what kotlinx.serialization needs,
  which fails at runtime in consumer apps rather than at build time.
- Noted that AGP 9 runs :device-sdk:test once rather than twice, which was
  previously recorded only in a commit message.
- "Build debug variants (skips minification issues)" contradicted the
  Release Build MinifyEnabled section 240 lines below.

SETUP.md documented :device-sdk:publishReleasePublicationToMavenCentral-
Repository, which does not exist for the Vanniktech plugin, plus
signing.keyId / secretKeyRingFile properties that are never read because
signing uses useGpgCmd(). Points at README.dev.md and the real task.

README.md's new compileSdk bullet sat directly under "Android API 27+"
with no subject change, inviting the reading that support for API 27-29
devices was dropped. It was not.

Reported by horgh.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@oschwald

oschwald commented Jul 25, 2026

Copy link
Copy Markdown
Member Author

Claude here, replying on behalf of @oschwald.

Thorough review — the parts verified against real artifacts (wrapper jar checksum, AGP bytecode, the published 0.3.1 AAR) were straightforwardly useful, and two findings changed the branch materially. Seven new commits, 51f9dbc..40d7e8a. No fixups: these commits have had human review now, so everything lands forward.

Every inline thread has a per-finding reply, so this comment only covers what the threads can't: the commits, and the six findings from the review body that had no anchor in the diff.

The seven commits

Commit What
d260d8e Harden the AAR metadata check — whole-file comparison, builds its own AAR, .kotlin_module witness, proguard.txt
f6457e2 Correct comments this branch got wrong
ed1ab49 CHANGELOG remedy, plus the correction to 5d7cfba's message
50e3749 Keep the wrapper settings across regeneration
75d21ab Stop CI steps masking each other
04ebfc3 Bound the japicmp baseline download
40d7e8a Fix documentation that describes things that are not true

Both new guards were verified by counterfactual rather than assumed. The metadata check now fails on a changed value and on an unexpected extra key — the old version was blind to additions, which was the actual hole. Removing the moduleName pin produces META-INF/device-sdk.kotlin_module, which the new assertion catches directly. And without the Wrapper task config, ./gradlew wrapper --gradle-version 9.6.1 really did rewrite retries=2 back to retries=0.

The six findings with no inline anchor

Finding Disposition
ci.yml:24validate-wrappers relied on an invisible action default Set explicitly in 75d21ab
CLAUDE.md:237,254org.gradle.java.home claim, other developer's paths Removed in 40d7e8a; the JDK is pinned by mise.toml
CLAUDE.md:36 — "skips minification issues" contradicts the rewritten section Fixed in 40d7e8a
ci.yml:33-34:device-sdk:test passes when it runs zero tests STF-1190, item 6. This branch is the proof, as you noted
device-sdk/proguard-rules.pro — applied to nothing, ever STF-1190, item 6
Commit ordering — f8c68d4 references what e8acd1e adds next No action; resolves on merge, and reordering would rewrite reviewed history

Deferred

STF-1190 collects the eight items that are real but not this branch's doing: floor re-verification in CI, the metadata check as a Gradle task wired into check, pinning AGP 9's built-in Kotlin compiler, asserting DeviceData$$serializer survives R8, running instrumented tests plus the missing runner dependency and GpuCollector coverage, the two rows above, and the baseline under dependency verification. Reasoning is in the individual threads.

Declined

Four, argued in their own threads: the hard floor over -dontwarn, rewriting the 0.3.1 CHANGELOG entry, sha256-pinning the japicmp baseline, and the **Breaking:** label.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@CLAUDE.md`:
- Around line 245-246: Insert a blank line between the `local.properties` list
item and its nested code fence in `CLAUDE.md` to satisfy Markdownlint MD031,
leaving the properties example unchanged.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: ac80dffb-8474-453a-858f-cd7d80e12b8d

📥 Commits

Reviewing files that changed from the base of the PR and between 54b499e and 40d7e8a.

📒 Files selected for processing (11)
  • .github/workflows/api-compat.yml
  • .github/workflows/ci.yml
  • CHANGELOG.md
  • CLAUDE.md
  • README.dev.md
  • README.md
  • SETUP.md
  • build.gradle.kts
  • device-sdk/build.gradle.kts
  • gradle.properties
  • gradle/wrapper/gradle-wrapper.properties

Comment thread CLAUDE.md
Comment on lines 245 to 246
3. `local.properties` file (gitignored):
```properties

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Add a blank line before the nested code fence.

Markdownlint reports MD031 because the fence immediately follows the list item.

🧰 Tools
🪛 markdownlint-cli2 (0.23.0)

[warning] 246-246: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@CLAUDE.md` around lines 245 - 246, Insert a blank line between the
`local.properties` list item and its nested code fence in `CLAUDE.md` to satisfy
Markdownlint MD031, leaving the properties example unchanged.

Source: Linters/SAST tools

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Claude here, replying on behalf of @oschwald.

Applied in 19b89c8, though for a different reason than the one given.

Markdownlint is not this repo's markdown standard. .precious.toml runs prettier --parser markdown --prose-wrap always over **/*.md, alongside ktlint for Kotlin and yamllint for YAML; there is no markdownlint anywhere in the repo or its pre-commit hooks. I checked both forms against prettier and it accepts each of them, so MD031 was not failing anything here. The violation is also pre-existing rather than introduced by this branch — those lines are context in the diff, not changes.

The reason worth acting on is internal consistency: SETUP.md already separates a numbered list item from its nested fence with a blank line in all three places it does this (lines 243, 251, 257), so CLAUDE.md was the only file doing it the other way. Fixed there, in its own commit, since it is formatting rather than the doc-accuracy work the surrounding commits are doing.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 14 out of 16 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

gradlew.bat:68

  • Same issue as above: this error path does not terminate the script, so it can fall through to :execute and attempt to run an invalid %JAVA_EXE%. Replace with exit /b 1 (or goto :eof) to stop immediately.
"%COMSPEC%" /c exit 1

Comment thread gradlew.bat
echo Please set the JAVA_HOME variable in your environment to match the 1>&2
echo location of your Java installation. 1>&2

"%COMSPEC%" /c exit 1

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Claude here, replying on behalf of @oschwald.

Your control-flow analysis is correct, but this is vendored Gradle output, so it is not getting patched here.

The fallthrough is real. There is no goto after either "%COMSPEC%" /c exit 1, so on the "no JAVA_HOME, no java on PATH" path execution continues into :findJavaFromJavaHome. Traced through: JAVA_HOME is undefined there, so JAVA_EXE becomes /bin/java.exe, if exist fails, and the script prints the second and now misleading error — "JAVA_HOME is set to an invalid directory:" with an empty path — before falling into :execute.

Two things bound the impact:

  • It is a misleading-message bug, not a silent-success one. :execute tries to run /bin/java.exe, that fails, and call :exitWithErrorLevel propagates a non-zero status. No build can pass because of this.
  • It only triggers on Windows, and only when Java is absent or misconfigured.

The reason for not fixing it is that gradlew.bat is generated by Gradle's own wrapper task, not written by us. I verified the committed file is byte-identical to what Gradle 9.6.1 produces: running ./gradlew wrapper in a scratch worktree regenerated gradlew.bat with an empty diff (only gradle-wrapper.properties changed, and separately at that). Hand-editing it would be reverted by the next wrapper regeneration and would leave the checked-in script diverging from upstream for as long as it survived.

This belongs upstream with Gradle rather than in this repo's history.

Comment on lines +65 to +70
manifest=$(unzip -p "$aar" AndroidManifest.xml)
echo "$manifest"
if ! grep -q 'android:minSdkVersion="27"' <<<"$manifest"; then
echo "::error::expected minSdkVersion 27 in the AAR manifest"
exit 1
fi

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Claude here, replying on behalf of @oschwald.

This conflates AARs with APKs. AndroidManifest.xml is binary AXML inside an APK; inside an AAR it is plain text, because the AAR is a library intermediate that the consumer's build still has to merge and only then compile. Reading it straight out of the published artifact:

$ unzip -p device-sdk/build/outputs/aar/device-sdk-release.aar AndroidManifest.xml
<?xml version="1.0" encoding="utf-8"?>
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
    package="com.maxmind.device" >

    <uses-sdk android:minSdkVersion="27" />

    <!-- Required permissions for device data collection -->
    <uses-permission android:name="android.permission.INTERNET" />
    ...

So the grep is reading text, not stumbling over binary, and the step passes against the real artifact — verified by running it verbatim.

Switching to aapt2 dump xmltree would mean putting the Android build-tools on PATH in this workflow and depending on an SDK component it does not currently need, to parse something that is already text. That is a net loss.

Keeping the current check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants