[tools] Keep track of removed/unavailable frameworks in our list of frameworks.#25490
Conversation
…rameworks. This simplifies some code, and also makes it easier to determine what to do with some bindings we still have for some of these frameworks.
There was a problem hiding this comment.
Pull request overview
This PR updates the tooling framework metadata so that frameworks that have been removed (or are otherwise unavailable) can be identified via a unified API, with the goal of simplifying and centralizing framework-availability checks across bundler/linker/registrar code paths.
Changes:
- Extend
Framework/Frameworksto track “unavailable as of SDK version” (VersionUnavailable) and introduceFramework.IsFrameworkUnavailable (Application app). - Update the static registrar and module-reference linking logic to use the new “unavailable” concept instead of simulator-only checks and hardcoded Xcode-version special cases.
- Update the dotnet linker step to consult framework availability before inlining
Dlfcncalls.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/dotnet-linker/Steps/InlineDlfcnMethodsStep.cs | Switches inlining-skip logic to consult framework availability metadata. |
| tools/common/StaticRegistrar.cs | Uses framework availability metadata to decide imports/registration for unavailable frameworks. |
| tools/common/Frameworks.cs | Adds versioned unavailability tracking + new availability helper, and records removed frameworks (AssetsLibrary/NewsstandKit). |
| tools/common/Assembly.cs | Updates framework-linking decisions for module references to use the new unavailability helper. |
| tools/common/Application.cs | Renames/replaces the simulator-availability helper with IsFrameworkUnavailable. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
tools/common/Frameworks.cs:688
AssetsLibraryis no longer listed among the frameworks marked unavailable for Mac Catalyst in this switch. SinceGetMacCatalystFrameworksstarts from the iOS framework list, omitting it here means it will be treated as available on Mac Catalyst (and even have itsVersionbumped to 13.0), which is likely incorrect and can lead to link/import failures. AddAssetsLibraryback to the Mac Catalyst-unavailable set (either in the explicit list or the xtro/unknown list) if it’s still unsupported on Catalyst.
case "ARKit":
case "BrowserEngineCore":
case "CarPlay":
case "WatchConnectivity":
f.Unavailable = true;
|
/review |
|
✅ .NET for Apple Platforms PR Reviewer completed successfully! |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
✅ Overall Assessment: Clean refactoring with suggestions for improvement
This PR successfully consolidates framework availability logic into a single IsFrameworkUnavailable() method, eliminating duplicate Xcode version checks and improving maintainability. The approach is sound and the implementation is generally correct.
Findings
- 0 ❌ errors
- 1
⚠️ warnings - 5 💡 suggestions
What's Good
- Excellent consolidation: The new
IsFrameworkUnavailable()method unifies simulator, SDK version, and framework-specific unavailability checks into one place - Data-driven approach: Using
VersionUnavailableto track when frameworks become unavailable is cleaner than scattered Xcode version checks - Code cleanup: Removed ~30 lines of duplicate logic from StaticRegistrar.cs
- Correct nullability handling: Added null checks for
TypeDefinition?parameters inTryGetFrameworkmethods - Consistent log messages: Updated messages to use generic "current SDK" language instead of "simulator" or "Xcode 15+"
Suggestions for Improvement
-
⚠️ Semantic clarity: TheUnavailableproperty name is potentially confusing since it's set totruewheneverVersionUnavailableis provided, even if the framework is currently available in older SDKs. Consider renaming or documenting this more clearly. -
💡 Documentation: Add brief comments for the version numbers (17.4 for AssetsLibrary, 17.0 for NewsstandKit) to help maintainers understand which Xcode/SDK versions these correspond to.
-
💡 Consistency: Mac Catalyst exclusions use direct
Unavailable = trueassignment while iOS uses theversion_unavailableparameter pattern. Consider documenting why or making the patterns consistent.
CI Status
CI checks are still running. The code changes look correct, but please ensure all checks pass before merging.
Note: No test changes were included in this PR. The refactoring maintains existing behavior, so existing tests should continue to pass. If there are specific test scenarios for framework availability detection, consider verifying they cover the new consolidated logic.
Generated by .NET for Apple Platforms PR Reviewer for issue #25490 · ● 3.3M
✅ [PR Build #d4d2f2f] Build passed (Detect API changes) ✅Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
✅ [PR Build #d4d2f2f] Build passed (Build packages) ✅Pipeline on Agent |
✅ API diff for current PR / commitNET (empty diffs)✅ API diff vs stableNET (empty diffs)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
✅ [PR Build #d4d2f2f] Build passed (Build macOS tests) ✅Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
🚀 [CI Build #d4d2f2f] Test results 🚀Test results✅ All tests passed on VSTS: test results. 🎉 All 183 tests passed 🎉 Tests counts✅ cecil: All 1 tests passed. ( macOS tests✅ Tests on macOS Monterey (12): All 5 tests passed. Html Report (VSDrops) Download Linux Build VerificationPipeline on Agent |
This simplifies some code, and also makes it easier to determine what to do with
some bindings we still have for some of these frameworks.