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 23 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>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b505657e8
ℹ️ 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 217 screenshots: 217 matched. |
Kotlin string interpolation. `quote()` escaped nothing for `$`, so migrating a
Kotlin main class turned any hint value containing one into an interpolated
string -- an `android.gradleDep` of `implementation 'x:y:$version'` either fails
to compile as an unresolved reference or silently resolves to something else.
The target language is threaded into the quoting and `\$` is emitted for Kotlin
only, since Java has no such construct.
Imports in a default-package source. With no package declaration and no existing
import, `head.indexOf("package ")` returned -1 and the arithmetic put the import
at the first newline in the file -- inside the copyright comment. The project
was then left with unresolved annotations and its properties entries already
deleted. The class declaration is the anchor in that case.
Aliases in the Settings tool. Ownership was looked up by exact name, so with
`@Android(themeMode = ...)` owning `and.themeMode`, the row for its deprecated
alias `cn1.androidTheme` still offered Add -- creating the second declaration of
one effective setting that the next build refuses through the alias conflict
check. Both sides of the lookup are canonicalised now.
Credential masking. The scraper this catalog replaced inferred SECRET from names
containing password, secret or token, and the Settings field masks on that type.
Classifying them as STRING rendered a stored certificate password as visible
text. All five -- codename1.mac.certificatePassword, macNative.notarize.password,
windows.msix.password, windows.signing.password and facebook.clientToken -- are
SECRET again, with a test that holds every future credential-shaped name to it.
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: b2e0e86f14
ℹ️ 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".
…ain class The guard accepted an execution anywhere in the reactor, but ProcessAnnotationsMojo scans only the output directory of the module it is bound to. A binding on a platform or utility module therefore never sees the main class that common compiles, and the migration would still delete the working properties and leave annotations nothing ever reads -- the exact failure the guard was added to prevent, one level in. It now resolves the module whose base directory is the Codename One project directory, which is where the main class lives and where findMainClassSource looks, and requires the binding there. An execution bound to phase `none` is declared but never runs, so it no longer counts either. The refusal names that module and says explicitly that binding the goal elsewhere in the reactor does not help, since that is the mistake being made. Verified against a real project: gamebuilder passes with the binding on common, and is refused when it is moved to the javase module. 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: fd05dfa07e
ℹ️ 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".
Being declared in a real phase was still not enough. ProcessAnnotationsMojo returns immediately when `skip` is set, and again when its output directory does not exist -- which is every phase before `compile`. An execution configured `<skip>true</skip>`, or bound to `generate-sources`, therefore emits no annotation resource at all, and the migration would delete the working properties and leave nothing behind. The guard now requires the execution to be unskipped and bound at or after `compile`, taking an absent phase as the goal's own default of `process-classes`. Skip is read from both the execution and the plugin configuration. Verified end to end: gamebuilder proceeds normally, and is refused once its execution carries <skip>true</skip>. 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: 3037e76565
ℹ️ 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 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
|
Compared 144 screenshots: 144 matched. |
The group name is part of the property key, so registering harden.level under both `hardening` and `Hardening` overwrites nothing -- it creates a second group, and BuildHintEditor renders every group it finds. The user saw duplicate controls for one setting, for all of harden.*, nativeTheme, ios.themeMode and and.themeMode. The comment claiming the hand-written entries take precedence because the setter never overwrites was simply wrong: the two never collided on a key. BuildHintSchemaDefaults now records the hints it describes as it registers them, and the generated companion skips those. Precedence is explicit rather than assumed, and it cannot drift, since the record is built from the same set() calls that do the describing. Verified by walking the registered properties: 89 hints in the editor, none appearing under more than one group, and harden.level, nativeTheme, ios.themeMode and and.themeMode all resolving to their hand-written group. Also makes the integration test fail when the merged settings file is absent. It was the only assertion that annotation hints reach the build request, and skipping it on a missing file meant a regression in goal ordering, target validation or the merge itself would have left the test green. 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: af8ff86d77
ℹ️ 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".
Four rounds of review went into guessing, from the POM, whether process-annotations would run: it can be bound on the wrong module, bound to a phase with no compiled classes, skipped outright, or skipped through a property expression the model text does not resolve. Each fix closed one case and the next review found another, which is what a static prediction of another mojo's behaviour is going to keep doing. The goal now applies the whole migration, runs process-classes over the module that holds the main class, and checks that every migrated hint came back out of the emitted resource. If any did not, both files are put back exactly as they were and the failure says what was missing. Whatever the next way to not-run turns out to be, the answer is still correct. Both files have to move together before that check: leaving the properties in place while the annotations are added is itself the duplicate-declaration case, so the build would fail for that reason and never say whether processing works. The first version of this change had that wrong, and the verification caught it. Verified on a generated project: 7 hints in, 6 migrated and confirmed emitted, java.version correctly kept. With the binding removed the goal refuses and both files come back byte-identical. The Settings tool has the same problem from the other side. It read ownership only from the emitted resource, so in the window right after a migration -- the source declares the annotations, no build has run -- every hint looked unowned and Add was offered for one the annotations already set. It now falls back to reading the annotations off the main class, matching attribute names at the top level of each annotation so a comma, bracket or equals sign inside a value cannot register as one. 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: 586e6bc3fb
ℹ️ 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 rollback snapshot was taken with the UTF-8 read helper and restored with the ISO-8859-1 writer, so any raw high byte in an unrelated property -- an accented codename1.displayName, say -- came back changed while the goal reported that both files were put back exactly as they were. It is snapshotted with the properties encoding now, and the two helpers are explicit about which encoding they use rather than one of them being the default. Verified on a generated project carrying a raw 0xE9: after a failed migration the settings file is byte-identical and the byte is still there. The Settings tool consulted the main-class source only when the emitted manifest was missing. The manifest is a build artifact and goes stale in both directions -- absent right after a migration, and out of date the moment an attribute is added to a project that was built earlier -- so a newly annotated hint looked unowned and Add wrote the duplicate declaration the next build refuses. The source is read every time now, because it is the only current statement of what the annotations declare, and the manifest is merged on top for its origins. The union is the safe direction: over-reporting ownership only withholds an editor, while under-reporting breaks the build. 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: b1dff60641
ℹ️ 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".
…nner The main class was read and written as UTF-8, so a project whose sources use a different encoding had its whole file reinterpreted while the annotations were spliced in: a raw byte in a comment or a string literal came back changed even when the migration succeeded. Reading project.build.sourceEncoding would only narrow that to projects which declare it correctly. Instead both ends use ISO-8859-1, which maps every byte 0-255 to the same char, so decode -> splice ASCII -> encode reproduces the original bytes exactly whatever the real encoding is. The markers this code looks for -- package, import, the class declaration -- are ASCII, and every ASCII-compatible encoding decodes those identically under that scheme. Verified on a generated project whose main class carries three raw 0xE9 bytes in a comment, making it invalid UTF-8: all three survive the migration and the annotations are still inserted correctly. The Settings tool's source scanner skipped strings but not comments, so a comment carrying an unmatched delimiter -- @iOS(/* required for issue ( */ teamId = "x") -- lost the annotation's boundary and left teamId editable, which is the case that writes the duplicate declaration. It now skips line comments, block comments and character literals as well, through one shared helper used by both the balancer and the attribute scan. 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: 76cd1dcb5f
ℹ️ 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 P1 is about the feature, not the migration convenience: a mojo's default phase does not add an execution to a project, so an existing application that follows the package documentation and adopts the annotations compiles cleanly and ships with every annotated hint missing. Nothing said so. CN1BuildMojo now checks, when no annotation manifest was found, whether the application classes carry build hint annotations at all -- read out of the class file's annotation table, so it sees what the compiler emitted rather than what the source appears to say. If they do, the build fails with the execution to add. Both a directory and a jar are scanned, because a reactor `package` build hands the dependency module's jar rather than its output directory, which is exactly the shape this has to work in. The package documentation now states the requirement too. Verified on a generated project: with the binding removed and one @iOS on the main class the build refuses; with the binding restored it applies the hint and carries on. Two more from the same review: - Verification accepted a manifest an earlier build had left behind, so with processing now skipped or unbound the check passed against a stale file, the properties were deleted, and the next clean build dropped the hints. The resource is removed before the nested build, so what is checked is what that invocation produced. - The Settings source scan matched only the imported simple name, missing the equally valid `@com.codename1.annotations.buildhints.Ios(...)`. Both spellings are matched now, with the name boundary checked so `@Ios` cannot match `@IosPrivacy`. The boundary test is hand-rolled because Character.isJavaIdentifierPart is outside the API subset this class compiles against -- the bytecode compliance gate caught that. 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: 8be6d5f661
ℹ️ 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 from the same review. @iOS() with every member left at its default is legal Java -- it is what is left after the last attribute is deleted -- and the processor emits the manifest for it, stamped with the main class but carrying no hint. The merge judged by the hint count, read that as "the processor never ran", and refused the build until the annotation itself was deleted. The manifest's presence is what proves processing happened, so that is what the check now reads; the refusal still fires for the case it exists for, annotations in the compiled classes with no manifest anywhere. The migration goal restored both files when the verification build failed, but not when the mutation itself did. If the annotations went in and the properties rewrite then failed -- unwritable file, full disk, a partial write -- the project was left declaring the same hint twice, which is exactly the state the next build refuses to compile: worse than not having migrated at all. The restore is one helper now and runs for either failure. A dangling javadoc left over from a removed method went with it. pr.yml ignores scripts/** and re-includes a fixed list, so a PR touching only the catalog gate, its miner, or its baseline started no workflow at all -- the gate could be broken, or its empty baseline relaxed, without ever running. The five files are re-included in both the pull_request and push filters. The merge test needed the annotated class present alongside the empty manifest to trip the refusal at all; without it the test passed against the bug it was written for. 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: fb5a6c7169
ℹ️ 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 from the same review, all cases where something looked applied and was not. The main-class stamp says which class produced the manifest, not when. Nothing clears target/classes between builds, so a project that ran process-annotations once and then stopped -- goal unbound, skipped, moved to a phase that no longer runs -- keeps a manifest naming the right class while the annotations beside it change. The merge accepted it, applied the older values, and the guard added for exactly this never fired. The processor now records a fingerprint of the annotations it read, taken over the raw members rather than the hints they convert into so it moves for anything the developer can change: a different value, an added or removed attribute, a whole annotation gained or lost. The merge recomputes it from the main class on the classpath -- directory or jar -- and refuses a manifest that does not match, naming it as left over from an earlier build. It refuses only on positive evidence: no main class name, no class file, no recorded fingerprint, or an unreadable one, and the manifest is taken at face value as before. A hint set by @hardening reached the settings only in createAntProject, which runs after the early hardening pre-flight and after hardeningCacheKey is read for the Android up-to-date check. The early pass computed "unhardened" from the properties file while the finished build recorded "hardened:...", so the keys never matched and an up-to-date APK was rebuilt on every invocation. Worse, an unsupported hardening request made through an annotation escaped the refusal that pass exists to perform. Annotation hints are merged there too, before the -D overlay so a command-line hint still wins. Properties.load turns € in the settings file into a real euro sign, and migrate-build-hints writes the source back through ISO-8859-1 to keep the untouched part byte-identical -- so emitting the character raw wrote '?' for anything unmappable and a high byte for anything else, corrupting a UTF-8 source. The verification build would not have noticed: it checks that the hint came back, not what its value was. Non-ASCII is written as \uXXXX, which Java and Kotlin both accept, and a backslash before one still survives -- Java recognises a unicode escape only after an even number of backslashes, so there is a test pinning that rather than leaving it to luck. 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: 51fb76af15
ℹ️ 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 fingerprint added last round covers the annotations and nothing else, so editing codenameone_settings.properties cannot invalidate it. With processing skipped or unbound, a line added for a hint an annotation already sets left a manifest that still matched -- and the merge quietly replaced the value the developer had just written. The next clean build regenerated the manifest, the processor saw both declarations, and the build failed. Same source, two different outcomes, decided by whether target/classes happened to be cleaned. The merge now refuses a hint the properties file also declares instead of overlaying it, with the message the processor would have given. Aliases count as the same setting, so and.captureRecord in the file still collides with @android(captureRecord). This is a safety net rather than the primary check: when the processor runs it has already failed for the same reason and can point at the offending line. It only matters in the builds the processor never saw, which are exactly the ones that were silently 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: 9c5a58622d
ℹ️ 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".
| if (System.getProperty(key) == null) { | ||
| System.setProperty(key, p.getProperty(key)); |
There was a problem hiding this comment.
When process-annotations becomes skipped or unbound after a successful build and an annotation is then changed, the old manifest remains in target/classes, and this loop publishes its stale values without checking cn1.buildHints.sourceDigest against the compiled main class. The native-build path now rejects exactly this stale-manifest case, but the simulator silently runs with the previous values (notably for simulator-visible hints such as desktop.titleBar or nativeTheme); perform the same digest validation here before setting system properties.
Useful? React with 👍 / 👎.
| if (c == '\\' && i + 1 < logicalLine.length()) { | ||
| key.append(logicalLine.charAt(++i)); | ||
| continue; |
There was a problem hiding this comment.
When a valid properties key contains a Unicode escape, such as codename1.arg.\u0069os.teamId=ABCDE, Properties.load() decodes it to codename1.arg.ios.teamId and schedules it for migration, but this parser merely removes the backslash and produces codename1.arg.u0069os.teamId. The original declaration is therefore not removed, the verification build reports a duplicate, and the migration rolls back instead of handling a properties form it claims to support; decode \uXXXX sequences using Properties semantics while parsing keys.
Useful? React with 👍 / 👎.
Merged master, which brought in #5583 (complications on the watch, and a Wear artifact beside the phone APK). It adds ten hints the builders read, and the catalog gate failed on the merge result: every hint the code reads has to be described, and the empty baseline means there is nowhere to park one. That is the gate working, not a conflict. #5583 was written before the catalog existed, so it had nothing to add its hints to. Each row's type and default come from the call site rather than from the name: android.blockLabel boolean, false android.surfaces.complicationUpdateSeconds int, 0 android.watchModule boolean, true android.watchVersionCode int, no default -- unset means derive from the offset below android.watchVersionCodeOffset int, 100000000 android.wear.complicationsVersion string, 1.2.1 android.wear.tilesVersion string, 1.4.1 android.wear.protoLayoutVersion string, 1.2.1 android.wear.guavaVersion string, 31.1-android watchNative.surfaces.deploymentTarget string, 10.0 Catalogued, not annotated: the catalog has to describe every hint, but exposing one as a typed attribute is a curation decision, and inventing API for somebody else's feature in a merge commit is not that. They are documented, typed and value-checked, and can be annotated later without churn. The one nuance worth recording is watchNative.surfaces.deploymentTarget, whose default is the watch app's floor rather than the extension's: WidgetKit reaches back to watchOS 9, but the extension is embedded in the watch app, so the lower number would advertise support that does not exist. The regenerated developer-guide table is the only other change -- no annotation churn, as intended. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The developer guide gate treats Vale warnings as errors, and the ten rows added in the previous commit brought seven alerts with them -- the guide requires contractions, so "is not", "cannot", "does not" and "it is" all fail, and "silently" is on the adverb list. Reworded in the catalog, which is where the prose lives; the table is generated from it. The meaning is unchanged in every case, including the two that needed more than a contraction: "a value other than a whole number" rather than "that is not", and "refuses to install on ... support the user never gets" rather than "cannot install on ... does not exist". Vale is clean across all 116 files of the guide. 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: eb58dac840
ℹ️ 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".
| <dependency> | ||
| <groupId>${project.groupId}</groupId> | ||
| <artifactId>codenameone-build-hint-catalog</artifactId> | ||
| <version>${project.version}</version> |
There was a problem hiding this comment.
Adding this runtime dependency requires adding codenameone-build-hint-catalog to both artifact checks in .github/workflows/release-on-maven-central.yml. When a Central deploy reports failure or an R2 copy is truncated, the current recovery/confirmation loops verify the plugin and the older platform catalog but not this artifact, so the final gate can accept and advertise a release whose Maven plugin fails dependency resolution.
Useful? React with 👍 / 👎.
| String[] markers = { | ||
| "@" + simple, | ||
| "@com.codename1.annotations.buildhints." + simple, | ||
| }; |
There was a problem hiding this comment.
When a Kotlin main class uses a valid aliased import such as import com.codename1.annotations.buildhints.Ios as BuildIos followed by @BuildIos(teamId = "..."), neither of these two markers matches it. If the generated manifest is absent or stale when Settings opens, the annotated hint is consequently offered for Add, which writes a duplicate properties declaration and makes the next process-annotations run fail; resolve Kotlin import aliases while deriving source ownership.
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