feat: [SDK-4774] support Unity no-location builds#877
Conversation
The scripted iOS/Android builds use ManagedStrippingLevel.High, which stripped the reflection-only OneSignaliOS/OneSignalAndroid platform implementations. _getDefaultInstance() then threw "Sequence contains no elements", so OneSignal never initialized and the permission prompt never appeared. Add a link.xml (matching the main demo) to preserve the OneSignal assemblies, and align the demo UI with the .NET no-location demo (Permission + Push ID rows). Co-authored-by: Cursor <cursoragent@cursor.com>
Explain that Assets/OneSignal/link.xml must not be deleted: scripted builds use high managed stripping and the OneSignal platform classes are reflection-resolved, so removing it breaks SDK initialization. Co-authored-by: Cursor <cursoragent@cursor.com>
Adds a "Send Test Notification" button that posts to the OneSignal REST API targeting the current push subscription, matching the .NET and Capacitor no-location demos. Enables the UnityWebRequest module needed for the request. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@claude review once |
| sleep 30 | ||
| done | ||
| if [ "$FOUND" != "true" ]; then echo "✗ Android SDK version ${VERSION} not found after 5 attempts"; exit 1; fi | ||
| sed -i -E "s/spec=\"com\.onesignal:OneSignal:[0-9][0-9.]*\"/spec=\"com.onesignal:OneSignal:$VERSION\"/" com.onesignal.unity.android/Editor/OneSignalAndroidDependencies.xml | ||
| sed -i -E "s/const string Version = \"[0-9][0-9.]*\"/const string Version = \"$VERSION\"/" com.onesignal.unity.android/Editor/OneSignalAndroidDependencies.cs | ||
| sed -i -E "s/<androidPackage spec=\"com\.onesignal:OneSignal:[0-9][0-9.]*\"/<androidPackage spec=\"com.onesignal:OneSignal:$VERSION\"/" examples/demo/Assets/OneSignal/Editor/OneSignalAndroidDependencies.xml | ||
| sed -i -E "s/'com\.onesignal:OneSignal:[0-9][0-9.]*'/'com.onesignal:OneSignal:$VERSION'/" examples/demo/Assets/Plugins/Android/mainTemplate.gradle | ||
| sed -i -E "s/<package>com\.onesignal:OneSignal:[0-9][0-9.]*<\/package>/<package>com.onesignal:OneSignal:$VERSION<\/package>/" examples/demo/ProjectSettings/AndroidResolverDependencies.xml | ||
|
|
There was a problem hiding this comment.
🔴 The release workflow in .github/workflows/create-release-pr.yml only updates files under examples/demo/ and never touches the new examples/demo-no-location/ project, so the four committed dependency files there (Assets/OneSignal/Editor/OneSignal{Android,iOS}Dependencies.xml, Assets/Plugins/Android/mainTemplate.gradle, ProjectSettings/AndroidResolverDependencies.xml) will retain stale versions after each release. Additionally, the existing sed patterns only match the umbrella com.onesignal:OneSignal:VERSION artifact and would not match this demo's granular artifacts (core/notifications/in-app-messages, OneSignalXCFramework/OneSignal\[InAppMessages\]) even if the paths were added. Fix needs both path additions and new sed patterns that target the granular coordinate forms.
Extended reasoning...
What the bug is. This PR adds a new demo project at examples/demo-no-location/ and checks in four files that hardcode the OneSignal native SDK versions (currently 5.9.5 for Android, 5.5.3 for iOS). The release workflow at .github/workflows/create-release-pr.yml is responsible for bumping these hardcoded versions in the umbrella examples/demo/ project on every release, but its sed steps only enumerate examples/demo/ paths — examples/demo-no-location/ is never touched.
The specific code path. Lines 113-115 (Android) update examples/demo/Assets/OneSignal/Editor/OneSignalAndroidDependencies.xml, examples/demo/Assets/Plugins/Android/mainTemplate.gradle, and examples/demo/ProjectSettings/AndroidResolverDependencies.xml. Line 137 (iOS) updates examples/demo/Assets/OneSignal/Editor/OneSignaliOSDependencies.xml. None of these touch examples/demo-no-location/. Beyond the missing paths, the regexes themselves are umbrella-artifact-only: com\.onesignal:OneSignal:[0-9][0-9.]* matches com.onesignal:OneSignal:5.9.5 but does not match the granular forms used by the no-location demo (com.onesignal:core:5.9.5, com.onesignal:notifications:5.9.5, com.onesignal:in-app-messages:5.9.5).
Why existing code doesn't prevent it. The new OneSignalAndroidDependencies.cs / OneSignaliOSDependencies.cs files do regenerate the XML manifests via [InitializeOnLoad] when Unity opens the project, so the two XMLs under Assets/OneSignal/Editor/ will eventually self-heal — but only once a developer opens demo-no-location in the Unity Editor. The release workflow runs in CI without Unity, so the committed release state still ships stale XML. More importantly, Assets/Plugins/Android/mainTemplate.gradle and ProjectSettings/AndroidResolverDependencies.xml are EDM4U snapshots that only refresh on an explicit Force Resolve, which is not triggered by anything in the release pipeline.
Impact. A release bump produces a commit where examples/demo/ is at the new version and examples/demo-no-location/ is still at the previous version, across both Android (gradle + resolver xml) and iOS (pod manifest). Anyone checking out the release tag sees inconsistent demo state. SDK consumers installing via UPM are unaffected — this is a release-engineering / demo-hygiene correctness issue, not a runtime bug.
Step-by-step proof. Suppose the next release is invoked with android_version=5.10.0, ios_version=5.6.0:
- Workflow updates
com.onesignal.unity.android/Editor/OneSignalAndroidDependencies.csVersion const →5.10.0. ✓ - Workflow runs
sed -i -E "s/<androidPackage spec=\"com\.onesignal:OneSignal:[0-9][0-9.]*\"/.../" examples/demo/Assets/OneSignal/Editor/OneSignalAndroidDependencies.xml→ matches and rewrites. ✓ - Workflow does NOT run sed on
examples/demo-no-location/Assets/OneSignal/Editor/OneSignalAndroidDependencies.xml. The file still readscom.onesignal:core:5.9.5,com.onesignal:notifications:5.9.5,com.onesignal:in-app-messages:5.9.5. ✗ - Even if the path were added, the regex
com\.onesignal:OneSignal:[0-9][0-9.]*does not matchcom.onesignal:core:5.9.5(artifact idcore, notOneSignal). Still stale. ✗ - Same outcome for
examples/demo-no-location/Assets/Plugins/Android/mainTemplate.gradle(implementation 'com.onesignal:core:5.9.5'etc.) andexamples/demo-no-location/ProjectSettings/AndroidResolverDependencies.xml(<package>com.onesignal:core:5.9.5</package>etc.). ✗ - The release commit lands with
examples/demo/updated to 5.10.0/5.6.0 andexamples/demo-no-location/stuck at 5.9.5/5.5.3.
How to fix. Extend the Android step to also run granular-artifact substitutions across the demo-no-location files, e.g.:
for artifact in core notifications in-app-messages; do
sed -i -E "s/(com\.onesignal::)[0-9][0-9.]*/\1$VERSION/g" \
examples/demo-no-location/Assets/OneSignal/Editor/OneSignalAndroidDependencies.xml \
examples/demo-no-location/Assets/Plugins/Android/mainTemplate.gradle \
examples/demo-no-location/ProjectSettings/AndroidResolverDependencies.xml
doneAnd the iOS step needs to update examples/demo-no-location/Assets/OneSignal/Editor/OneSignaliOSDependencies.xml — the existing version="[0-9][0-9.]*" regex would actually match the granular OneSignalXCFramework/OneSignal and OneSignalXCFramework/OneSignalInAppMessages iosPod entries, so just adding the path to line 137 is sufficient on the iOS side.
| public bool IsShared | ||
| { | ||
| get => _location.Call<bool>("isShared"); | ||
| set => _location.Call("setShared", value); | ||
| get | ||
| { | ||
| try | ||
| { | ||
| return _location != null && _location.Call<bool>("isShared"); | ||
| } | ||
| catch (Exception) | ||
| { | ||
| SDKDebug.Error(LocationModuleNotAvailable); | ||
| return false; | ||
| } | ||
| } | ||
| set | ||
| { | ||
| try | ||
| { | ||
| if (_location != null) | ||
| _location.Call("setShared", value); | ||
| } | ||
| catch (Exception) | ||
| { | ||
| SDKDebug.Error(LocationModuleNotAvailable); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 On Android, when the native location module is missing, IsShared getter/setter silently no-op without logging because the _location != null short-circuit skips the try-block before SDKDebug.Error can fire. This is inconsistent with RequestPermission in the same file (which always logs via the caught NRE) and with the iOS bridge in OneSignalUnityBridgeLocation.mm (which logs on every call). Suggest dropping the null guards (let the NRE catch and log, matching RequestPermission) or adding an explicit if (_location == null) { SDKDebug.Error(LocationModuleNotAvailable); ... } branch in each accessor.
Extended reasoning...
Bug: In com.onesignal.unity.android/Runtime/AndroidLocationManager.cs the constructor catches the missing-module exception from sdkClass.CallStatic<AndroidJavaObject>("getLocation"), logs LocationModuleNotAvailable once, and leaves _location = null. After that, the IsShared accessors are guarded with a null short-circuit:
- Getter (line 61):
return _location != null && _location.Call<bool>("isShared");— when_location == null, C# short-circuits the&&and returnsfalsewithout entering the try body, soSDKDebug.Erroris never invoked. - Setter (lines 73-74):
if (_location != null) _location.Call("setShared", value);— same short-circuit; nothing is logged. RequestPermission(line 88): unconditionally dereferences_location.Call(...), so a null_locationthrows aNullReferenceExceptionthat the catch block converts intoSDKDebug.Error(LocationModuleNotAvailable)on every call.
Why existing code doesn't prevent it: the try/catch around each accessor only catches the JNI exception that would come from a real Call(...) on a non-null _location. The null guard added in this PR sits outside that path, so no exception is ever thrown for the null branch and the diagnostic log added in the same change is unreachable for the IsShared accessors.
Impact: the PR description says it 'logs SDK-level errors when location APIs are called without the native location module', and the iOS bridge (OneSignalUnityBridgeLocation.mm) enforces that contract by logging OneSignalLocationModuleNotFoundMessage on every _oneSignalLocationGetIsShared, _oneSignalLocationSetIsShared, and _oneSignalLocationRequestPermission call via the ONESIGNAL_DISABLE_LOCATION macro. On Android, RequestPermission matches that contract but the IsShared accessors are silent for every call after construction. A developer toggling off the module and trying to debug why OneSignal.Location.IsShared = true had no effect on Android would see only the one boot-time error, while the iOS counterpart would emit an API-call-site error each time. This is a cross-platform diagnostic divergence rather than a functional break — the no-op + false return value documented in the README still holds.
Step-by-step proof: consider the no-location demo (which this PR adds) with disableLocation = true. (1) AndroidOneSignal.Initialize constructs AndroidLocationManager. (2) sdkClass.CallStatic<AndroidJavaObject>("getLocation") throws because the com.onesignal:location module is absent; the constructor's catch logs once. (3) The app reads OneSignal.Location.IsShared once per frame to display state. (4) Each call enters the getter, evaluates _location != null to false, short-circuits, returns false. No Call is made, no exception, the catch block is dead code for this path. (5) On iOS, the same loop hits _oneSignalLocationGetIsShared, which goes through #if ONESIGNAL_DISABLE_LOCATION and logs every frame. Result: same code path, divergent diagnostic output.
Addressing the refutation: the refuter is right that (a) the constructor already logs once, (b) the README documents the no-op contract, and (c) logging every frame can be argued as spam. Those are fair points — which is why this is filed as a nit, not normal severity. But the PR's own description and the iOS implementation set the precedent that each API call without the module produces a log, and RequestPermission in this same file already honors that contract via the NRE catch. The Android IsShared accessors are the only callers that diverge. If log-once-at-boot is the intended contract, the iOS bridge and RequestPermission should be updated to match; otherwise IsShared should follow them. Either resolution works — the current state where three of four call sites log and one doesn't is the issue.
Fix: remove the null short-circuit so the NRE path catches and logs (matching RequestPermission), or add an explicit early-return:
get {
if (_location == null) {
SDKDebug.Error(LocationModuleNotAvailable);
return false;
}
try { return _location.Call<bool>("isShared"); }
catch (Exception) { SDKDebug.Error(LocationModuleNotAvailable); return false; }
}
Description
One Line Summary
Adds a no-location Unity demo and a robust flag-controlled native location dependency opt-out for Unity SDK builds.
Details
Motivation
Apps that do not use OneSignal location APIs should be able to exclude the native location module from iOS and Android builds. This also aligns Unity with the .NET, Capacitor, and Cordova SDK opt-out patterns while avoiding mutation of shared SDK package files during dependency resolution.
Scope
ONESIGNAL_DISABLE_LOCATIONsupport layered over the existing Unity Editor setting, with the environment variable taking precedence for CLI and CI builds.Assets/OneSignal/Editorinstead of rewriting shared packageEditorfiles.examples/demo-no-location, including iOS/Android scripts, build settings, no-location native dependency sets, stripping preservation, and a test notification flow.Testing
Unit testing
Not added; these changes are primarily Unity Editor build pipeline, native dependency resolution, and demo build behavior.
Manual testing
examples/demo-no-location/run-ios.sh --no-installgenerated granular iOS pods withoutOneSignalLocation.xcodebuildforexamples/demo-no-locationiOS simulator completed withEXIT=0; verified noOneSignalLocation.frameworkwas linked.examples/demo/run-ios.sh --no-installgenerated umbrella iOS and Android dependency manifests for the main demo.examples/demo-no-location/run-android.sh --no-installbuilt the APK with granular Android dependencies (core,notifications,in-app-messages)../run-all.sh --sdk=unity --platform=iospassed: 34 tests passing.Affected code checklist
Checklist
Overview
Testing
Final pass
Made with Cursor