Repository navigation
feat!: Replace CommonServiceLocator with IServiceProvider in LogExtensionPoint - #94
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The PR description does not include a required linked GitHub issue, and the reported dotnet test Uno.Core.Tests run indicates a failing test that should be addressed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR removes the runtime dependency on CommonServiceLocator from Uno.Core.Extensions.Logging.Singleton by switching the ambient ILoggerFactory lookup to an optional System.IServiceProvider, avoiding shipping CommonServiceLocator.dll in downstream Uno apps and reducing licensing risk.
Changes:
- Removed the
CommonServiceLocatorpackage reference andServiceLocatorusage fromLogExtensionPoint. - Added
LogExtensionPoint.ServiceProviderfor optional first-use resolution ofILoggerFactory, preserving the existing fallback behavior to a defaultLoggerFactory. - Added unit tests covering service-provider resolution, fallback paths, and exception behavior.
File summaries
| File | Description |
|---|---|
| src/Uno.Core.Extensions.Logging.Singleton/LogExtensionPoint.cs | Replaces service-locator-based factory resolution with an optional IServiceProvider and retains fallback logic. |
| src/Uno.Core.Extensions.Logging.Singleton/Uno.Core.Extensions.Logging.Singleton.csproj | Drops the CommonServiceLocator package reference so it no longer ships transitively. |
| src/Uno.Core.Tests/LogExtensionPointTests.cs | Adds tests validating ServiceProvider behavior, default fallback, and exception propagation. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…sionPoint BREAKING CHANGE: consumers that supplied an ILoggerFactory through ServiceLocator.SetLocatorProvider must now assign LogExtensionPoint.ServiceProvider instead. Consumers that never set a locator provider - which is every app using Microsoft.Extensions.DependencyInjection - are unaffected, since that path already fell through to a default LoggerFactory. Uno.Core.Extensions.Logging.Singleton is referenced by Uno.WinUI, so its dependency on CommonServiceLocator ships CommonServiceLocator.dll inside every Uno application. That package is Microsoft.Practices.ServiceLocation, copyright Microsoft 2008, and it is licensed under the MS-PL, which the FSF classifies as incompatible with the GPL and AGPL. Every GPL/AGPL-licensed Uno app therefore redistributes an assembly it cannot relicense, over a service locator API that predates Microsoft.Extensions.DependencyInjection. The whole dependency was one optional lookup in GetFactory(), so replace it with System.IServiceProvider, which is in the BCL and is what MS.DI already implements. - Add LogExtensionPoint.ServiceProvider, an optional IServiceProvider used to resolve the ambient ILoggerFactory on first use - Resolve through it in GetFactory() instead of ServiceLocator.Current, keeping the existing fallback to a default LoggerFactory - Preserve the original exception behaviour exactly: NullReferenceException and InvalidOperationException fall back, anything else still propagates, now expressed as an exception filter rather than a catch-and-rethrow - Drop the CommonServiceLocator PackageReference Uno.Core.Extensions.Compatibility keeps its CommonServiceLocator reference: it exposes IServiceLocator in its public API by design, and it is not shipped as part of Uno applications. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XD6CXamcmygU1ErZvjfc9v
The ServiceLocator branch this replaces had no test coverage. - Resolve the ambient factory from a stub IServiceProvider - Fall back to a default factory when no provider is set, and when the provider returns something that is not an ILoggerFactory - Propagate exceptions other than NullReference/InvalidOperation - Reset LogExtensionPoint.ServiceProvider between tests Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XD6CXamcmygU1ErZvjfc9v
fd660ef to
2ad5927
Compare
+semver: major master moves to the 5.0 line so servicing/4.2 can keep publishing 4.2.0-dev.* for consumers on the 4.x API. The 5.0 line carries the LogExtensionPoint change in #94, which replaces CommonServiceLocator with System.IServiceProvider and is a breaking API change. Consumers needing the old surface stay on 4.2.x.
Addresses PR review: the LoggerFactory created in AmbientLoggerFactory_ServiceProviderResolvesFactory_UsesResolvedFactory was never disposed. Disposing it alone would leave the static AmbientLoggerFactory cache pointing at a disposed instance, so Cleanup now clears that cache too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QnbcC6sgycChA1huPHfnnF
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Tick the box to add this pull request to the merge queue (same as
|
morning4coffe-dev
left a comment
There was a problem hiding this comment.
Reviewed at df01d38.
The IServiceProvider substitution is scoped to Logging.Singleton and preserves optional first-use resolution, cached-factory behavior, the existing interceptor path, and the expected fallback/exception-propagation policy. The Compatibility package retains its legacy locator contracts, and the breaking configuration migration is explicitly documented.
No blocking findings.
What
Uno.Core.Extensions.Logging.Singletonis referenced byUno.WinUI, so its dependency on CommonServiceLocator ends up shippingCommonServiceLocator.dllinside every Uno application. This PR removes that dependency by swapping the service locator lookup forSystem.IServiceProvider.Why
Two reasons, one legal and one architectural.
Licensing. CommonServiceLocator is licensed under the MS-PL, which the FSF classifies as incompatible with the GPL. Every GPL/AGPL-licensed Uno app currently redistributes an assembly it cannot relicense under its own terms. This came up while producing third-party licence notices for an AGPL-licensed Uno app — it is the only non-permissive licence in the entire shipped dependency graph.
Age. The package is
Microsoft.Practices.ServiceLocation, copyright Microsoft 2008 — the patterns & practices service locator, predatingMicrosoft.Extensions.DependencyInjection.Verified it genuinely ships (not a dev-time dependency):
CommonServiceLocator.dllis copied to the output fornet10.0-desktop,net10.0-windows10.0.26100(includingAppX/) andnet10.0-androidin a stock Uno app.How
The entire dependency was one optional lookup in
GetFactory():Replaced with
System.IServiceProvider— in the BCL on every target framework here, and the abstractionMicrosoft.Extensions.DependencyInjectionalready implements. No new package reference, and nothing reflective (so no trimming or AOT cost).LogExtensionPoint.ServiceProvider, an optionalIServiceProviderused to resolve the ambientILoggerFactoryon first useGetFactory()resolves through it, keeping the existing fallback to a defaultLoggerFactoryNullReferenceExceptionandInvalidOperationExceptionfall back to a default factory, anything else still propagates — now expressed as an exception filter instead of catch-and-rethrowCommonServiceLocatorPackageReferenceBreaking change
Consumers that supplied an
ILoggerFactoryviaServiceLocator.SetLocatorProvider(...)must now assignLogExtensionPoint.ServiceProviderinstead:Consumers that never set a locator provider — every app using
Microsoft.Extensions.DependencyInjection— are unaffected, since that path already fell through tonew LoggerFactory().Scope
Deliberately limited to
Uno.Core.Extensions.Logging.Singleton, the only app-shipped package that referenced CommonServiceLocator (Collections,Disposables,EqualityandLoggingnever did).Uno.Core.Extensions.Compatibilitykeeps its reference: it exposesIServiceLocatorin its public API by design (IServiceLocatorProvider,ServiceLocatorProviderExtensions), and it is not shipped as part of Uno applications. Removing it there would be a much larger API break for no redistribution benefit — happy to follow up separately if you'd like it gone repo-wide.Testing
The replaced branch had no test coverage. Added 5 tests to
LogExtensionPointTestscovering resolution from a stub provider, both fallback paths, exception propagation, and the property round-trip.dotnet test Uno.Core.Tests— 267 passed, 1 failed, 2 skipped.The single failure is pre-existing and unrelated:
ValidationHelperFixture.CurrencyValidationassertsIsCurrency("1 121,2", fr-fr)using a plain space (U+0020), but ICU on modern Windows returns U+202F (narrow no-break space) as the fr-FR group separator. It touches no logging code. Happy to file that separately.🤖 Generated with Claude Code
https://claude.ai/code/session_01XD6CXamcmygU1ErZvjfc9v