-
-
Notifications
You must be signed in to change notification settings - Fork 47
ADFA-5126: Keep volatile build metadata out of module ABIs #1671
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: stage
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -68,7 +68,20 @@ val Project.simpleVersionName: String | |
| } | ||
| val buildTypeShort = if (buildType == "debug") "d" else "r" | ||
|
|
||
| val calendar = java.util.Calendar.getInstance() | ||
| // Derived from the commit being built, not the wall clock, so rebuilding a | ||
| // commit produces the same version string. With the wall clock, any two builds | ||
| // a minute apart produced different values, which changed BuildInfo and so | ||
| // build-info.jar on every build. See ADR 0012. | ||
| // | ||
| // Fixed to UTC deliberately: Calendar.getInstance() uses the JVM default zone, | ||
| // which would make the version a function of the builder's timezone as well as | ||
| // the commit, so the same commit built in two places would not agree. | ||
| val calendar = | ||
| java.util.Calendar | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could use an import at the top of the file |
||
| .getInstance(java.util.TimeZone.getTimeZone("UTC")) | ||
| .apply { | ||
| timeInMillis = CI.commitEpochSeconds(project) * 1000L | ||
| } | ||
| val month = calendar.get(java.util.Calendar.MONTH) + 1 | ||
| val day = calendar.get(java.util.Calendar.DAY_OF_MONTH) | ||
| val hour = calendar.get(java.util.Calendar.HOUR_OF_DAY) | ||
|
|
@@ -89,7 +102,12 @@ val Project.simpleVersionName: String | |
|
|
||
| val Project.releaseVersion: String | ||
| get() { | ||
| val raw = providers.gradleProperty("next_release_version").orNull.orEmpty().trim() | ||
| val raw = | ||
| providers | ||
| .gradleProperty("next_release_version") | ||
| .orNull | ||
| .orEmpty() | ||
| .trim() | ||
| if (raw.isNotEmpty() && !Regex("""^\d{2}\.\d{2}$""").matches(raw)) { | ||
| throw GradleException( | ||
| "Invalid next_release_version '$raw'; expected YY.ww (two digits, dot, two digits), e.g. 25.47", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,126 @@ | ||
| # 0012. Keep volatile build metadata out of module ABIs | ||
|
|
||
| - **Status:** Proposed | ||
| - **Date:** 2026-08-13 | ||
| - **Deciders:** Code On The Go team | ||
|
|
||
| ## Context | ||
|
|
||
| `:build-info` generates `BuildInfo.java` from a template and sits at the root of the | ||
| dependency graph. Five of its generated fields change from build to build: | ||
|
|
||
| ```java | ||
| VERSION_NAME_SIMPLE = "C-d-0810-1555" // wall-clock time, to the minute | ||
| VERSION_NAME_PUBLISHING = "C-d-0810-1555-98ea6f6a4-SNAPSHOT" // time + commit hash | ||
| VERSION_NAME_DOWNLOAD = "C-d-0810-1555-98ea6f6a4-SNAPSHOT" // time + commit hash | ||
| CI_GIT_BRANCH = "ci-bench" | ||
| CI_GIT_COMMIT_HASH = "98ea6f6a4" | ||
| ``` | ||
|
|
||
| All are `public static final String`. Java and Kotlin inline compile-time constants | ||
| into every consumer, so a constant's *value* belongs to the declaring module's ABI. | ||
| Every build therefore changed `:build-info`'s ABI and forced the whole project to | ||
| recompile. | ||
|
|
||
| Three of the five derive from the current time (`simpleVersionName` in | ||
| `ProjectConfig.kt` formats `C-{d|r}-MMDD-HHMM`), so this fires on **any two builds a | ||
| minute apart, even of an identical commit**. That is strictly worse than the commit | ||
| hash, and it is why the problem reproduces off CI. | ||
|
|
||
| Measured locally with a scripted no-change scenario - no source edit whatsoever, only | ||
| a different `GITHUB_SHA`: | ||
|
|
||
| ``` | ||
| 30 compileV8DebugKotlin <- every Kotlin module in the project | ||
| 12 kaptGenerateStubsV8DebugKotlin | ||
| 11 kaptV8DebugKotlin | ||
| ``` | ||
|
Comment on lines
+33
to
+37
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Add a language identifier to the fenced output block. The fence at Line 33 has no language identifier. This triggers MD040 in the supplied static analysis output. Use 🧰 Tools🪛 markdownlint-cli2 (0.23.2)[warning] 33-33: Fenced code blocks should have a language specified (MD040, fenced-code-language) 🤖 Prompt for AI AgentsSource: Linters/SAST tools |
||
|
|
||
| The same signature appears on CI (30 executed `compileV8DebugKotlin`). A build in | ||
| which nothing changed recompiles the entire tree. | ||
|
|
||
| The churn also propagates a second time. `common/.../BuildInfoUtils.kt` declares: | ||
|
|
||
| ```kotlin | ||
| const val BASIC_INFO = "${BuildInfo.INTERNAL_NAME} (${BuildInfo.VERSION_NAME_SIMPLE})" | ||
| ``` | ||
|
|
||
| A Kotlin `const val` is inlined too, so `:common`'s ABI churns as well and everything | ||
| depending on `:common` recompiles from there. | ||
|
|
||
| ## Decision | ||
|
|
||
| Generate the volatile fields with **non-constant initialisers**, so `javac` emits no | ||
| `ConstantValue` attribute and the values leave the ABI entirely: | ||
|
|
||
| ```java | ||
| public static final String VERSION_NAME_SIMPLE = volatileValue("@@VERSION_NAME_SIMPLE@@"); | ||
| ``` | ||
|
|
||
| The rule this encodes: **a value that changes between builds must never be a | ||
| compile-time constant.** Where it is declared matters less than whether it is | ||
| inlinable. | ||
|
|
||
| `:common`'s `BASIC_INFO` becomes a non-`const` `val`. This is not optional - a Kotlin | ||
| `const val` requires a compile-time constant initialiser, so it stops compiling until | ||
| corrected. | ||
|
|
||
| Stable fields (package name, repo coordinates, AGP versions, F-Droid flags) keep their | ||
| constant form. | ||
|
|
||
| Two related changes follow from the same invariant: | ||
|
|
||
| - `simpleVersionName` derives its timestamp from the commit being built rather than | ||
| the wall clock, so the generated source is a function of the commit. Format and | ||
| ordering are unchanged, so nothing product-visible moves. The calendar is fixed to | ||
| UTC, otherwise the version would be a function of the builder's timezone too. | ||
| - `:build-info`'s Jar sets `preserveFileTimestamps = false` and | ||
| `reproducibleFileOrder = true`. This is not optional in practice: with the timestamp | ||
| fixed, `BuildInfo.java` became byte-identical between rebuilds while the *jar* still | ||
| changed, because Gradle embeds per-entry timestamps by default. kapt tracks that jar | ||
| through an input property named `internalNonAbiClasspath` - jar bytes rather than the | ||
| ABI - so the ABI fix above cannot reach it and only a reproducible jar can. | ||
|
|
||
| ## Consequences | ||
|
|
||
| **Positive** | ||
| - A commit, or the clock advancing, no longer changes any module's ABI. Recompilation | ||
| is confined to modules whose sources actually changed: 1 Kotlin module for a no-op or | ||
| a leaf edit, 10 for a three-module edit containing one real ABI change. | ||
| - Gradle's build cache and up-to-date checks become effective for the first time. | ||
| - The invariant is enforced by the compiler rather than by convention: reintroducing a | ||
| `const val` over a volatile value fails the build. | ||
|
|
||
| **Negative / costs** | ||
| - `BuildInfo`'s volatile fields can no longer be used where Java or Kotlin requires a | ||
| compile-time constant (annotation arguments, `when` branch constants). None of the | ||
| current call sites need that. | ||
| - The `volatileValue()` indirection is unusual and invites "simplification" back into a | ||
| plain constant. The generated file carries a comment saying why. | ||
| - Values move from being inlined at each call site to a single static read. The runtime | ||
| cost is immaterial; the behaviour is unchanged. | ||
| - kapt still re-runs across its 11 modules: it resolves the full compile classpath | ||
| rather than the ABI-normalised one. Tracked by ADFA-4598 (kapt to KSP). | ||
|
|
||
| ## Alternatives considered | ||
|
|
||
| - **Move the fields into `:app`'s `BuildConfig`.** Considered first and rejected on | ||
| evidence: `:common` and `:editor` consume `VERSION_NAME_SIMPLE`, and neither can | ||
| depend on `:app`. It would have addressed only the two `CI_GIT_*` fields and left the | ||
| dominant, time-based churn untouched. | ||
| - **A separate `:build-info-git` leaf module.** Same defect - it isolates the git | ||
| fields but not the version fields that library modules genuinely need. | ||
| - **Drop the timestamp from `simpleVersionName`.** Attacks the root cause rather than | ||
| the propagation, and would help independently. Rejected *for this ADR* because the | ||
| version string is product-visible (Firebase release notes, tester-facing builds, | ||
| Jira), so it is a product decision rather than a build one. Worth revisiting. | ||
| - **Leave it and rely on the remote build cache.** Does not help: the compile tasks | ||
| miss the cache precisely because their compile classpath genuinely changed. | ||
|
|
||
| ## Related | ||
|
|
||
| - [0005](0005-per-abi-product-flavors.md) - the flavor dimension that multiplies every | ||
| build task, and so multiplies the cost of this churn. | ||
| - [Build and CI glossary](../process/build-ci-glossary.md) - *ABI change*, *ABI churn*, | ||
| *build graph health*. | ||
| - ADFA-5126 - the ticket, with the full before/after measurements. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| # Build and CI glossary | ||
|
|
||
| Vocabulary for build and CI work on Code On The Go. Terms are defined here so that a | ||
| word means one thing across code, tickets, PRs, and conversation. | ||
|
|
||
| This file is a **glossary only**. It holds no implementation detail and no decision | ||
| rationale - decisions live in [docs/adr/](../adr/), structure lives in | ||
| [ARCHITECTURE.md](../../ARCHITECTURE.md). | ||
|
|
||
| ## Terms | ||
|
|
||
| **Critical path** | ||
| The longest chain of work that must finish before CI reports a verdict. Work that runs | ||
| concurrently on another runner is not on the critical path even though it costs time. | ||
| Distinct from *runner occupancy*. | ||
|
|
||
| **Runner occupancy** | ||
| Total runner-minutes a single push consumes, summed across every job it starts. A push | ||
| can have a short critical path and high occupancy (two runners busy in parallel). | ||
| Occupancy is what makes other people's builds queue; critical path is what makes one | ||
| developer wait. Reducing one can increase the other. | ||
|
|
||
| **ABI change** (of a module) | ||
| A change to a module's public compile-time surface: signatures, public constants, | ||
| anything a dependent module compiles against. Dependents must recompile. Contrast | ||
| *non-ABI change* (a method body, a comment) where dependents need not recompile. | ||
| Java and Kotlin **inline** compile-time constants such as `static final String`, so | ||
| changing a constant's *value* is an ABI change even though the declaration is untouched. | ||
|
|
||
| **ABI churn** | ||
| An ABI change that carries no semantic meaning for dependents, forcing recompilation | ||
| for nothing. Build metadata stamped into a widely-depended-on module is the canonical | ||
| source - see [ADR 0012](../adr/0012-volatile-build-metadata-out-of-abis.md). | ||
|
|
||
| **Build graph health** | ||
| How closely the set of re-executed tasks matches the set of genuinely affected tasks. | ||
| Measured as the ratio of `executed` to `up-to-date`/`from-cache` tasks in Gradle's | ||
| summary line. Independent of hardware, and therefore comparable across machines - | ||
| unlike wall clock. | ||
|
|
||
| **Baseline** | ||
| A recorded measurement of the pipeline before a change, against which later iterations | ||
| are compared. A measurement is only a baseline if it was produced under the same | ||
| protocol and scenario as the runs compared to it. | ||
|
|
||
| **Scenario** | ||
| A deterministic, scripted source change of defined scope, used as a measurement | ||
| workload. Scenarios differ in blast radius - no-op, single leaf module, ABI change in | ||
| a core module, multi-module - so one pipeline produces a profile rather than a number. | ||
|
|
||
| **Warm workspace** | ||
| A checkout whose `build/` outputs and Gradle caches survive from a previous run. The | ||
| steady state of a self-hosted runner, and the state any representative measurement must | ||
| reproduce. Contrast a *cold* build, which no runner ever performs in practice. | ||
|
|
||
| ## Related | ||
|
|
||
| - [ARCHITECTURE.md](../../ARCHITECTURE.md) - module map, layering, tech stack. | ||
| - [docs/adr/](../adr/) - the decisions and their rationale. | ||
| - [CLAUDE.md](../../CLAUDE.md) - build and test invocations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: appdevforall/CodeOnTheGo
Length of output: 50381
🏁 Script executed:
Repository: appdevforall/CodeOnTheGo
Length of output: 28606
🏁 Script executed:
Repository: appdevforall/CodeOnTheGo
Length of output: 27430
🌐 Web query:
Gradle ProviderFactory.exec ExecOutput isIgnoreExitValue standardOutput asText non-zero exit behavior💡 Result:
In Gradle's
ProviderFactory.execAPI, the behavior ofExecOutput.standardOutput.asText.get()—or any attempt to access the output provider—is inherently tied to the process execution status [1][2]. If a process finishes with a non-zero exit code, calling.get()on the output provider will fail by default, throwing an exception [1]. This occurs because Gradle considers a non-zero exit code a process failure, and the provider cannot successfully resolve the output if the process itself is deemed to have failed [1]. To change this behavior and allow access to the output even when a process returns a non-zero exit code, you must explicitly configure the process to ignore the exit value [1]. This is done using theisIgnoreExitValueproperty within theexecconfiguration block [3][1]: providers.exec { commandLine("your-command") isIgnoreExitValue = true } OnceisIgnoreExitValueis set totrue, the process will not throw an exception on a non-zero exit code [3][1]. You can then safely call.get()on the output provider to retrieve the process's standard output [1]. If you also need to handle or inspect the exit code itself, you can access it viaExecOutput.result[1]: val execOutput = providers.exec { commandLine("your-command") isIgnoreExitValue = true } val output = execOutput.standardOutput.asText.get val exitCode = execOutput.result.get.exitValue Note thatproviders.execis designed for lazy, configuration-time execution where Gradle captures the output as a build input [4][2]. Because it is lazy, the process will not execute until you call.get()on one of theExecOutputproviders (likestandardOutput,standardError, orresult) [2][5].Citations:
Log the non-reproducible fallback.
When Git fails, returns no numeric timestamp, or parsing fails, this code silently uses
System.currentTimeMillis(). WithisIgnoreExitValue = true, inspectExecOutput.resultbecause a non-zero exit does not throw. Log the Git failure and the loss of reproducibility before applying the fallback.🤖 Prompt for AI Agents
Source: Coding guidelines