Fix class closure false positive on platform modules - #5481
Open
shai-almog wants to merge 3 commits into
Open
Conversation
The class closure check treated codenameone-core and java-runtime as server-provided only when they showed up in the platform module's own artifacts. They never do: they are `provided` scope in the app's common module and `provided` is not transitive, so an ios/android module that depends on common alone does not see them -- the same hole the local JavaScript path already documents. The framework is also stripped from the staged jar, so with an empty provided set every framework class referenced from a package the app shares with the framework -- a patched framework class, or a helper placed in a com.codename1 package to reach package private API -- was reported missing. A stock app plus one class in com/codename1/ui made `mvn clean package -Dcodename1.buildTarget=ios-source` fail with most of com.codename1.ui listed as missing from the build. Resolve codenameone-core / java-runtime from the plugin's own dependencies when the module's classpath does not carry them, and skip the check entirely when core cannot be resolved at all: without its class list the check cannot tell a stale class from a framework class. The decision about what is stripped from the staged jar now lives in one shared method used by both the jar assembly and the check, so the two can no longer disagree about what is actually missing. That also covers `provided` scope third party dependencies, which the check had not accounted for either. Verified against a generated app: the failing project builds, a genuine stale class (deleted source, surviving dependent class file) is still reported, and only it. 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: d4b4002013
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Codename One Maven plugin’s staging and class-closure verification logic to avoid false-positive “missing framework classes” errors on platform modules (e.g., ios, android) when framework artifacts are intentionally stripped from the staged jar and re-supplied by the build server.
Changes:
- Introduces shared strip-decision helpers (
isLocalJavascriptBuild,isStrippedFromStagedJar) and uses them in both jar assembly and the class-closure verifier to keep behavior consistent. - Enhances
verifyApplicationClassClosure()to resolve framework jars from the plugin’s own dependencies when they aren’t visible via the platform module’sproject.getArtifacts(). - Adds unit tests covering stripping behavior across build targets/scopes, including local JavaScript and Kotlin stdlib handling.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| maven/codenameone-maven-plugin/src/main/java/com/codename1/maven/CN1BuildMojo.java | Centralizes and reuses stripping rules between jar staging and class-closure verification; resolves server-supplied framework jars for platform modules. |
| maven/codenameone-maven-plugin/src/test/java/com/codename1/maven/CN1BuildMojoTest.java | Adds unit tests for the new shared stripping logic across representative build targets/scopes. |
Comments suppressed due to low confidence (1)
maven/codenameone-maven-plugin/src/main/java/com/codename1/maven/CN1BuildMojo.java:598
isLocalBuildTarget()callsbuildTarget.startsWith(...)without a null guard.isStrippedFromStagedJar(..., buildTarget)calls into this method and is a static helper that can be invoked with a null buildTarget, which would throw a NullPointerException.
private static boolean isLocalBuildTarget(String buildTarget) {
// windows-device (BUILD_TARGET_WINDOWS_NATIVE) is a *cloud* build: it sends
// a "win32" build to the server (see the windows-device target in
// buildxml-template.xml), mirroring linux-device. Only the explicit
// local-windows-device cross-compile and the windows-source project
// generation are local.
return (buildTarget.startsWith("local-") || BUILD_TARGET_XCODE_PROJECT.equals(buildTarget)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Collaborator
Author
|
Compared 181 screenshots: 181 matched. |
Collaborator
Author
|
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 144 screenshots: 144 matched. |
Collaborator
Author
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 217 screenshots: 217 matched. |
Collaborator
Author
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Review feedback on the closure check: Only codenameone-core, java-runtime and kotlin-stdlib are put back after being left out of the staged jar, so only those may satisfy a dangling reference. Reusing the assembly exclusion decision for that also excused references into an arbitrary `provided` or `runtime` scope dependency, which nobody supplies before the translators run -- an incomplete jar would have been uploaded instead of reported. The two decisions are now separate methods, with the narrow one feeding the check. An artifact with a null scope is no longer stripped from the staged jar: a missing scope is not a statement that the artifact sits outside the application, and dropping its classes can only break the build. Also add the Codename One header the copyright gate requires on the touched test file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
More review feedback: Skipping the check silently at debug level hid the fact that stale classes would go unreported, so say it at warn level with the build target and the coordinates that could not be resolved. Hold the provided jars in a LinkedHashSet rather than de-duplicating a list by scanning it, and take a Collection in the verifier so the set can be passed straight through. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment on lines
+226
to
+233
| static boolean isSuppliedByBuildServer(String groupId, String artifactId, String buildTarget) { | ||
| if (GROUP_ID.equals(groupId) && contains(artifactId, BUNDLE_ARTIFACT_ID_BLACKLIST)) { | ||
| return !isLocalJavascriptBuild(buildTarget); | ||
| } | ||
| return !isLocalBuildTarget(buildTarget) | ||
| && "org.jetbrains.kotlin".equals(groupId) | ||
| && "kotlin-stdlib".equals(artifactId); | ||
| } |
Comment on lines
+242
to
+258
| static boolean isStrippedFromStagedJar(String groupId, String artifactId, String scope, String buildTarget) { | ||
| if (GROUP_ID.equals(groupId) && contains(artifactId, BUNDLE_ARTIFACT_ID_BLACKLIST)) { | ||
| // For local JavaScript builds we need codenameone-core and java-runtime classes | ||
| // in the staged jar - the build server normally re-supplies those, but ParparVM's | ||
| // ByteCodeTranslator runs locally here and resolves everything from the staged class | ||
| // directory. | ||
| return !isLocalJavascriptBuild(buildTarget); | ||
| } | ||
| if (isSuppliedByBuildServer(groupId, artifactId, buildTarget)) { | ||
| // When sending to the build server, we'll strip the kotlin-stdlib and the server will | ||
| // provide it. For local builds, it's easier to just include it. | ||
| return true; | ||
| } | ||
| // An artifact with no scope was never scoped out of the application, so | ||
| // keep its classes rather than dropping them from the jar. | ||
| return scope != null && !"compile".equals(scope); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
mvn clean package -Dcodename1.buildTarget=ios-source(and the cloud iOS/Android targets) fails on a fully clean tree with most ofcom.codename1.uilisted as missing from the build, since #5390:Root cause
The check marks a referenced class missing when it is absent from the staged jar-with-dependencies and lives in a package the project owns. Two things collide on a platform module:
codenameone-core/java-runtimeare left out of the staged jar because the build re-supplies them, so no framework class is in the jar. This is true for local targets such asios-sourcetoo - only a local JavaScript build keeps them, because ParparVM then translates on this machine and resolves everything from the staged classes.verifyApplicationClassClosurebuilt its "this gets re-supplied" set fromproject.getArtifacts()of the module being built only. Those artifacts areprovidedscope in the app'scommonmodule andprovidedis not transitive, so anios/androidmodule that depends oncommonalone never sees them. The set was always empty there - the same hole the local JavaScript path already documents a few lines below.The trigger is any application class in a package the framework also owns: a patched framework class, or a helper deliberately placed in a
com.codename1package to reach package private API. That single class pulls the whole framework package into the project package space, and every framework class the app touches then looks missing.Reproduced on a stock archetype project: it builds clean, and adding one class under
common/src/main/java/com/codename1/ui/makes the iOS build fail withcom.codename1.ui.Button/Command/Component/Container/Dialog/Form/Toolbarreported missing.Fix
codenameone-core/java-runtimefrom the plugin's own dependencies when the module's classpath does not carry them.isStrippedFromStagedJargoverns what the assembly leaves out, and the narrowerisSuppliedByBuildServer(codenameone-core, java-runtime, kotlin-stdlib) is what may satisfy a dangling reference. Aprovidedscope third party dependency is left out but nobody puts it back, so references into it stay reportable.Verification
End to end against a generated app with the patched plugin:
com.codename1.ui: BUILD SUCCESS (was the reported failure)..class, leave the stale dependent class file): still fails, reporting only the deleted class.Note that the check has been inert on iOS/Android modules since it landed (its provided set was always empty there); this is what makes it work on them.
🤖 Generated with Claude Code