Repository navigation
Stop an ordinary AndroidX dependency from failing the build on a duplicate Kotlin class - #5649
Merged
Merged
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d2e5b6c34
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
|
Developer Guide build artifacts are available for download from this workflow run:
Developer Guide quality checks: |
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Contributor
Cloudflare Preview
|
Collaborator
Author
|
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 160 screenshots: 160 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 181 screenshots: 181 matched. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 276f77ea34
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2dd8e7e274
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0748f9b9d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
|
Compared 144 screenshots: 144 matched. |
Collaborator
Author
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1348c5ffec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
|
Compared 217 screenshots: 217 matched. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef649457f6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8076a68aa6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2fd2b2a7f6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39db849342
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…icate Kotlin class
Kotlin 1.8.0 folded kotlin-stdlib-jdk7 and kotlin-stdlib-jdk8 into
kotlin-stdlib and left the two jdk artifacts as empty shims. Gradle resolves
every module's version on its own, so a graph reaching kotlin-stdlib at 1.8 or
newer through one path and kotlin-stdlib-jdk8 at something older through
another ends up with two real jars carrying the same classes:
Duplicate class kotlin.collections.jdk8.CollectionsJDK8Kt found in modules
kotlin-stdlib-1.8.22.jar (org.jetbrains.kotlin:kotlin-stdlib:1.8.22) and
kotlin-stdlib-jdk8-1.6.21.jar (org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.21)
Nothing Kotlin-shaped in the app is needed to produce it, which is what makes
the report unreadable from the app side: com.android.billingclient:billing:9.1.0
does it on its own, reaching kotlin-stdlib 1.8.22 through
androidx.core:core -> core-ktx and kotlin-stdlib-jdk8 1.6.21 through
androidx.core:core -> lifecycle-runtime -> kotlinx-coroutines-android. The app
declares one library and the failure names two Kotlin artifacts it has never
heard of. Our own androidx.appcompat default puts a second kotlin-stdlib-jdk8
branch in the graph too, so the exposure is not confined to billing.
Gradle would normally sort this out. From 1.9.22 kotlin-stdlib publishes module
metadata constraining kotlin-stdlib-jdk7 and jdk8 to 1.8.0 -- exactly the
alignment added here. The 1.8.x line, which is what the current AndroidX
releases resolve to, publishes no .module file at all, only a POM, and a POM
cannot express a constraint. So on 1.8.x nothing tells Gradle the two artifacts
overlap. This supplies for 1.8.x what JetBrains supplies from 1.9.22 on.
Written as a constraint rather than a force: it raises a version, never lowers
one, and never pulls a module into a graph that does not already contain it. An
app with no Kotlin anywhere resolves exactly as before. Skipped when the Kotlin
Gradle plugin is applied, since it performs the same alignment itself and the
Kotlin version such a build compiles with can be older than the floor. Skipped
when the app already names either jdk artifact or the Kotlin BOM in its own
Gradle build hints, and switchable with android.kotlinStdlibAlignment=false.
Verified against the real graph with Gradle rather than by reading POMs. Both
coordinates resolved from google() + mavenCentral() for billing:9.1.0 plus
appcompat:1.6.1:
without the block: kotlin-stdlib:1.8.22, kotlin-stdlib-jdk7:1.6.21,
kotlin-stdlib-jdk8:1.6.21 <- two real jars, the failure
with the block: kotlin-stdlib:1.8.22, kotlin-stdlib-jdk7:1.8.0,
kotlin-stdlib-jdk8:1.8.0 <- shims, no duplicate
and for an app whose graph has no Kotlin in it, zero org.jetbrains.kotlin
modules either way. jdk7 resolving to 1.6.21 alongside jdk8 is why both are
aligned and not just the one the reports name.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The build hint catalog's doc text is rendered into the developer guide, where Microsoft.Contractions is an error rather than a suggestion, so "It is expressed as a Gradle constraint" failed the prose gate on a file nothing in the tree edits by hand. Reproduced locally against the rendered table rather than guessed at: vale over docs/developer-guide/_generated-build-hints.adoc reports the one alert with the old wording and none with this one, and LanguageTool runs clean with status ok (not the "Detected java 1.8" fail-open). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two review findings, both real, both verified against a resolved Gradle graph rather than reasoned about. Skipping whenever a Kotlin plugin was applied was too broad. Only 1.8 and newer align the jdk stdlib variants themselves; on the android.useGradle8=false path this builder selects 1.7.22, which does not. Measured: plugin 1.7.22 alone stdlib 1.7.22 + jdk7/jdk8 1.7.22 no duplicate plugin 1.7.22 + billing 9.1.0 stdlib 1.8.22 + jdk7/jdk8 1.7.22 DUPLICATE the same, with the block stdlib 1.8.22 + jdk7/jdk8 1.8.0 fixed The middle row is worse than a transitive accident: the 1.7 plugin ADDS kotlin-stdlib-jdk8 at its own version, so the pre-merge real jar is guaranteed present rather than merely possible. The test is now the applied plugin's version, and an unreadable one -- kotlin-gradle-plugin:$kotlin_version parses to nothing -- counts as "does not align" so the block is written rather than skipped. That costs one case, stated in the class comment rather than left to be discovered: on the same pre-1.8 path, an app whose graph has no merged stdlib did not need the block and gets its stdlib raised to 1.8.0 anyway, newer than the compiler in use, which Kotlin warns about. Gradle cannot express a constraint conditional on what another module resolved to, so the choice is a warning where it was not needed against a failed build where it was. Suppression is now per artifact. jdk8 depends on jdk7, so an app pinning jdk8 raises jdk7 with it -- but an app pinning jdk7 leaves jdk8 exactly where the graph put it, and dropping the whole block there left the original duplicate intact with its fix switched off. Safe to split because the two jars' class sets are disjoint (kotlin.jdk7 / kotlin.io.path against kotlin.collections.jdk8 / kotlin.streams.jdk8), so constraining one and not the other cannot make a new duplicate. The Kotlin BOM still suppresses both, since it aligns the whole group. Three new cases cover this, and all three fail against the previous behaviour -- checked by reverting each half in turn, not assumed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A BOM manages the whole org.jetbrains.kotlin group, jdk7 and jdk8 included, so it looked like the one marker that could safely suppress both constraints. It is not, unless it is new enough: a platform contributes constraints, and the highest version still wins, so a BOM can raise the jdk artifacts but cannot pull kotlin-stdlib back down. Measured against a graph that wants stdlib 1.8.22: no BOM stdlib 1.8.22 + jdk7/jdk8 1.6.21 duplicate kotlin-bom 1.7.22 stdlib 1.8.22 + jdk7/jdk8 1.7.22 STILL a duplicate kotlin-bom 1.9.22 all 1.9.22, jdk artifacts shims safe 1.7.22 + this block stdlib 1.8.22 + jdk7/jdk8 1.8.0 fixed So the BOM is now tested the same way the Kotlin Gradle plugin already is, by version rather than by presence, and through the same predicate -- the two questions turned out to be one question asked about two things. An unreadable version reads as "does not align" for both, so kotlin-bom:$kotlinVersion gets the block rather than silently losing it. Worth recording that the first probe of this said the opposite. A bare Gradle configuration with no attributes cannot select a platform's constraint variant, so the BOM appeared to do nothing at any version and the finding looked wrong. It was the probe that was wrong. Anything measuring platform behaviour needs a configuration with real usage attributes, which is what the numbers above use. Three new cases, all three failing against the previous behaviour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… anywhere
Two review findings, one on each repo, with the same root cause: the check asked
whether a string occurs in the app's Gradle text rather than whether the app
actually declares anything. Both near misses switch the alignment off for an app
that pinned nothing, which is the duplicate-class failure this change exists to
prevent:
// implementation platform('org.jetbrains.kotlin:kotlin-bom:1.9.22')
exclude group: 'org.jetbrains.kotlin', module: 'kotlin-stdlib-jdk8'
The first is not a declaration. The second is the opposite of one -- a Gradle
exclusion applies only to the dependency edge it is written on, so an
independent path still brings the class-bearing jar it names.
Comments are now stripped before matching and exclusion lines are dropped, and a
declaration has to be spelled as one: the colon-joined coordinate or the map
form. Anything else falls through to "not declared", which is the safe
direction -- emitting a constraint the app did not need only raises an artifact
to a shim, while skipping one it did need fails the build. The BOM's version is
read from the same active text, so a commented-out BOM cannot supply the version
that suppresses the block.
One trap worth naming, because the obvious implementation has it: stripping from
every "//" cuts `maven { url 'https://...' }` in half, and these fragments do
carry repository URLs. A "//" only opens a comment when it does not follow a
colon, and there is a case pinning that.
Four new cases. Two fail against the previous behaviour; the other two pin
things the stricter matching could plausibly have broken -- the map form still
counting as a pin, and a URL still not being a comment.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… list android.supportv4Dep is written into the generated dependencies block a few lines below the constraints, and it was not among the fragments the alignment was told about -- so an app pinning a jdk artifact through that hint would have had the pin ignored and the constraint written over the top of it. Fixed by taking the enumeration from ShieldInjector's GRADLE_TEXT_HINTS, which is this tree's list of hints interpolated into a Gradle file, rather than from the ones that came to mind. Everything else on that list lands in buildscript, repositories or the android block, where a dependency cannot be declared, and aarDependencies is generated from .aar filenames and cannot express a version. The new check reads the builder's source, because an omission is invisible to a test that only exercises what is passed. It took two goes to make it real, and both failures are worth recording since they are the ordinary way this kind of check ends up proving nothing: - Matching the bare hint name passed with the argument deleted, because the comment above the argument list names android.supportv4Dep too. It matches the call form now. - Slicing the call to the first "));" cut the closing paren off the LAST argument, so that fragment never matched and the check failed for a reason unrelated to what it tests. It slices to the statement terminator now. Verified in both directions: passing on the real source, failing when the argument is deleted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…mmented plugins Two more findings of the same shape as the last round -- text that mentions a coordinate is not a declaration that governs the build -- plus one gate left where it is, on purpose. A declaration on a variant or test configuration does not reach the one the constraints are written on. debugImplementation of a new-enough BOM constrains the debug variant alone, so suppressing on it removed the constraint from the release build that still needed it, and the release build is the one that ships. The variant forms camel-case the configuration they derive from, so requiring the configuration's own lowercase spelling excludes debugImplementation, releaseImplementation and testImplementation without listing them and cannot be defeated by a variant name nobody thought of. api is accepted alongside it, since an api declaration is a real pin on the main variant. The Kotlin plugin version had the same hole through a different parser. HealthManifestFragments takes the first bare substring match, so a commented-out 1.8+ plugin sitting above an active 1.7.x one was read as the applied version and the alignment was skipped for a build whose real plugin does not align. The builder strips comments before parsing now, through the same helper this class already used on its own fragments. Not taken: widening the gate from Gradle 6 to 4.6. The reasoning is now a comment on the gate rather than only in a review thread, because the next reader will ask the same question. 4.6 selects AGP 3.2.0, which cannot compile against a compileSdk the current AndroidX releases require, and that path is given appcompat 1.0.0, whose graph carries no Kotlin at all -- a merged stdlib cannot appear there. Widening would put an untested constraints block into AGP 3.x builds that work today to fix a clash they cannot have, and the two directions are not symmetrical: too narrow leaves an ancient build with a failure it already had, too wide breaks a build that currently succeeds. Three new cases, and the two behaviours were checked by reverting each in turn. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lared
Two findings, both the same crude normalization rather than two bugs, and both
ending the same way: an app's explicit pin ignored and the constraint written
over the top of it, which is the opposite of what naming the artifact in a build
hint is documented to do.
implementation( configuration and coordinate
'org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.7.22' on different lines
)
implementation('...kotlin-stdlib-jdk8:1.7.22') { exclude group: 'x' }
a real pin, dropped whole
for containing "exclude"
So a line whose parentheses are still open is joined to the next, and a
statement is truncated at "exclude" rather than discarded: what precedes an
exclusion is the declaration, what follows it is the part that must not count. A
bare exclude line truncates to nothing, so it is still not a pin -- that case
kept its test and still passes.
Parenthesis counting ignores anything inside a string literal, so a coordinate
carrying one cannot unbalance it. Text left with parentheses open at the end of
a fragment is unbalanced Gradle, and its lines are kept separate rather than
glued into one: gluing would let a configuration from one statement and a
coordinate from another read as a single declaration, and suppression is the
direction that must never be reached by accident. That has its own case.
Four new cases. The two the review named fail against the previous behaviour;
the other two pin the ways this fix could have gone wrong.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3a84f8988
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A conditional swap between two coordinates of this family is a choice
between two of ours, and which arm runs is not readable here. Taking the
replacement let `def dep = '..jdk8:1.7.22'` followed by `if (useNew) dep =
'..jdk8:1.9.22'` read as merged-era, so the declaration below needed no
constraint -- and with the condition false the class-bearing 1.7.22 jar is
still there. The lower version is kept, as it is for two versions of the
same rich requirement.
The mirror of that shape was worse and turned up while checking this one:
`if (legacy) dep = '..1.7.22'` on ONE line was not read as an assignment at
all, because the walk began at `if` and stopped at its parenthesis, so the
name kept whatever it started with. The declaration walk steps past a
header whose body is on the same line now, and an assignment reached that
way is conditional, which is what makes keeping the lower one apply to it.
A selection rule may name its module by whole coordinate --
`withModule('org.jetbrains.kotlin:kotlin-stdlib-jdk8')` -- which is neither
the bare artifact name nor the group on its own, so a rule written that way
looked like it concerned nothing of ours and the rejected version was
written anyway.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9d99191d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A closure passed in parentheses is the same call as a trailing one, so
`componentSelection({ rules -> .. })` is a selection block -- requiring the
brace to follow the name missed it before anything could read its body.
The ARTIFACT in a coordinate selector has to be one of ours. Matching the
group prefix alone -- added one round ago for withModule -- read a rule on
`kotlin-reflect` as one on this family, and the block stood down for a
rejection that cannot touch either shim. A rule keyed on the group with no
artifact still counts, because it covers them.
The constraint handler takes a configuration and a notation as well, so
`constraints.add('implementation', 'g:a:1.7.22!!')` is a strict pin the app
really has; rejecting it because the receiver is not `dependencies` wrote
the shim constraints against it.
`subprojects { dependencies { .. } }` configures the children rather than
this application. The note beside the foreign-scope list already drew the
line -- allprojects includes this project, subprojects does not -- and only
the second half of it was acted on.
The withModule finding reported alongside these was already fixed by the
previous commit; verified against the current behaviour rather than assumed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 480af157a3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Whether a keyword was CALLED settles which one speaks; what it was called
with is a separate question. Falling through on a null let `require
'1.9.22'; strictly providers.gradleProperty('legacy').get()` report the
requirement, so a shim whose strict version may be pre-merge read as
merged-era, its own constraint was skipped, and the sibling was raised
around it.
Groovy accepts parentheses around a stored value, and one that did not
START with a literal was recorded as unknown -- so `def dep = ('g:a:1.7
.22!!')` left the pin invisible to whatever used the name.
android.gradle.androidx and android.xgradle_default_config run inside ONE
android { } closure in the script -- the first directly in it, the second
in its defaultConfig block. A synthetic closure each made a scope boundary
Gradle does not have. The scalars that sit between them in the script are
inside the shared argument now, which is what keeps the enumeration test's
ordering true.
That builder change was untested at first: with a closure each the
arguments are still in the right ORDER, so the enumeration test passed
either way and the new test only exercised the alignment with pre-wrapped
text. It reads the call and requires ONE argument to carry both hints now,
which is what fails when the closures are split.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f46dae4cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A destructured name is scoped like any other. Written straight into the
map, one declared inside a block outlived it -- so an inner
`def (dep, x) = [..]` shadowed an extra property for the rest of the file
and its coordinate was inlined into a later declaration that has nothing
to do with it. It is registered with the scope before it is recorded now,
exactly as a single declaration is.
An unqualified call is a declaration because a configuration is never
reached through a receiver -- but Groovy's output helpers are unqualified
too, so `println('g:a:1.7.22!!')` read as a strict pin and stood the block
down for a string the app was only logging.
That one is a list, and the reason is written beside it: the review asked
to restrict this to actual configuration invocations, and those cannot be
listed because an app may call a configuration anything. Naming the
PRINTERS instead makes it the complement of an open set, and it fails
safely -- a helper missing from the list keeps being read as a
declaration, which is today's behaviour and costs at worst the duplicate
an app already had. Listing configurations would drop a real pin the
moment a project names one nobody anticipated.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6140dd3309
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6140dd3309
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The feature is fifty lines: emit a Gradle constraint holding kotlin-stdlib-jdk7 and kotlin-stdlib-jdk8 at the version where their classes moved into kotlin-stdlib, so a graph that reaches an old shim transitively stops failing checkDuplicateClasses. Around those fifty lines had grown 4,400 more that read the app's own Gradle to decide whether the app had already pinned that family -- rich versions, maps, withModule, componentSelection, capabilitiesResolution, extendsFrom, addProvider, ext in three spellings, destructuring, ternaries, line continuations, CR-only line endings. Every review round found another spelling it misread, and none of them changed the answer for the graph the feature exists for, which names the shims nowhere. The question was never "parse this". It is "has the app decided this version itself", and the honest answer is a token check: the text names kotlin-stdlib and contains one of strictly, !!, force, reject, enforcedPlatform, useVersion, useTarget, substitute or failOnVersionConflict. It over-suppresses, on purpose -- leaving the floor out costs an app the duplicate class it already had, which android.kotlinStdlibAlignment=false does deliberately, while adding a floor over a real pin breaks a build that works today. The builder now passes every app-controlled fragment as plain text, with no wrapping or ordering, since a whole-text check has no use for either. The test suite goes the same way: 192 tests over parser spellings for 10 over what the feature promises.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76cb5e1251
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…f one list The class javadoc claimed the builder logs a notice when the app already holds the stdlib family. It did not: the stand-down happened inside constraintsBlock, which returns an empty string and says nothing, so the one case support would need to explain later was the silent one. The builder now collects the app-controlled fragments into a single appGradle array and uses it for both questions -- whether the app pins the family, and what to align over. Asking one over one set of fragments and aligning over another is the same defect wearing a different shape, so the test pins that too, and matches build hint names WITH their quotes: android.xgradle is a prefix of android.xgradle_default_config, and a bare contains() stayed true after the argument was deleted. The catch is kept and its comment corrected. It no longer guards a scanner -- there is no indexing left to get wrong -- but the block is an optimisation over a build that already worked apart from one duplicate class, and it runs on every AndroidX build. Three lines buy the difference between believing it cannot fail a build and knowing it cannot.
build-test has been failing intermittently with one test out of 6074 reporting "timed out after 5000ms; edt=display-not-initialized" -- a different class each time, never reproducible locally. The harness has been patched twice for it, and the comments there record the symptom accurately but treat it as a test-infrastructure problem. It is not. A thread that has left mainEDTLoop's dispatch loop is still isAlive() for the whole of its teardown, and init() decided whether to start a dispatch thread on exactly that evidence. So the ordering is: 1. the old generation's EDT leaves the loop and is descheduled 2. init() sees INSTANCE.edt alive, adopts it, starts nothing 3. the old thread resumes and finishes dying The new generation now has no dispatch thread at all. Everything it queues waits forever, and Display.isInitialized() answers false while codenameOneRunning stays true -- a state init() cannot repair, since it guards on that flag. Every test in the class then times out. The departing thread now publishes the fact rather than leaving it to be inferred from isAlive(): it clears edtDispatching the instant it stops dispatching, ahead of a teardown that can take arbitrarily long, and init() treats a non-dispatching thread as no dispatch thread. It stays the recorded EDT until the very end, because the teardown is meant to run AS the EDT -- disposeAll() is there to dispose windows on the thread their tree expects, and clearing edt early would make isEdt() false for exactly that call. It also tears down the implementation it was serving, read at loop exit, rather than whatever the static field points at by the time the teardown gets there. Read at loop exit and not at loop entry: a thread can serve more than one generation, because an init() while it is still dispatching adopts it legitimately. EdtHandoverTest holds the window open deterministically with an implementation that blocks inside deinitialize(). It fails on master in 5.5s (the dispatch never happens) and passes here in 0.6s; reverting either the edtDispatching check or the late clearing of edt fails it again, on that assertion.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b7e7690b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Three findings from review, all in the dangerous direction -- a floor written over a version something else is holding down. The serious one is our own doing. When the project has Kotlin sources this builder applies a Kotlin Gradle plugin and declares the stdlib at the compiler's version, which on the Gradle 6 and 7 path is 1.7.22. The 1.8.0 shims depend on stdlib 1.8.0, so raising them pulls the base stdlib up with them and the 1.7.22 compiler is then reading a stdlib newer than itself: "Module was compiled with an incompatible version of Kotlin". That turns a Kotlin app which builds today into one that does not, on the common path, and the generated declaration carries no pinning word so nothing stood the alignment down. It now stands down whenever this project compiles Kotlin -- the plugin owns that family, and the alignment exists for the Java-only graph that reaches the shims transitively and names them nowhere. The other two are gaps in the vocabulary. resolutionStrategy has a setter as well as a command, and a case-sensitive search for "force" finds `force` and misses `setForcedModules`, so the search now lower cases the text -- with Locale.ENGLISH, since a Turkish default turns "STRICTLY" into a dotless-i word that matches nothing, a trap already commented in this builder. And `require` joins the list for its bounded form: `require '[1.7,1.8)'` excludes the floor, so demanding 1.8.0 leaves nothing that satisfies both. The unbounded form is soft and would be raised happily; standing down for it too is the cheap side of the trade this whole guard is built on. Each of the four is covered by a test that fails when the change is reverted, including the builder passing hasKotlinSources -- asserted on the whole argument list, because the name also appears in the log branch above it and a looser check stayed true after the argument was replaced with a literal.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fcadc1804
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…as a pin Two findings, both correct, both about a gap left rather than a gap missed. The EDT handover was narrowed, not closed. Leaving the dispatch loop and announcing it were still two steps, so an init() landing between them saw a live thread with edtDispatching still true, adopted it, started nothing -- and the departing thread then captured the incoming implementation and tore that down instead. The window went from the whole teardown to a few instructions, which is exactly the width that bit us on a loaded runner in the first place. Now there is one exit and it is taken under `lock`: the thread reads codenameOneRunning, captures the implementation it served, and clears the flag as a single event. init() decides under the same monitor and claims the flag there, then creates the thread outside it, because setThreadPriority reaches the platform's own UI thread on some ports and holding the lock across that would trade the race for a deadlock. Two orderings remain and both are right: either the thread has left, and init starts a replacement, or it has not, and it reads the codenameOneRunning that init set and keeps dispatching for the new generation. The second is the stdlib guard. A version range needs no keyword at all -- 'org.jetbrains.kotlin:kotlin-stdlib-jdk8:[1.7,1.8)' reads as an ordinary coordinate and excludes the floor, so constraining to 1.8.0 leaves nothing that satisfies both. The signature is the comma, which appears nowhere else inside a version: digits on its left, digits or a closing bracket on its right. Map notation puts a quote to the left of every comma, which is the case this must not fire on, and it is tested in both directions.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8735b1f7b9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…th no comma Four more from review, and one pushed back on in the only place a reviewer will read it. The EDT clear had the same shape as the bug above it. Testing `edt == currentThread()` and then assigning null are two steps, and an init() publishing a replacement between them nulls a LIVE dispatch thread: the loop keeps running, but isEdt() stops recognising it, so callSeriallyAndWait() from the EDT waits on itself. Both sides now run under `lock` -- init builds and prioritises the thread on a local first, so the port call that reaches the platform's UI thread still happens outside it. Closed by construction rather than by test; the harness has no hook between those two statements. kotlin-stdlib-bom does not exist. The real coordinate is kotlin-bom, and the enforced-BOM test asserted against the made-up one, so it passed while every real enforced BOM went unseen. The family is now both names. hasKotlinSources scans src/main/java, and Kotlin can arrive from a source set it never looks at with the app applying the plugin itself -- in which case nothing names the stdlib and naming it cannot be the test. Applying a Kotlin Gradle plugin now stands the alignment down on its own. android.topDependency joins the scan while we are here: it is the buildscript block, this builder already reads it to decide whether to add a kotlin-gradle-plugin classpath, and leaving it out hid the clearest statement an app can make about this family. A range needs no comma either. [1.7.22] admits exactly one version, so a bracket against a digit is a range as surely as a comma between digits. Pushed back on dependency locking, in a comment beside the family check: a lockfile is a strict constraint and would genuinely conflict, but this builder writes the project from scratch and has no locking, no lockfile and no hint that ships one -- and locking with no lock state does nothing. The comment says what would have to change for that to become reachable.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35d818a236
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This PR is a Kotlin stdlib build hint. It had no business editing the EDT dispatch loop in the core framework, and the intermittent edt=display-not-initialized failure it was chasing is on master, not caused by anything here. Reverting Display.java and removing EdtHandoverTest keeps this change to the builders.
Measured against a real Gradle 6.5 and 8.5 resolving from Maven Central,
which is what this change should have started with. The graph the
feature exists for -- kotlin-stdlib 1.8.10 with kotlin-stdlib-jdk8
1.6.21 -- resolves to both jars and duplicates classes, reproduced. The
constraint this emitted did fix that. It also turned a strict pin on the
shim into "Could not resolve ... {strictly 1.6.21}", and a reject into
the same: a build that resolved before the alignment and not after it.
That is what the guard was for, and why it kept growing: a constraint
RAISES a version, an app can be holding one down, and enumerating the
ways it might be doing so from Gradle text has no end.
A capability states a fact instead -- from 1.8.0 up, kotlin-stdlib
provides what the shims provide -- and Gradle drops the redundant shim.
Nothing moves, so there is nothing to conflict with and nothing to
detect. The strict pin and the reject both resolve now. An enforced BOM,
a force, and a bounded range resolve as they always did. An all-1.7
project is untouched, because the capability is only declared from the
floor up, which is also why a Kotlin compiler older than 1.8 cannot be
affected. A graph with no Kotlin is inert, and applying the rule twice
is harmless. failOnVersionConflict with an old shim still fails -- and
fails identically with no script at all, so that graph is already
broken.
The measurements also killed things I had implemented on reasoning
alone. A bounded range does NOT exclude the floor: Gradle raised
kotlin-stdlib-jdk8:[1.6,1.8) to 1.8.0 and resolved. Neither does a
single-version range or a bounded require. force and enforcedPlatform
simply won over the constraint with no failure. All of that detection is
gone, along with the word list, the Kotlin toolchain scan and the
android.topDependency read, because the class now takes no input at all.
Test 5 of 7 asserts the property this rests on: the script requires no
version of anything.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7492d38d5a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The resolution tests proved the graph; this closes the gap I flagged.
checkDebugDuplicateClasses on AGP 8.1.4 reproduces the customer's error
exactly -- "Duplicate class kotlin.collections.jdk8.CollectionsJDK8Kt
found in modules kotlin-stdlib-1.8.10 and kotlin-stdlib-jdk8-1.6.21" --
and passes with this script.
The comparison that matters: the same Android build, with the app
pinning the shim strictly, succeeds with the capability and fails with
the constraint this replaced ("Could not resolve
org.jetbrains.kotlin:kotlin-stdlib-jdk8:{strictly 1.6.21}"). An all-1.7
project builds untouched.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb99f791a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Both findings reproduced before anything was changed, and the first one was real: stdlib 1.8.0 with a NEWER kotlin-stdlib-jdk8 1.9.0 resolves to 1.9.0 throughout when untouched, and to 1.8.0 with this script. The shim at 1.9.0 is empty and duplicates nothing; its only contribution is a requirement on stdlib 1.9.0, and evicting it took that with it. A silent downgrade of the base module. The cause was reusing the shims' own implicit capability. Every version of a shim holds it, including the empty ones, so a conflict was created where no duplicate exists -- and that conflict has no right answer. Measured: selecting the stdlib downgrades the base module, and selectHighestVersion() picks the shim and evicts kotlin-stdlib altogether, leaving a graph of empty shims with no stdlib in it. removeCapability does not remove an implicit capability, which was tried and measured too -- and my own try/catch hid that from me until I made the rule throw. So the capability is ours now: kotlin-stdlib at or above the floor declares com.codenameone:kotlin-stdlib-jdkN-superseded, and a shim BELOW the floor declares the same. Exactly the two modules that overlap hold it, so the conflict exists where the duplicate exists and nowhere else. The newer shim keeps its requirement and the graph matches the untouched one. The second finding is fixed by the same change and guarded anyway: a project candidate's id is a ProjectComponentIdentifier with no module property, and reading one throws MissingPropertyException. The lookup now checks ModuleComponentIdentifier first. With a capability only we declare, a project cannot hold it in the first place. Re-measured end to end: ten resolution scenarios on Gradle 8.5, three on Gradle 6.5, and checkDebugDuplicateClasses on AGP 8.1.4 for the duplicate, the strict pin and the newer shim. Three mutations, each caught.
Reproduced before changing it, because the comment on that gate says to. With android.useAndroidX=false explicitly set, AGP 8.1.4, kotlin-stdlib 1.8.10 beside kotlin-stdlib-jdk8 1.6.21: checkDebugDuplicateClasses fails exactly as it does with AndroidX on, and passes with this script. The gate was written on the reasoning that a non-AndroidX graph cannot reach a merged kotlin-stdlib, and that reasoning is simply wrong -- the duplicate has nothing to do with AndroidX. Those builds were left broken. The Gradle 6 floor stays, since capabilitiesResolution is the mechanism and AGP 3.x on Gradle 4.6 is a different world. A test reads the gate and fails if useAndroidX comes back. Pushed back on one finding, in a comment beside the emission since the thread is not read: selecting a stdlib reachable only THROUGH the shims was said to expand until the Gradle daemon exhausts its heap. kotlin-stdlib-jdk8:1.7.0 as the only route, plus a force to 1.8.0, resolves in seconds under a 512MB heap on Gradle 8.5 and on 8.14.2, and it is a graph this FIXES -- the baseline there carries the duplicate.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The failure
Reported by a customer who added Play Billing 9.1.0 to a project with no Kotlin
in it at all. Both halves of the duplicate come from that one dependency:
Kotlin 1.8.0 folded the jdk7/jdk8 stdlib classes into
kotlin-stdliband leftthe two jdk artifacts as empty shims. Gradle resolves each module's version
independently:
kotlin-stdlibwins at 1.8.22,kotlin-stdlib-jdk8stays at1.6.21, and both jars really carry
CollectionsJDK8Kt.Verified against the published jars:
CollectionsJDK8Ktkotlin-stdlib:1.8.22kotlin-stdlib-jdk8:1.6.21kotlin-stdlib-jdk8:1.8.0Why Gradle does not fix it itself
It normally would. From 1.9.22,
kotlin-stdlibpublishes Gradle module metadatawhose
jvmApiElements/jvmRuntimeElementsvariants constrainkotlin-stdlib-jdk7andkotlin-stdlib-jdk8to 1.8.0 — exactly the alignmentthis PR adds. The 1.8.x line, which is what current AndroidX resolves to,
publishes no
.modulefile at all (checked 1.8.0 / 1.8.10 / 1.8.20 /1.8.22 / 1.9.0 — all 404), only a POM, and a POM cannot express a constraint.
The change
A
constraintsblock appended inside the generateddependencies { }:A constraint, not a force: it raises a version, never lowers one, and never
pulls a module into a graph that lacks it. It is skipped when the Kotlin Gradle
plugin is applied (it does the same alignment itself, and a build compiling
Kotlin below the floor should not get a newer stdlib underneath it), skipped
when the app already names either jdk artifact or the Kotlin BOM in its own
Gradle build hints, and switchable off with a new
android.kotlinStdlibAlignment=falsehint (declared in the catalog).Gated on AndroidX + Gradle ≥ 6 — the block is written on
implementationandthe constraints DSL arrived in Gradle 4.6; the legacy support-library templates
predate both and predate the releases that produce the clash.
Verification
Resolved the real graph with Gradle rather than reading POMs —
billing:9.1.0+appcompat:1.6.1fromgoogle()+mavenCentral():For an app with no Kotlin in its graph: zero
org.jetbrains.kotlinmoduleseither way — the constraint is inert. Note jdk7 also resolved to 1.6.21, which
is why both artifacts are aligned rather than only the one the error names.
Also run locally:
KotlinStdlibAlignmentTest— 14 cases, mostly about the block not beingemitted; includes a source-text check that the builder still concatenates it
into the generated
dependenciesblock. Confirmed non-vacuous (deleting thatone term fails the test).
codenameone-maven-pluginsuite: 1861 tests, 0 failures.codenameone-maven-pluginandbuild-hint-catalog: 0 findings.scripts/check-build-hint-catalog.sh,scripts/gen-build-hint-annotations.sh --check,scripts/check-control-characters.py: clean.Companion
The BuildDaemon carries the twin of this change for cloud builds:
codenameone/BuildDaemon#PLACEHOLDER