Check build hints at compile time instead of shipping them inert - #5586
Check build hints at compile time instead of shipping them inert#5586shai-almog wants to merge 99 commits into
Conversation
A build hint is a `codename1.arg.<name>=<value>` line that reaches a builder as
`request.getArg(name, default)`. Nothing checked the name, so a misspelling was
accepted, copied into the build request, never read, and silently discarded: a
green build with the setting simply not applied.
Our own agent reference had been shipping `android.xPermissions`,
`android.minSdkVersion` and `android.sdkVersion` for exactly that reason. The
builders read `android.xpermissions`, `android.min_sdk_version`, and nothing at
all.
Most hints can now be written as annotations on the application's main class,
where javac does the checking: a misspelled name is an unknown symbol, a wrong
value type is a type error, and a value outside a hint's supported set is an
unknown enum constant.
@iOS(newStorageLocation = true, themeMode = IosThemeMode.MODERN)
@android(minSdkVersion = 24, useAndroidX = true)
@desktop(titleBar = DesktopTitleBar.NATIVE)
public class MyApplication extends Lifecycle {
}
The builders are untouched: `BuildHintAnnotationProcessor` converts the
annotations back into the same key/value pairs and `CN1BuildMojo` merges them
before the command-line overlay, the CN1Lib merges and both preflights, so a
library still appends onto an annotation-supplied value and `-D` still wins.
`Simulator` publishes them as system properties at startup so `cn1:run` sees
hints that no longer live in the properties file.
The properties file is untouched too. It stays the way to set the long tail and
the open-ended families such as `android.permission.<NAME>` that an annotation
cannot express, with no new warnings or errors. Declaring one hint both ways is
a build error.
One catalog, five generated views
---------------------------------
The hint set had been described in five places that had drifted apart: a prose
table in the developer guide, a runtime scraper of that table in the Settings
tool that guessed each type by string-matching the description, a fifteen-entry
schema in the simulator, a fourteen-entry separator map in the plugin, and a
hand-written agent reference. Only 147 of ~520 names appeared in more than one.
`maven/build-hint-catalog` is now the single source of truth (529 hints: 457
mined from the builders, 56 documented-but-unread, 16 dynamic families; 82
exposed as annotation attributes). The annotations, the binding table the
processor reads back, the guide's table, the simulator's editor schema and the
agent reference are all generated from it. The guide's table goes from 208 rows
to 529 with no prose lost.
Enums are emitted only where the accepted set is demonstrable from the code that
reads the hint -- `HardeningPreflight` rejects an unknown `harden.level`,
`IOSDependencyManager` throws on an unknown `ios.dependencyManager`, and
`GenerateDesktopAppWrapperMojo` silently falls back to `native` on an unknown
`desktop.titleBar`, which is the failure this removes.
Generated projects
------------------
The archetype and all four initializr templates now carry the annotations, and
`cn1:migrate-build-hints` moves an existing project over. Eleven in-repo
projects are migrated. `java.version` deliberately stays in the properties file:
it picks the toolchain that compiles the class the annotations live on.
Gates
-----
`scripts/check-build-hint-catalog.sh` fails when code reads a hint the catalog
does not describe, and when our own docs or templates name one that no builder
reads. Its baseline is empty, so it is a hard gate rather than a ratchet.
`scripts/gen-build-hint-annotations.sh --check` fails on generated-file drift.
Both run in the Java 8 leg of PR CI.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![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: 8d2cfcfde3
ℹ️ 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".
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
`.gitignore` carries a repo-wide `**/build/*`. The catalog's package is `com.codename1.build.shared`, so all 13 of its sources sat under a path segment named `build` and `git add` silently skipped them. Only `pom.xml` was committed: the module built locally from the working tree and produced an empty jar in CI, which is why `codenameone-maven-plugin` then failed with `cannot find symbol` on `BuildHints` and nearly every job went red. The sibling `platform-feature-catalog` lives in the same package and is fine, because it was added before that rule existed -- tracked files stay tracked, so nothing ever pointed at the hazard. Un-ignore `build` when it is a Java package rather than a build output directory, with the rationale beside the rule so the next file added there is not lost the same way. `maven/core/build/*` and `CodenameOne/build/*` stay ignored. Also from review: - Every bare `open()` in the four Python scripts now uses a context manager, so the handle closes even if parsing or `json.dump` raises, and the writes state their encoding. - The generator no longer emits an IP literal as an annotation default. PMD reads `default "127.0.0.1"` as hardcoded configuration, and the default clause is documentation only -- the processor emits a hint solely for members the developer actually wrote -- so the value moves to the javadoc where it belongs. - Files the migration touched that never carried a copyright header now have the complete one. The archetype's `__mainName__.java` is excluded instead: it is a template for the user's own application class, and stamping a Codename One GPL header onto it would put our licence on their code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Pin generated projects to a version containing the annotations
The updated Initializr starter archives now import and use com.codename1.annotations.buildhints, but GeneratorModel.java:44 still generates projects pinned to CN1 7.0.267, whose core artifact predates this package. Consequently every newly generated barebones, Kotlin, Grub, or Tweet project fails compilation on the unresolved annotations unless the user manually changes the CN1 version; either defer these template changes or update the generated runtime/plugin version to the first release containing them.
ℹ️ 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 archived bootstrap ran generation at module scope, so gen_external.py's `import gen_catalog` -- which only wants three helper functions -- rewrote every catalog source as a side effect. Generation and its diagnostics now live in `main()` behind a `__main__` guard, and the module-level file reads became `load_license()` / `load_mined()` / `load_docs()`, so importing does no I/O and cannot fail on inputs the archived copy deliberately does not carry. Verified both directions: importing leaves the catalog untouched, and running the two scripts end to end still reproduces the committed catalog byte for byte. Also drops `json` and `subprocess` from check-build-hint-catalog.py. Both were left from an earlier version that shelled out to the miner instead of importing it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Compared 12 screenshots: 12 matched. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d727c7d976
ℹ️ 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".
|
Developer Guide build artifacts are available for download from this workflow run:
Developer Guide quality checks: |
|
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
|
Compared 149 screenshots: 149 matched. Benchmark ResultsDetailed Performance Metrics
|
Cloudflare Preview
|
|
Compared 149 screenshots: 149 matched. Benchmark ResultsDetailed Performance Metrics
|
|
Compared 149 screenshots: 149 matched. |
|
Compared 149 screenshots: 149 matched. |
|
Compared 149 screenshots: 149 matched. Benchmark ResultsDetailed Performance Metrics
|
|
Compared 181 screenshots: 181 matched. |
Three separate gates rejected generated output. Each is fixed in the generator so the class of problem cannot come back through a catalog edit. Unmappable characters. The prose is imported from the developer guide, which uses typographic punctuation, and `CodenameOne/src` is also compiled by an Ant javac step with ASCII encoding where a single em dash is `error: unmappable character for encoding ASCII` -- a build failure, not a warning. A Unicode escape would not have helped: javac expands `\uXXXX` before it strips comments, so the character reappears. `toAscii` now folds the punctuation that actually occurs, and *refuses* anything it has no mapping for rather than dropping it, because silently deleting a character from a hint's documentation is the worse outcome. Broken table. `ios.spm.packages` is documented as `identity|url|requirement`, and a bare `|` starts a new AsciiDoc cell, so asciidoctor reported "dropping cells from incomplete row" for the whole 529-row table. Cells are escaped now. Vale. The guide enforces the Microsoft style as errors, and the generated table feeds it, so the catalog's prose has to satisfy it too: contractions, no "and so on", no stray adverbs. A default value is not prose, though -- the one remaining hit was `android.file_paths`, whose default is an XML fragment -- so a quoted default now carries the `// vale-skip:` comment .vale.ini documents for individual false positives. Also fixes a data bug the guide exposed. The miner preserved Java escape sequences instead of decoding them, so `android.file_paths` and `android.facebook_permissions` recorded defaults containing literal backslashes that the build never sees, and those reached the rendered table. The miner decodes escapes and re-quotes safely, and the two catalog entries are corrected. 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: 96bff9038a
ℹ️ 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".
`killedThreadReportsItselfFinished` failed the Java 21 leg with "FormTest timed out after 5000ms; edt=initialized pendingSerialCalls=0". The waits in this class used a 5000ms deadline, which is exactly the `@FormTest` timeout in EDTTestInterceptor -- so on a loaded runner the poll loop consumed the entire harness budget and the interceptor fired first. The report then said only that the method timed out, with nothing about which condition never became true. The waits now use 2000ms, well inside the harness budget and still roughly two thousand times the ~1ms these threads actually take to stop. A genuine regression now fails on the test's own assertion, which names what went wrong. Pre-existing (the test arrived with #5526) and unrelated to the build hint work: core-unittests has no dependency on the JavaSE port, so none of the simulator registration in this branch runs there, this branch changes nothing under com.codename1.db or EasyThread, and the Java 8 leg passed the same commit. 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: a343fe3335
ℹ️ 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 mojo's defaultPhase does not bind it to a project -- the project's POM has to -- and nothing turns a build hint annotation back into a codename1.arg.* pair except the process-annotations goal. So migrating a project without that binding deleted working properties and replaced them with annotations no goal ever reads: the hints vanished from the build with no diagnostic anywhere. Five projects in this branch were already in that state. gamebuilder, docs/demos, video-builder and cn1playground bind the plugin but not that goal, so the binding is added. input-validation-app's common module has no build section at all, so its migration is reverted rather than inventing a lifecycle for a demo app. The goal now checks the reactor for the binding and refuses with the execution block to paste, so this cannot happen to anyone else. Three more from the same review: - The deletion pass recognized only `key=value`. `Properties.load` also accepts `key:value`, `key value`, escaped separators inside the key, and logical continuation lines; a declaration it failed to match was left behind while the annotation was added, so the next build failed with the duplicate-hint error this goal exists to prevent. Keys are parsed the way Properties.load defines them now, with a unit test per form. - The settings file was read as ISO-8859-1 and written back as UTF-8, turning any unrelated non-ASCII byte -- an accented displayName, say -- into mojibake. It is written back as ISO-8859-1. - cn1.androidTheme and cn1.nativeTheme are deprecated aliases of and.themeMode and nativeTheme, which the builders honour as fallbacks. Neither declared aliasOf, so conflict detection missed them and one value silently won. Also: the generation script rebuilt the generator only when its class was absent, so editing a catalog source and rerunning regenerated every view from the previous build's bytecode -- reporting success while ignoring the edit, and passing --check on a tree that was genuinely stale. It always rebuilds now. 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: 0edef42ca4
ℹ️ 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".
…plicating a hint docs/demos is the developer guide's snippet project: deliberately incomplete code fragments that illustrate @entity, @route, @AppIntent and @mapped. Binding process-annotations there put those snippets in front of the other processors, which correctly rejected six of them, so the migration is reverted and its two hints are back in the properties file. That the project omitted the goal was the point, not an oversight. The other three newly bound projects were checked rather than assumed: gamebuilder, video-builder and cn1playground each run process-annotations cleanly and emit 6, 3 and 5 hints respectively. Settings could still create the duplicate the migration is careful to avoid. In a generated project ios.themeMode and its neighbours are annotations, but the Build Hints UI decides a hint is inactive from the properties file alone and its Add button writes a property -- producing a second declaration that fails the next build. The tool now reads META-INF/codenameone/build-hints.properties, the file the processor writes on every build and deletes when the last annotation goes, and renders those hints read-only with the attribute that owns them: "Set by @iOS(themeMode) on the main class." An unbuilt project has no such file and behaves as before. Also fixes the SpotBugs finding this branch introduced: `backslashes % 2 == 1` in the continuation scan is false for negative odd numbers, so it is `!= 0`. The count cannot go negative, but the idiom is wrong regardless of that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Keep Initializr templates compatible with the pinned runtime
Every Initializr source archive now imports com.codename1.annotations.buildhints and uses the new annotations, while GeneratorModel.CN1_PLUGIN_VERSION still rewrites generated projects to 7.0.267, whose codenameone-core predates that package; the generated common POM also omits process-annotations. Consequently all newly downloaded Initializr projects fail compilation instead of receiving the defaults removed from common.zip's settings file. Leave these templates property-backed until Initializr targets the release containing this feature, or bump the generated version and bind the processor.
ℹ️ 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".
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
Every project the archetype and the initializr produce is pinned to a released Codename One version -- the initializr hardcodes 7.0.267 in GeneratorModel.CN1_PLUGIN_VERSION -- and no released core carries com.codename1.annotations.buildhints. So a generated project would import annotations that do not resolve and fail to compile before the user has written a line, and the settings those templates stopped declaring would simply be gone. The templates are reverted to exactly their previous state: the archetype's __mainName__.java and codenameone_settings.properties, and the initializr's common.zip and four source archives. They can move to annotations in a follow-up once a release containing the package is out. The generated build hint table is dropped from the agent skill reference for the same reason -- it documented a form those projects cannot use yet -- so the generator no longer rewrites markdown at all. What stays from that area is unrelated to annotations: the skill reference described build hints that no builder reads, so a reader copying them got a green build and no effect. android.xPermissions is spelled android.xpermissions, android.minSdkVersion is android.min_sdk_version, and android.sdkVersion, android.googlePlayVersion, build.compile, build.timeout, javascript.html5, javascript.bundleResources and ios.orientation do not exist at all. Those corrections are right for the published version too, and the catalog gate now holds our own documentation to them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Inside `${ ... }` the first quote starts a NEW literal rather than
closing the outer one, so `"${"@iOS(teamId = x)"}"` ended the string
early and exposed its contents as live code. In Settings that read as an
annotation nobody wrote, hiding the editor for a hint nothing owns --
indistinguishable from the tool being broken. In the processor the same
text read as a declaration nobody wrote.
Both scanners step over a template expression whole, matching braces so
a `}` inside the nested literal does not close it early, and recursing
through nested strings. Raw strings carry templates too, so a `"""`
inside one is a nested literal rather than the terminator.
Fixed in both parsers at once: this is the shape that has come back
repeatedly in this review, where one of them learns a language rule and
the other does not.
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: 1bf4691ef7
ℹ️ 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 nesting fix taught both scanners that a quote inside `${ ... }` opens
a nested string, but the expression is ordinary code and so it also holds
comments and char literals -- where a quote is neither. `${ /* " */ 1 }`
was read as opening a nested string, so the rest of the file was
swallowed: in the processor a live declaration after it was blanked and
its class dropped as an orphan, and in Settings every annotation after it
disappeared.
Comments nest in Kotlin, so the block form is matched by depth, and a
brace inside a comment no longer closes the expression either.
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: 2a64e926b9
ℹ️ 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".
`import ...Ios as Base` then `typealias AppIos = Base` is legal and still
compiles to our annotation, but the two kinds of alias were collected
into one list rather than composed: the typealias named the IMPORT alias,
which the closure had never heard of, so the hint read as unowned and Add
wrote the duplicate the next build refuses. The import aliases now seed
the closure -- per source, since an import applies only to the file that
writes it, so a typealias elsewhere naming the same word is not this one.
And inside a template expression an escaped identifier is a NAME:
everything between the backticks belongs to it, so a quote there does not
open a string and a brace does not close the expression. `${ `"` }` left
the template looking unterminated, and the rest of the file was blanked
-- a live declaration after it dropped as an orphan in the processor, and
every annotation after it disappeared in Settings.
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: 3269a927bc
ℹ️ 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 Kotlin typealias is top-level but it is not global, and the sweep over the project took every Kotlin file regardless of package. An alias declared in an unrelated package could then vouch for a same-named annotation the main class really does write itself, so the hint read as owned when nothing owns it -- which hides the editor and is indistinguishable from the tool being broken. A source counts now only where the main file can actually see it: the same package, or a package it imports from, by name or on demand. The package reader that decides it was already written inline inside declaresClass and is now shared, so the two cannot disagree about what a file's package is. 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: 0ea601ba83
ℹ️ 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".
Visibility is per symbol, not per package. `import com.other.Unrelated` exposes nothing else from com.other, and `import com.other.AppIos as Custom` exposes that one under Custom. The package-level answer was wrong in both directions: an unrelated import let an alias hide the editor for a hint nothing owns, and a renamed one lost its local name so a real annotation went unrecognised and Add wrote the duplicate. Each declaration now carries the name its own file uses, the name the main file sees it under, and the package it lives in. The chain follows the local name within that package -- which is the scope a top-level declaration resolves in, so a chain may span files of one package -- and only the visible end reaches the main file, under whatever name the import gives it. An intermediate link the main file cannot name no longer leaks out of its package. 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: 3ac9d2df5b
ℹ️ 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 link may be imported: package `a` declares `typealias Base = Ios`, package `b` imports `a.Base` and declares `typealias AppIos = Base`. Looking the target up only in the declaring file's own package stopped the chain there, so the hint read as unowned and Add wrote the duplicate declaration the next build refuses. Each target is now resolved through the file that writes it, most specific first: a qualified spelling names its package outright, a named import -- under its own name or an `as` name -- says where it comes from, and failing both it is the declaring package's own or comes from a package imported on demand. Worked out once per declaration rather than on every pass of the closure. 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: 4b8351d18a
ℹ️ 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".
On a top-level Kotlin declaration `private` means visible in that FILE, not in the package -- so another file's private alias is not a name the main source can write. Exposing it let it vouch for an unrelated annotation of the same name, so the hint read as owned when nothing owns it and the editor was hidden, which is indistinguishable from the tool being broken. The modifier is read backwards over the run that may precede the keyword and stops at anything that is not one, so a `private` belonging to whatever came before is not taken for this declaration's. `internal` is module-wide and so is not this file's alone; a private alias in the main file itself is in the file it belongs to and still counts. 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: 0952896dab
ℹ️ 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 continuation marker with nothing after it is still a marker: a file whose last byte is that backslash reads as an empty value to Properties.load, while leaving it in produced a key ending in `\` that matched nothing -- so the declaration stayed behind and the verification build failed on the duplicate the goal had just created. Stripped after the continuation loop as well as inside it. And `private /* note */ typealias AppIos = Ios` is legal, while the backward walk for the modifier skips only whitespace and stopped at the comment, reporting the declaration as public. It reads over a blanked copy now -- offsets and line breaks preserved -- since the forward scanner cannot help a backward walk. The alias survey moved out of the per-hint loop while doing that: which aliases exist, and what the main file calls them, does not depend on which hint is being asked about. 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: 8e3fbdafce
ℹ️ 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 languages resolve a simple name to a same-package type before an on-demand import, so a project that declares its own `Ios` and wildcard-imports ours writes its own. Reading that as ours hid the editor for a hint the processor never emits, which is indistinguishable from the tool being broken. A NAMED import still wins, since it is the more specific statement and a file may not both import a name and declare it. The type lookup is wider than the main-class one on purpose: an annotation is `annotation class` in Kotlin and `@interface` in Java, and any top-level type of that name shadows the import. The shadowing sources are this file and the rest of its package as already read. The sweep exists for typealiases and so is Kotlin-only, which leaves a same-named type in a file nobody opened reading as owned -- exactly as it did before this check existed, and recorded where the list is built rather than left to be discovered. 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: 54c6c7f167
ℹ️ 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 same-package rule that beats an on-demand import is a Java rule as much as a Kotlin one -- a `p/Ios.java` beside the main class is what `@Ios` means there, whatever the wildcard import says -- but the sweep that supplies the peers was passed only for a Kotlin main class, because it had been written for typealiases. So the shadow check saw the main file alone in every Java project, which is most of them. The sweep now reads `.java` as well as `.kt` and is passed for both, with the same bounds on files read and directories walked. A package declaration reads the same in either language, so a mixed project's peers are classified correctly whichever the main class is written in. 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: 59b78ee1f9
ℹ️ 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".
Two consequences of reading the project's other sources, both mine from the commit before. A peer is parsed with the language it is written in now, not the main file's. The two genuinely disagree -- a block comment nests in Kotlin and does not in Java, raw strings close differently, and Java translates unicode escapes before it tokenizes -- so a Java peer read by Kotlin's rules could land in the wrong package and shadow nothing. The language travels with the text rather than being inferred at the far end, and a Java peer is decoded as it is read. And a test tree takes no part in compiling the main class, so a same-package type there shadows nothing. Counting it made a production `@Ios` look like somebody else's, so the hint read as unowned and Add wrote the duplicate declaration the next build refuses. 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: 61a944b2ce
ℹ️ 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".
`target/generated-sources` is a compile root that Maven plugins add, so a declaration there is one the compiler sees -- and the blanket exclusion of `target` as an output directory skipped it. A same-package alias or type generated into it went unread, so the hint looked unowned and Add wrote the duplicate the next build refuses. Recognised by name rather than by reading the effective model, which is a large amount of machinery for one conventional directory. The three rules the walk applies -- output directories hold copies, unless `build` is a package name under a source tree, and a `src/test` tree takes no part in compiling the main class -- moved out of the loop into predicates that take strings. Each was a bug first and none of them was covered, because the walk needs a project directory and the Codename One FileSystemStorage; now only the listing loop itself is untested. 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: d3eef27818
ℹ️ 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".
…vate The compiler's source encoding is a project setting the orphan filter cannot see, and it decoded everything as UTF-8 -- so an ISO-8859-1 source declaring a non-ASCII package came back as replacement characters, never matched, and a live annotated class was dropped with its placement error lost. The file is now read as UTF-8 when it decodes as UTF-8 and as ISO-8859-1 when it does not, which reads a single-byte source correctly instead of mangling it. What neither reading can settle -- a non-ASCII name in some third encoding -- is inconclusive rather than an orphan, because everywhere else in this walk an unanswerable question keeps the class. And in Settings, a peer's `private annotation class Ios` is visible in that file only, so it shadows nothing; counting it made a real `@Ios` read as somebody else's, so the hint looked unowned and Add wrote the duplicate the next build refuses. Same rule the type aliases already had. The modifier walk learned the class modifiers while doing it -- it stopped at `annotation` in `private annotation class` and reported the declaration public. 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: 9eb6090ecf
ℹ️ 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".
Two Settings-side twins of fixes made elsewhere in this branch. The main-source walk still skipped `target` and `build` wholesale, so a main class generated into a compile root under one of them could not be found -- its ownership then read as unknown, and Settings offered an annotation-owned hint as editable. It shares the peer sweep's rules now, which is where those rules were made testable. And every source was decoded as UTF-8. The compiler's source encoding is a project setting this tool does not have, so a single-byte source came back as replacement characters: a non-ASCII package or class name never matched codename1.packageName, the real main source was rejected, and a hint an annotation owns read as editable. UTF-8 where the file decodes as UTF-8, ISO-8859-1 where it does not, which never fails. The UTF-8 check is hand-rolled for the same reason the name predicate and the hex reader are: CharsetDecoder is outside the API subset this class compiles against. It rejects a truncated sequence, an overlong one and the surrogate range, all of which decode to a replacement character rather than failing if left to String. 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: b4749f38ab
ℹ️ 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".
Continuation does not apply to a comment: `# note \` ends at the newline and the declaration below it is an ordinary property, which is what Properties.load reads. Joining the two made the pair read as a comment, so the migrated declaration was retained while the annotation was added beside it, and the verification build failed on the duplicate the goal had just created. Both comment markers, and a real continuation on a real declaration still continues. 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: 83681dc842
ℹ️ 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".
| String importLine = kotlin | ||
| ? "import com.codename1.annotations.buildhints.*" | ||
| : "import com.codename1.annotations.buildhints.*;"; |
There was a problem hiding this comment.
When the main source already imports an unrelated type with one of these simple names (for example, import com.example.Build;), or declares one in the same package, this wildcard import does not make the generated @Build(...) refer to Codename One's annotation because the explicit/same-package type takes precedence. The verification build therefore fails and rolls back an otherwise valid migration; emit fully qualified annotation names or account for conflicting imports instead.
Useful? React with 👍 / 👎.
1 participant
The problem
A build hint is a
codename1.arg.<name>=<value>line that reaches a builder asrequest.getArg(name, default). Nothing checked the name, so a misspelling was accepted, copied into the build request, never read, and silently discarded — a green build with the setting simply not applied.Our own agent reference had been shipping keys in exactly that state:
skill/references/build-hints.mdandroid.xPermissionsandroid.xpermissions(AndroidGradleBuilder.java:1206)android.minSdkVersionandroid.min_sdk_versionandroid.sdkVersionandroid.targetSDKVersionis the real one)The change
Most hints can now be written as annotations on the application's main class, where javac does the checking: a misspelled name is an unknown symbol, a wrong value type is a type error, and a value outside a hint's supported set is an unknown enum constant.
The builders are untouched.
BuildHintAnnotationProcessorconverts the annotations back into the same key/value pairs, andCN1BuildMojomerges them before the command-line overlay, the CN1Lib merges and both preflights — so a library still appends onto an annotation-supplied value and-Dstill wins.Simulatorpublishes them as system properties at startup socn1:runsees hints that no longer live in the properties file.The properties file is untouched too. It stays the way to set the long tail and the open-ended families such as
android.permission.<NAME>that an annotation cannot express, with no new warnings or errors on that path. Declaring one hint both ways is a build error.Scope: generated projects are deliberately not migrated here
Every project the archetype and the initializr produce is pinned to a released Codename One version — the initializr hardcodes
7.0.267inGeneratorModel.CN1_PLUGIN_VERSION— and no released core carriescom.codename1.annotations.buildhints. A generated project would import annotations that do not resolve and fail to compile before the user has written a line.So the archetype's
__mainName__.javaandcodenameone_settings.properties, and the initializr'scommon.zipand four source archives, are unchanged. They move to annotations in a follow-up once a release containing the package is out.scripts/skindesigner(7.0.255) stays on properties for the same reason, andcn1:migrate-build-hintsrefuses any project whose core lacks the package.The in-repo tool projects that build against 8.0-SNAPSHOT from source are migrated:
gamebuilder,video-builder,cn1playground,certificatewizard,guibuilder,fidelity-app,purchase-test-app,settings,hellocodenameone,protocol-e2e.docs/demosis deliberately excluded: it is the developer guide's snippet project, full of intentionally incomplete fragments, and running the annotation processors over it fails by design.One catalog, generated views
The hint set was described in five places that had drifted apart: a prose table in the developer guide, a runtime scraper of that table in the Settings tool that guessed each type by string-matching the description, a fifteen-entry schema in the simulator, a fourteen-entry separator map in the plugin, and a hand-written agent reference. Only 147 of ~520 names appeared in more than one.
maven/build-hint-catalogis now the single source of truth — 529 hints: 457 mined from the builders, 56 documented-but-unread (external), 16 dynamic families, 82 exposed as annotation attributes across 7 annotations and 10 enums. The annotations, the binding table the processor reads back, the developer guide's table and the simulator's editor schema are all generated from it.The guide's table goes from 208 rows to 529 with no prose lost, and gains Type, Default and Annotation columns it never had.
Enums are emitted only where the accepted set is demonstrable from the code that reads the hint —
HardeningPreflightrejects an unknownharden.level,IOSDependencyManagerthrows on an unknownios.dependencyManager, andGenerateDesktopAppWrapperMojosilently falls back tonativeon an unknowndesktop.titleBar, which is precisely the failure this removes.Settings tool
It no longer scrapes the guide's AsciiDoc and guesses types; it reads the catalog. It also validates closed value domains, and refuses to edit a hint an annotation already owns — reading
META-INF/codenameone/build-hints.propertiesand showing "Set by@Ios(themeMode)on the main class" — because writing a property for such a hint would create the duplicate declaration that fails the next build.Gates
scripts/check-build-hint-catalog.shfails when code reads a hint the catalog does not describe, and when our own docs or project templates name one that no builder reads. Its baseline is empty, so it is a hard gate rather than a ratchet.scripts/gen-build-hint-annotations.sh --checkfails on generated-file drift.Object/Annotationmethod-name rule, enum domains, separators matching whatLibraryHintMergerused to define).Both scripts run in the Java 8 leg of PR CI.
Verification
maven/integration-tests/build-hint-annotations-test.shgenerates a project from the archetype, adds the annotations by hand (the template no longer ships them), and checks the emitted wire values — a list joined with the hint's own separator, an enum written as the catalog's value rather than the constant name, and an attribute nobody set written not at all — that they reach the real build request, and that declaring one hint twice fails with a message naming both the hint and the annotation attribute.Also confirmed empirically, since the whole "unset means absent" design rests on it: javac records a member the developer wrote even when its value equals the annotation's default, and omits one left unwritten.
Notes for review
com.codename1.annotations.buildhints, not...annotations.build:.gitignorecarries a repo-wide**/build/*rule that silently swallowed every source under thecom.codename1.build.sharedpackage path. The rule now un-ignoresbuildwhen it is a Java package rather than a build output directory.android.permission.,harden., …) and are modelled as dynamic families rather than settable keys.Pre-existing problems surfaced but not fixed here
cn1:testbuild hints are broken today.RunTestsMojo.java:253sets the forked runner's CWD totarget/cn1-reports, where neither ofloadCodenameOneSettings()'s fallbacks looks, sobuildHint()returns null there for properties-file hints too.<skipTests>true</skipTests>), so one had rotted against aGridLayout(3, 2)that became(5, 2)in d15cfa5. The skip is now a property (default unchanged) and the assertion tracks the column count.GeneratedAccess_*files are stale against the current core; building it regenerates ~3,300 lines. Left alone to keep this change scoped.Follow-up
Once a release carrying
com.codename1.annotations.buildhintsis published:__mainName__.javaand settings template to annotations.common.zipand the four source archives, bumpGeneratorModel.CN1_PLUGIN_VERSION, and bindprocess-annotationsin the generated common POM.scripts/initializrandscripts/skindesignerthemselves.🤖 Generated with Claude Code