Remove unnecessary binding redirects from app.configs#15786
Conversation
Remove all dependentAssembly binding redirects from the three net462 host app.configs (testhost.x86, datacollector, vstest.console). Binding redirects are a legacy .NET Framework mechanism that papers over version mismatches instead of fixing them, and actively cause harm when they get out of sync with the shipped DLLs (see #15765). Removed redirects: - ObjectModel (old adapter compat v11-14, no longer needed) - TestWindow.Interfaces, UnitTestFramework (VS-only, don't ship) - FileSystemGlobbing, CompilerServices.Unsafe, Collections.Immutable, Reflection.Metadata, Memory, Buffers (System polyfills) - DiagnosticSource, Text.Encodings.Web (don't ship, were harmful) Updated verify-binding-redirects.ps1 to enforce zero redirects and fail if any are re-introduced. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Love how optimistically this is described :D let's see it crumble :D |
There was a problem hiding this comment.
Pull request overview
This PR removes all assembly binding redirects from the .NET Framework app.config files for vstest.console, testhost.x86, and datacollector, and updates the packaging verification script to enforce a “no binding redirects” policy going forward (motivated by #15765).
Changes:
- Removed all
<dependentAssembly>/<bindingRedirect>entries from the three shipped app.config files. - Simplified
eng/verify-binding-redirects.ps1to fail when binding redirects are present (rather than trying to sync redirect versions from extracted packages).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/vstest.console/app.config | Removes all binding redirects from the vstest.console .NET Framework config. |
| src/testhost.x86/app.config | Removes all binding redirects while keeping probing configuration. |
| src/datacollector/app.config | Removes all binding redirects from the datacollector .NET Framework config. |
| eng/verify-binding-redirects.ps1 | Changes validation from “keep redirects in sync” to “redirects forbidden”. |
| $dependentAssemblies = $xml.SelectNodes("//asm:dependentAssembly", $nsMgr) | ||
| foreach ($dep in $dependentAssemblies) { | ||
| $identity = $dep.SelectSingleNode("asm:assemblyIdentity", $nsMgr) | ||
| $redirect = $dep.SelectSingleNode("asm:bindingRedirect", $nsMgr) | ||
| if (-not $identity -or -not $redirect) { continue } | ||
|
|
| if (-not $errors) { | ||
| Write-Host " OK - no binding redirects." | ||
| } | ||
| } |
nohwnd
left a comment
nohwnd
left a comment
There was a problem hiding this comment.
Review summary
The overall direction is correct — removing stale binding redirects and replacing the sync-version logic with a zero-tolerance check is a clear improvement, well-motivated by issue #15765.
Two issues in eng/verify-binding-redirects.ps1:
-
Logic bug in "OK" output (line 52):
if (-not $errors)tests the global accumulator, not the current entry. A later clean config won't print "OK" if any earlier config failed. Needs a per-iteration flag (details inline). -
Orphaned mandatory parameters (lines 17–18):
$PackageDirsand$Configurationare still[Parameter(Mandatory)]but the function no longer uses them. The call inverify-nupkgs.ps1still satisfies the constraint, so it won't error at runtime — but they're dead code that should be cleaned up.
No concerns with the app.config changes themselves; the deletions match the description exactly.
🧠 Reviewed by expert-reviewer workflow
🧠 Reviewed by Expert Code Reviewer 🧠
This comment has been minimized.
This comment has been minimized.
|
Closing - removing all binding redirects breaks testhost because it loads arbitrary user assemblies that may reference older versions of shipped DLLs (e.g. System.Collections.Immutable 8.0.0.0 when we ship 10.0.0.0). The redirects for shipped DLLs are actually needed for version unification. The real fix is scoped: remove only orphaned redirects (DLLs we don't ship), which is handled by #15778 (merged) and #15776/#15777 for release branches. |
|
Understood — removing this from my tracking. The correct approach is the scoped fix (removing only orphaned redirects for DLLs we don't ship), which is covered by #15778 and the release branch PRs. Closing this out.
|
Remove all binding redirects from testhost.x86, datacollector, and vstest.console app.configs. These were redirects for ObjectModel (old v11-14 adapter compat), VS-only DLLs (TestWindow.Interfaces, UnitTestFramework), System polyfills (Memory, Buffers, CompilerServices.Unsafe, Collections.Immutable, Reflection.Metadata, FileSystemGlobbing), and DLLs that don't even ship (DiagnosticSource, Encodings.Web — see #15765 for the damage those caused).
Updated verify-binding-redirects.ps1 to enforce zero redirects going forward — it now fails if any <dependentAssembly>\ elements are found, instead of trying to keep them in sync.