[System.Configuration] Fix settings upgrade on iOS and tvOS - #132283
[System.Configuration] Fix settings upgrade on iOS and tvOS#132283steveisok wants to merge 4 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Fixes settings upgrade discovery on iOS/tvOS by switching the identity component used for user.config paths from an installation-specific executable path hash to a stable main bundle identifier hash, with a constrained fallback scan of legacy directories to locate prior-version settings.
Changes:
- Add a
System.NativePAL entrypoint to retrieve the main app bundle identifier on Apple platforms. - Update
ClientConfigPathsto use a bounded hash of the bundle identifier for iOS/tvOS settings identity, and expose a legacy prefix for constrained migration lookup. - Extend
LocalFileSettingsProviderprevious-config resolution to prefer stable-identity history, then (when applicable) fall back to legacy identity directories with filtering/ambiguity safeguards; add targeted tests.
Show a summary per file
| File | Description |
|---|---|
| src/native/libs/System.Native/pal_environment.m | Implements SystemNative_GetMainBundleIdentifier via NSBundle.mainBundle.bundleIdentifier. |
| src/native/libs/System.Native/pal_environment.h | Exposes the new PAL export for managed interop. |
| src/native/libs/System.Native/pal_environment.c | Provides a non-Apple stub returning NULL. |
| src/native/libs/System.Native/entrypoints.c | Registers the new entrypoint for libSystem.Native DllImport dispatch. |
| src/libraries/System.Configuration.ConfigurationManager/src/System/Configuration/AppleApplication.cs | Adds managed interop helper to retrieve and free the bundle identifier string. |
| src/libraries/System.Configuration.ConfigurationManager/src/System/Configuration/ClientConfigPaths.cs | Uses bundle identifier hashing for iOS/tvOS identity suffix; adds LegacyConfigDirectoryPrefix. |
| src/libraries/System.Configuration.ConfigurationManager/src/System/Configuration/LocalFileSettingsProvider.cs | Refactors previous-config discovery into FindPreviousConfigFile with stable-first + constrained legacy fallback. |
| src/libraries/System.Configuration.ConfigurationManager/tests/System/Configuration/LocalFileSettingsProviderTests.cs | Adds unit tests covering stable identity hashing and prior-config selection/fallback rules. |
| src/libraries/System.Configuration.ConfigurationManager/src/System.Configuration.ConfigurationManager.csproj | Includes the new AppleApplication.cs in compilation. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 0
- Review effort level: Lite
|
Tagging subscribers to 'os-ios': @vitek-karas, @kotlarmilos, @steveisok, @akoeplinger |
| [LibraryImport("libSystem.Native", EntryPoint = "SystemNative_Free")] | ||
| internal static partial void Free(IntPtr ptr); | ||
|
|
||
| [LibraryImport("libSystem.Native", EntryPoint = "SystemNative_GetMainBundleIdentifier")] |
There was a problem hiding this comment.
System.Configuration.ConfigurationManager is standalone nuget package. It cannot depend on native shims.
There was a problem hiding this comment.
From https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/interop-guidelines.md#unix-shims : The System.Native shims are a private implementation detail of the Microsoft.NETCore.App shared framework and are intended only for use by code inside of the shared framework.
There was a problem hiding this comment.
Thanks - I couldn't remember and was operating on like it was ok. I'll adjust to something else.
There was a problem hiding this comment.
Agreed. I removed the System.Native dependency and now import the required CFBundle/CFString APIs directly from CoreFoundation, keeping the OOB package self-contained. The direct CoreFoundation path also passed the full iOS simulator suite.
Note
This reply was generated with GitHub Copilot.
| } | ||
|
|
||
| [Fact] | ||
| public void AppleMobileIdentity_UsesStableBoundedBundleIdentifierHash() |
There was a problem hiding this comment.
This problem is not limited to Apple.
Windows store apps have it too. The paths to Windows store apps look like C:\Program Files\WindowsApps\Microsoft.Windows.Photos_2026.11080.5006.0_x64__8wekyb3d8bbwe and they are changing with each update.
There was a problem hiding this comment.
I'll see if I can address this too.
There was a problem hiding this comment.
Addressed as well. Packaged Windows apps now use GetCurrentPackageFamilyName as the stable identity, while unpackaged Windows retains the existing behavior. The constrained legacy lookup is shared by both stable identity paths.
Note
This reply was generated with GitHub Copilot.
|
System.Configuration.Manager is not trimming and AOT compatible (by design). Is it really worth trying to "fix it" to work better on mobile? It is never going to be in a happy place on mobile and other form factors that require trimming or AOT to work well. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2a374416-1441-4b46-aeee-27cb227d0d78
Fair point. There's still some use in the face of that as I've seen issues around this pop up over time. I think a narrow fix is still worthwhile. |
There was a problem hiding this comment.
Review details
Suppressed comments (3)
src/libraries/System.Configuration.ConfigurationManager/src/System/Configuration/WindowsApplication.cs:38
- GetCurrentPackageFamilyName currently throws Win32Exception for unexpected return codes. Since the package family name is used only as a best-effort stable identity (with an existing legacy fallback), letting this throw will make ClientConfigPaths construction fail and can break settings access in packaged apps if the API ever returns an error. Consider treating non-success as "no stable identity" and returning null instead.
if (error != ERROR_INSUFFICIENT_BUFFER)
{
throw new Win32Exception(error);
}
src/libraries/System.Configuration.ConfigurationManager/tests/System/Configuration/LocalFileSettingsProviderTests.cs:134
- This test uses reflection to locate AppleApplication/GetMainBundleIdentifier; if the type or method name changes, it will currently fail with a NullReferenceException rather than a clear assertion failure. Adding explicit null asserts makes failures easier to diagnose.
Type appleApplication = typeof(LocalFileSettingsProvider).Assembly.GetType("System.Configuration.AppleApplication");
MethodInfo getMainBundleIdentifier = appleApplication.GetMethod(
"GetMainBundleIdentifier",
BindingFlags.NonPublic | BindingFlags.Static);
src/libraries/System.Configuration.ConfigurationManager/src/System/Configuration/ClientConfigPaths.cs:162
- LegacyConfigDirectoryPrefix/StableConfigDirectoryName are currently enabled based only on stableIdentity being non-empty. If hashing the stable identity falls back to the legacy exe-path/strong-name suffix (e.g., due to PlatformNotSupportedException in GetApplicationIdentitySuffix), this will still treat the current legacy directory as "stable" and change the upgrade lookup behavior (searching across legacy identity directories). Consider enabling these properties only when the computed hashSuffix actually used the stable identity type.
LegacyConfigDirectoryPrefix = !string.IsNullOrEmpty(stableIdentity) &&
!string.IsNullOrEmpty(namePrefix)
? namePrefix + "_"
: null;
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
…stem/Configuration/WindowsApplication.cs Co-authored-by: Jan Kotas <jkotas@microsoft.com>
…stem/Configuration/WindowsApplication.cs Co-authored-by: Jan Kotas <jkotas@microsoft.com>
| return null; | ||
| } | ||
|
|
||
| foreach (DirectoryInfo versionDirectory in identityDirectory.GetDirectories()) |
There was a problem hiding this comment.
Just curious if there's a reason to prefer GetDirectories over EnumerateDirectories here?
Fixes #121053
Packaged applications can move between installation paths when updated.
ClientConfigPathspreviously hashed that installation-specific executable path, soApplicationSettingsBase.Upgrade()could not locate settings from an earlier installation.This change:
No public APIs are added. The standalone package does not depend on runtime-private native shims.
Validation:
Note
This pull request description was generated with GitHub Copilot.