Compile every cn1lib's native sources, and fix what that found - #5595
Compile every cn1lib's native sources, and fix what that found#5595shai-almog wants to merge 12 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4658694449
ℹ️ 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".
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 775f74e2d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d10f958f0
ℹ️ 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 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
|
Compared 181 screenshots: 181 matched. |
A cn1lib ships its native sources: the packager stores them as resources and the customer's Xcode or Gradle is the first compiler that ever sees them. Two libraries were broken in exactly that gap. WHAT WAS BROKEN cn1-admob, iOS. The library pulled Google-Mobile-Ads-SDK with no version, so every app resolves the newest pod, and the sources reference two identifiers that are not in it: GADSimulatorID, which SDK 12 removed because the SDK counts every simulator as a test device on its own, and BRIDGE_RETAINED, which has never existed anywhere in the port -- the macro is BRIDGE_CAST, from a header this file does not import. Reported twice through support; the earlier fix (PR #5570) was for the Android compile break and did not touch this. The same initialize() dropped tagForChildDirected, tagForUnderAge and maxAdContentRating on the floor. The interface documents them as mirroring AdConfig and Android honours them, so an app asking for child-directed treatment silently did not get it on iOS. cn1-admob, Android. createBanner returned PeerComponent.create(adView), but a NativeInterface method declared to return PeerComponent is special-cased by the builder, which emits PeerComponent.create(impl.createBanner(...)) around it. Wrapped twice, AndroidImplementation.createNativePeer rejects its own AndroidPeer with IllegalArgumentException the first time a banner is shown. The shipping GoogleMaps cn1lib returns android.view.View, which is the contract. cn1-applovin carried both defects verbatim, plus an initialize() calling ALSdk.mediationProvider and initializeSdkWithCompletionHandler:, both removed in MAX 13. cn1-unity-levelplay carried both defects and had never compiled on either platform. Its Objective-C used ISInterstitialDelegate, ISRewardedVideoDelegate and ISBannerDelegate, which are in neither the 8.x nor the 9.x SDK, and its Java implemented LevelPlayInterstitialListener without onAdShowSucceeded. Both sides are rewritten against the unified LevelPlay API (LPMInterstitialAd / LPMRewardedAd / LPMBannerAdView, com.unity3d.mediation.*), pinned to the same SDK generation on both platforms. That also retires a design flaw: the old singleton entry points forced every callback through a "currently active handle" field, which lost events whenever two ads of one format were in flight. The three iOS pods are now pinned. An unpinned pod is what turned a Google rename into a broken build for every app using the library. WHAT NOW CATCHES IT ad-cn1lib-ios-native-check.yml compiles the ad libraries' Objective-C against the pod each one pins, in a throwaway static-library target -- a static library archives .o files without linking, so the externs the real build supplies do not have to resolve. It reads the pod out of codenameone_library_required.properties rather than repeating it, so the version the check compiles against is the version apps build against, by construction. The translator's cn1_globals.h is supplied as a prefix header, which is what the generated <app>-Prefix.pch does in a real build and what makes these files compilable at all outside a device build. check-cn1lib-android-api.py fetches the artifacts each library's android.gradleDep names and compiles its Android sources against exactly those, for every cn1lib rather than the one that has a hand-written Maven check. It runs on the JDK the Android build uses, because LevelPlay 9.6 ships class files javac 8 refuses to read. cn1-admob's existing Maven-driven check keeps working; its port stubs move to scripts/cn1lib-api-check/stubs so both mechanisms share one definition instead of duplicating it. check-cn1lib-native-sources.py covers the two platforms that need no SDK: the C glue the native win32/Linux ports compile into the app, and the JavaScript port implementations, which have no build step at all. That needs a C compiler, so gcc joins the CI container. check-cn1lib-native-coverage.py is the gate that keeps this from decaying. A workflow only covers the libraries its matrix names, so a new cn1lib is uncovered by default and nothing says so -- the state cn1-admob and cn1-unity-levelplay were in. It fails when a library ships Objective-C that no matrix names, and when a library pulls a CocoaPod without a version. check-native-peer-returns.py catches the double-wrap statically, on both the Java and the Objective-C side, since it is valid code that only fails on a device. VERIFICATION Every finding was reproduced against the real SDK before it was fixed, and every new gate was run against a deliberately broken input as well as a passing one. All five libraries that ship Objective-C (three ad, two AI) go through the workflow's own recipe -- xcodegen, pod install, xcodebuild -- and build; all five Android implementations compile against their pinned artifacts; all six cn1lib reactors package, and the emitted .cn1lib archives carry the new sources and pins. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> Signed-off-by: Shai Almog <67850168+shai-almog@users.noreply.github.com>
Review found the probes compiling with ARC on, which is not what a Codename One app does. The generated app target is manual-retain-release -- CLANG_ENABLE_OBJC_ARC = NO in both configurations of vm/ByteCodeTranslator/src/template/template.xcodeproj -- and IPhoneBuilder turns ARC on per file rather than per target, for exactly three of them (arcPhaseFixScript: CN1Vision.m, CN1Language.m, CN1Inference.m). A cn1lib's sources are not in that list, so they compile under MRR today. An ARC-only gate is therefore weaker than it looks: __bridge is an error without ARC, and a missing release is invisible with it, so either model alone accepts code the other rejects. Both native-check workflows now compile each library under both, which is one extra cheap job per library and makes the answer independent of a per-file flag we do not control from here. Checked before changing anything: all five libraries that ship Objective-C compile clean under -fno-objc-arc as well as -fobjc-arc, so this widens the gate without papering over a finding. The BRIDGE_CAST definitions added earlier are what make that true -- they resolve to __bridge only under __has_feature(objc_arc) and to nothing otherwise, which is the pattern AppleMapProvider.m already used. The matrix value reaches the project as a literal YES or NO. An earlier draft passed it as an xcodebuild setting and referenced $(CN1_PROBE_ARC) from project.yml, which silently degrades to NO if the variable ever goes missing -- a gate that stops covering half of what it claims without saying so. Also adds vm/ByteCodeTranslator/src/cn1_globals.h to the ad workflow's push paths. It was in the pull_request paths only, so a direct push to master touching the header the probe copies would not have run the check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two holes in the coverage this PR adds, both found in review. pr.yml excludes scripts/ wholesale and re-includes named files, so a pull request touching only one of the four new check scripts started no CI at all -- a change that breaks a gate could merge without that gate ever running, which is the failure mode this PR exists to remove. All four are re-included now, and so is scripts/cn1lib-api-check/stubs, since those stubs are the API surface the Android check compiles every library against. The iOS probes staged *.m only, while check-cn1lib-native-coverage.py counts a .mm file as an iOS native source. A library with an Objective-C++ bridge would therefore be reported as covered while nothing compiled it. Both probes stage .mm as well. Staging is now guarded: if a library ends up with no Objective-C at all, the probe builds an empty target and reports success, which is a green light for an unchecked library. The guard counts with find rather than ls, because ls reports failure when either glob is unmatched -- the normal case for a library with no .mm, which would have failed every job here. Exercised both ways locally under set -euxo pipefail before committing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
e784bd0 to
2fee68a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2fee68a08c
ℹ️ 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".
check-native-peer-returns.py read .m files only, while check-cn1lib-native-coverage.py counts a .mm file as an iOS native source, so a peer method implemented in Objective-C++ had no gate at all. Nothing else covers it. A return type that conflicts with the void* the builder generates is a clang warning rather than an error, so the Xcode probe compiles such a bridge and reports success; this check is where the contract is actually enforced. Verified by pointing the check at an Objective-C++ copy of the AdMob bridge returning id instead of void*: reported before the change, clean after. 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: f62e61bded
ℹ️ 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".
check-cn1lib-native-sources.py compiled maven/cn1-*/win/src/main/c as host C, which parses the #else half of every _WIN32 guard and reports the file as compiling. Everything actually specific to the platform -- the windows.h include, LoadLibraryA, GetProcAddress -- went unread, so a break there passed the gate and reached the customer's Windows build intact. Demonstrated rather than assumed: with GetProcAddress misspelled inside the _WIN32 branch, the host compiler exits 0 and the gate reported "compiles". A mingw-w64 cross compiler rejects it, which is the behaviour now. That target also needs cn1_win_compat.h staged beside cn1_globals.h -- the Windows branch of cn1_globals.h includes it, where the other branch takes pthread.h -- so the include directory the check builds now carries both port headers rather than one. The cross compiler joins the CI container. Its absence is a skip, and skips fail under --require-all, so this cannot quietly go back to checking half a file. The port is built with clang-cl rather than mingw, so what this gates is the API existing and the syntax parsing, not that ABI; the script says so. 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: 50ce89f0fc
ℹ️ 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".
…g formats the bridge rejects Two findings from review, both on the ad bridges. OWNERSHIP. These sources compile manual-retain-release, so every alloc here left an owning reference on top of the dictionary's or the property's retain. disposeFullScreen: and disposeBanner: therefore freed nothing: the holder, its SDK ad and its delegate all survived, and a session that creates and disposes ads repeatedly accumulated every one of them -- GADBannerView and MAAdView among them, which are not small. Ownership is handed to the container through CN1_HANDOVER, which autoreleases under MRR and expands to nothing under ARC, where the extra reference does not exist and autorelease will not compile. The holder classes gain a dealloc that clears their retained properties, since MRR releases none of them on its own, guarded the same way. Verified with clang's static analyzer under -fno-objc-arc, which is built for exactly the over-release this kind of edit risks: clean on all three, and it caught one more leak while it was there -- an LPMInitRequestBuilder that the chained initialize call never released. The two AI bridges were analyzed too and are clean, so nothing else needed the same treatment. CAPABILITY. LevelPlayProvider.isFormatSupported answered "anything but NATIVE", inherited by copy from AdMob, where it happens to be true. LevelPlay has neither a rewarded-interstitial nor an app-open format and AppLovin MAX has no rewarded-interstitial, and all four bridges already refuse them, so callers were told a format was supported and then handed null. Each predicate now describes its own SDK. AdMob's is unchanged, being the one that was accurate. 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: 43b5d5fa85
ℹ️ 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".
covered_libraries() counted a library as covered because some workflow's YAML contained "lib: cn1-something". Any job that loops over libraries would have satisfied it -- a packaging matrix, a release matrix -- while nothing compiled a line of Objective-C. The guard against a new cn1lib shipping unchecked was itself checkable by accident. A workflow now qualifies only if it stages a cn1lib's Objective-C and runs xcodebuild over it, which is what separates a native check from any other loop over libraries, and a library qualifies only if that workflow also triggers on maven/<lib>/**. Naming a library in the matrix of a workflow that never fires for it means the check runs when something else changes, which is not coverage of that library. Those are two different mistakes and they get two different findings: one says to add the library to a native-check workflow, the other names the workflow it is already in and says its trigger paths are missing the library. A gate that reports the wrong cause costs more time than it saves. Both failure modes were exercised: a fake packaging workflow matrixing over the three ad libraries no longer covers them, and deleting one path entry from the real workflow's filters reports that library with the trigger message. 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: b651453c0f
ℹ️ 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 evidence check was per file, so a workflow holding a real native-check job alongside any other job that loops over libraries handed coverage to libraries that job never touched. The markers came from one job and the matrix from another, and the gate could not tell. Matrices are now read only from jobs that stage a cn1lib's Objective-C and run xcodebuild, with jobs separated by indentation. A YAML parser would be tidier, but PyYAML is not in the standard library and this runs in the CI container as it is, so the splitter is deliberately small: the lines under jobs: that sit at one indent level, each body running to the next. Exercised with a workflow shaped like the failure -- the native job matrixing two libraries and a packaging job below it matrixing the third, with the third still named in the trigger paths so only job scope could reject it. It is reported as uncovered, and was reported as covered before this change. 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: 569d022c48
ℹ️ 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".
…and read trigger paths per trigger Two findings from review. USE AFTER FREE. disposeBanner: removed the dictionary entry and then dispatched a block referring to the holder. Under manual retain/release the dictionary is that holder's only owner, so the removal deallocated it, and the block was copied afterwards -- capturing, and then retaining and dereferencing, an object that was already gone. Disposing a LevelPlay banner could crash. The view is read out before the dispatch now, so the block captures the object it actually needs while the dictionary still holds it, and the entry is removed straight after rather than inside the block: doing it inside would let a banner recreated on the same handle be removed by a disposal still in flight. This came in with the LevelPlay rewrite in this branch; the AdMob and AppLovin disposals dispatch nothing and were never exposed to it. Worth recording that the static analyzer does not see this one. It caught the ownership imbalance a few commits back, but it does not model dictionary ownership across dispatch_async, so this needed review to find. TRIGGER PATHS. The coverage guard tested for maven/<lib>/** anywhere in the workflow text, so a library listed under push but not pull_request read as covered while every pull request skipped its check -- the break would surface only after merging. The on: block and each trigger inside it are read separately now, and both have to list the library. Exercised by deleting the entry from one trigger at a time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
|
Compared 217 screenshots: 217 matched. |
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
|
Compared 144 screenshots: 144 matched. |
The ad check runs six macOS jobs and the AI check four, and neither declared a concurrency group. A macOS runner is scarce and a queued run holds its slots until it finishes, so pushing a branch several times does not replace the run in flight -- it lines the obsolete ones up in front of the current one. Five superseded runs of the ad workflow, thirty macOS jobs between them, were queued on this branch at once, and the run that mattered was behind all of them. Both now use the same group pr.yml does: keyed on head_ref for a pull request so a new push cancels the previous run, and falling back to run_id on push to master, where head_ref is empty, so every master commit is still checked in full rather than cancelled by the next 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: d0eba46157
ℹ️ 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".
…branch name head_ref is the source branch name with no fork identity in it, so two pull requests opened from different forks that both use a common branch name -- master, patch-1, and similar -- landed in the same concurrency group and cancelled each other's native check. The library that got cancelled would then show no result at all rather than a failure, which is the quiet kind of gap this branch is otherwise about closing. The pull request number is unique across forks, and on a push to master there is no pull request, so the group still falls back to the unique run_id and every master commit is checked in full. Noted for separately: pr.yml's concurrency group has the same head_ref collision. It is long-standing, unrelated to these workflows and affects the whole matrix, so it is not changed here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A cn1lib ships its native sources: the packager stores them as resources and the customer's Xcode or Gradle is the first compiler that ever sees them. Two libraries were broken in exactly that gap, and one of them reached support twice.
What was broken
cn1-admob, iOS. The library pulled
Google-Mobile-Ads-SDKwith no version, so every app resolves the newest pod, and the sources reference two identifiers that are not in it:GADSimulatorID— removed in SDK 12; the SDK counts every simulator as a test device on its own now, so the flag needs no entry at all.BRIDGE_RETAINED— has never existed anywhere in the port. The macro isBRIDGE_CAST, from a header this file does not import, and a retained cast would have been wrong regardless:NativeIPhoneViewretains the peer itself and releases it on collection.The same
initialize()also droppedtagForChildDirected,tagForUnderAgeandmaxAdContentRating. The interface documents them as mirroringAdConfigand Android honours them, so an app asking for child-directed treatment silently did not get it on iOS.cn1-admob, Android.
createBannerreturnedPeerComponent.create(adView), but aNativeInterfacemethod declared to returnPeerComponentis special-cased by the builder, which emitsPeerComponent.create(impl.createBanner(...))around it. Wrapped twice,AndroidImplementation.createNativePeerrejects its ownAndroidPeer:The shipping GoogleMaps cn1lib returns
android.view.View, which is the contract.cn1-applovin carried both defects verbatim, plus an
initialize()callingALSdk.mediationProviderandinitializeSdkWithCompletionHandler:, both removed in MAX 13.cn1-unity-levelplay carried both defects and had never compiled on either platform. Its Objective-C used
ISInterstitialDelegate/ISRewardedVideoDelegate/ISBannerDelegate, which are in neither the 8.x nor the 9.x SDK; its Java implementedLevelPlayInterstitialListenerwithoutonAdShowSucceeded. Both sides are rewritten against the unified LevelPlay API (LPMInterstitialAd/LPMRewardedAd/LPMBannerAdView,com.unity3d.mediation.*), pinned to the same SDK generation on both platforms. That also retires a design flaw: the old singleton entry points forced every callback through a "currently active handle" field, which lost events whenever two ads of one format were in flight. One ad object per handle removes it.All three iOS pods are now pinned. An unpinned pod is what turned a Google rename into a broken build for every app using the library.
What now catches it
ad-cn1lib-ios-native-check.ymlcodenameone_library_required.propertiesrather than repeating it, so the version the check compiles against is the version apps build against, by construction. The translator'scn1_globals.his supplied as a prefix header — what the generated<app>-Prefix.pchdoes in a real build, and what makes these files compilable outside a device build at all.scripts/check-cn1lib-android-api.pyandroid.gradleDepnames and compiles its Android sources against exactly those, for every cn1lib rather than the one with a hand-written Maven check. Runs on the JDK the Android build uses, because LevelPlay 9.6 ships class files javac 8 refuses to read.scripts/check-cn1lib-native-sources.pyscripts/check-cn1lib-native-coverage.pyscripts/check-native-peer-returns.pycn1-admob's existing Maven-driven API check keeps working; its port stubs move to
scripts/cn1lib-api-check/stubsso both mechanisms share one definition instead of duplicating it.gccjoins the CI container for the C check, with an apt step covering the window before the image rebuild propagates.Verification
Every finding was reproduced against the real SDK before it was fixed, and every new gate was run against a deliberately broken input as well as a passing one.
pod install,xcodebuild— and build..cn1libarchives carry the new sources and pins.Deliberately out of scope, and recorded as such in the script headers: cn1-ai-whisper's JNI
.cpp, which needs the NDK and a whisper.cpp checkout and which we compile into a committed.aar, so a break shows up when we rebuild that binary and can never reach an app build.AppleMapProvider.mandCN1IntentHost.mare already compiled by the real iOS app builds inscripts-ios.yml.🤖 Generated with Claude Code