Fix SCI binding failure in DTA hosts (main)#15724
Fix SCI binding failure in DTA hosts (main)#15724nohwnd wants to merge 12 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Forward-ports the rel/18.5 fix for the System.Collections.Immutable binding mismatch affecting Microsoft.VisualStudio.TestPlatform.Common.dll on .NET Framework hosts without binding redirects (notably Azure DevOps DTA), and adds an acceptance-test regression guard.
Changes:
- Add explicit
System.Collections.ImmutablePackageReferencefor.NETFrameworkbuilds inCoreUtilitiesandObjectModelto force consistent compile-time SCI references. - Add a new
DtaLikeHosttest asset (net472) that simulates DTA-style consumption of the shippedtools/net462layout (no binding redirects). - Add new acceptance tests that run
DtaLikeHostagainst both the nupkgtools/net462/...layout and the extracted V2.CLI VSIX layout.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/TestAssets/DtaLikeHost/Program.cs | Minimal host executable that triggers FastFilter.Builder to force SCI/SRM binding at runtime. |
| test/TestAssets/DtaLikeHost/DtaLikeHost.csproj | Builds the repro host and references shipped Common.dll/ObjectModel.dll from either nupkg tools layout or extracted VSIX layout. |
| test/Microsoft.TestPlatform.Acceptance.IntegrationTests/DistributedTestAgentScenarioTests.cs | New acceptance tests that build/run DtaLikeHost as a regression guard. |
| src/Microsoft.TestPlatform.ObjectModel/Microsoft.TestPlatform.ObjectModel.csproj | Forces SCI version selection for .NETFramework builds via explicit package reference. |
| src/Microsoft.TestPlatform.CoreUtilities/Microsoft.TestPlatform.CoreUtilities.csproj | Forces SCI version selection for .NETFramework builds via explicit package reference. |
| // Directory.Build.props, copies eng/Versions.props and inserts the | ||
| // "localy-built-packages" source into NuGet.config so PackageReference to | ||
| // Microsoft.TestPlatform resolves to our locally-built nupkg. | ||
| var projectPath = GetIsolatedTestAsset("DtaLikeHost.csproj"); |
There was a problem hiding this comment.
GetIsolatedTestAsset in AcceptanceTestBase requires a targetFramework argument (GetIsolatedTestAsset(string assetName, string targetFramework)), so this call will not compile as-is. Pass the intended TFM (looks like net472 for this asset).
| var projectPath = GetIsolatedTestAsset("DtaLikeHost.csproj"); | |
| var projectPath = GetIsolatedTestAsset("DtaLikeHost.csproj", Net472TargetFramework); |
| workingDir, | ||
| "artifacts", "bin", "TestAssets", "DtaLikeHost", | ||
| IntegrationTestEnvironment.BuildConfiguration, | ||
| Net472TargetFramework, |
There was a problem hiding this comment.
Net472TargetFramework is referenced here but isn’t defined in AcceptanceTestBase (it only defines net462/net481/net11.0 constants) and doesn’t appear elsewhere, so this won’t compile. Use the literal TFM ("net472") consistently or introduce a local const in this test.
| Net472TargetFramework, | |
| "net472", |
| // System.Collections.Immutable and System.Reflection.Metadata match the DLLs | ||
| // shipped next to it, so the host exe completes normally even without any | ||
| // binding redirects in its app.config. | ||
| ExecuteApplication(exePath, args: null, out var runOut, out var runErr, out var runExit); |
There was a problem hiding this comment.
ExecuteApplication forwards args directly into ProcessStartInfo.Arguments; passing null here can throw (depending on runtime) or produce inconsistent behavior. Prefer string.Empty when no arguments are needed.
| ExecuteApplication(exePath, args: null, out var runOut, out var runErr, out var runExit); | |
| ExecuteApplication(exePath, args: string.Empty, out var runOut, out var runErr, out var runExit); |
| <!-- | ||
| Explicitly reference System.Collections.Immutable on .NET Framework for all non-test product | ||
| projects. This overrides the older version pulled in transitively by System.Reflection.Metadata | ||
| 8.0.0 and ensures every net462 assembly in the shipped nupkg and V2.CLI VSIX references the | ||
| same SCI version as the SCI DLL we ship next to it. Required to fix FileLoadException in hosts | ||
| without binding redirects (e.g. Azure DevOps Distributed Test Agent) and to keep DLLs in the | ||
| VSIX-style Common7/IDE/Extensions/TestPlatform/Extensions/ layout consistent with each other. | ||
| --> | ||
| <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework' AND '$(IsTestProject)' != 'true'"> | ||
| <PackageReference Include="System.Collections.Immutable" Version="$(SystemCollectionsImmutableVersion)" /> | ||
| </ItemGroup> |
There was a problem hiding this comment.
This adds an unconditional (per-project) System.Collections.Immutable PackageReference for every non-test .NET Framework project via Directory.Build.targets, which is broader than the PR description/forward-ported fix that was scoped to specific projects (CoreUtilities/ObjectModel). If the intent is only to fix Common/ObjectModel compile-time SCI refs, consider scoping this ItemGroup to the affected projects (or update the PR description to reflect the wider dependency change).
| // With the fix in place, Common.dll's compiled metadata references for | ||
| // System.Collections.Immutable and System.Reflection.Metadata match the DLLs | ||
| // shipped next to it, so the host exe completes normally even without any | ||
| // binding redirects in its app.config. | ||
| ExecuteApplication(exePath, args: string.Empty, out var runOut, out var runErr, out var runExit); | ||
|
|
||
| Assert.AreEqual( | ||
| 0, | ||
| runExit, | ||
| "DtaLikeHost.exe exited non-zero, which means Common.dll's compiled metadata " + | ||
| "references for System.Collections.Immutable / System.Reflection.Metadata do " + | ||
| "not match the versions shipped next to it. DTA-style hosts (no binding " + | ||
| "redirects) will FileLoadException on FastFilter.Builder.\n" + |
There was a problem hiding this comment.
The failure message (and surrounding comments) claim the scenario validates both System.Collections.Immutable and System.Reflection.Metadata binding consistency, but DtaLikeHost only exercises FastFilter/ImmutableDictionary and does not force System.Reflection.Metadata to load. As a result, SRM drift could go undetected even if the test passes. Consider explicitly loading/using a SRM type in DtaLikeHost (or adjust the messaging to only claim coverage for SCI).
| private static string GetPatchedDotnetPath() | ||
| { | ||
| var executable = OSUtils.IsWindows ? "dotnet.exe" : "dotnet"; | ||
| return Path.GetFullPath(Path.Combine(IntegrationTestEnvironment.RepoRootDirectory, "artifacts", "tmp", ".dotnet", executable)); | ||
| } |
There was a problem hiding this comment.
GetPatchedDotnetPath duplicates the patched-dotnet path computation already embedded in IntegrationTestBase (ExecutePatchedDotnet). Consider factoring the patched dotnet path into a shared/protected helper to avoid future divergence if the patched-dotnet layout changes.
| var fastFilter = fastFilterField?.GetValue(wrapper); | ||
| Console.WriteLine($"FastFilter built: {fastFilter is not null}"); |
There was a problem hiding this comment.
The regression guard can become a false negative if FastFilter stops being constructed (e.g., implementation changes to delay or skip fast filter creation). Right now the program prints FastFilter built: False and still exits 0; consider treating a missing field / null value as a failure (exit non-zero) so the host always proves it actually exercised FastFilter.Builder (and therefore the SCI bind).
| var fastFilter = fastFilterField?.GetValue(wrapper); | |
| Console.WriteLine($"FastFilter built: {fastFilter is not null}"); | |
| if (fastFilterField is null) | |
| { | |
| Console.Error.WriteLine("REPRO HIT: unable to verify FastFilter construction because the private FastFilter field was not found."); | |
| return 1; | |
| } | |
| var fastFilter = fastFilterField.GetValue(wrapper); | |
| Console.WriteLine($"FastFilter built: {fastFilter is not null}"); | |
| if (fastFilter is null) | |
| { | |
| Console.Error.WriteLine("REPRO HIT: FilterExpressionWrapper was constructed, but FastFilter was not built."); | |
| return 1; | |
| } |
| { | ||
| // Stage the DtaLikeHost asset into a temp folder. GetIsolatedTestAsset rewrites | ||
| // Directory.Build.props, copies eng/Versions.props and inserts the | ||
| // "localy-built-packages" source into NuGet.config so PackageReference to |
There was a problem hiding this comment.
Typo in comment: "localy-built-packages" should be "locally-built-packages".
| // "localy-built-packages" source into NuGet.config so PackageReference to | |
| // "locally-built-packages" source into NuGet.config so PackageReference to |
| <PlatformAbstractionsDepsJsonFiles Include="..\..\Microsoft.TestPlatform.PlatformAbstractions\bin\$(Configuration)\$(TargetFramework)\*.deps.json"></PlatformAbstractionsDepsJsonFiles> | ||
| <PackageDepsJsonFiles Include="..\..\Microsoft.TestPlatform.PlatformAbstractions\bin\$(Configuration)\$(TargetFramework)\*.deps.json"></PackageDepsJsonFiles> | ||
| <SystemCollectionsImmutableFiles Include="$(PkgSystem_Collections_Immutable)\lib\netstandard2.0\*"></SystemCollectionsImmutableFiles> | ||
| <SystemCollectionsImmutableFiles Include="$(PkgSystem_Collections_Immutable)\lib\net462\*"></SystemCollectionsImmutableFiles> |
There was a problem hiding this comment.
SystemCollectionsImmutableFiles is now hard-coded to lib\net462\*. Since the repo already has $(NetFrameworkMinimum) set to net462, consider using that property in the path to avoid drift if the minimum .NET Framework TFM ever changes (and to make the intent clearer).
| <SystemCollectionsImmutableFiles Include="$(PkgSystem_Collections_Immutable)\lib\net462\*"></SystemCollectionsImmutableFiles> | |
| <SystemCollectionsImmutableFiles Include="$(PkgSystem_Collections_Immutable)\lib\$(NetFrameworkMinimum)\*"></SystemCollectionsImmutableFiles> |
| and System.Reflection.Metadata v8.0.0.0, while the package ships v9.0.0.0 of both | ||
| next to it, so any host without a matching binding redirect is expected to | ||
| FileLoadException when SCI / SRM is loaded. |
There was a problem hiding this comment.
The explanatory comment describes the package shipping System.Collections.Immutable and System.Reflection.Metadata v9.0.0.0 “of both next to it”. In this repo, SRM is pinned to 8.0.0 (and the app.config redirects reflect that), and SCI is now 9.0.0.11—so this comment is misleading for the repro scenario. Please update the version text / description to match what we actually ship so future debugging doesn’t chase the wrong mismatch.
| and System.Reflection.Metadata v8.0.0.0, while the package ships v9.0.0.0 of both | |
| next to it, so any host without a matching binding redirect is expected to | |
| FileLoadException when SCI / SRM is loaded. | |
| and System.Reflection.Metadata v8.0.0.0, while the package ships | |
| System.Collections.Immutable v9.0.0.11 and System.Reflection.Metadata v8.0.0.0 | |
| next to it, so any host without a matching binding redirect is expected to | |
| FileLoadException when SCI is loaded. |
Force .NET Framework product projects to compile against the netstandard2.0 build of System.Collections.Immutable (AssemblyVersion 9.0.0.0) instead of the net462 build (AssemblyVersion 9.0.0.11). SCI 9.0.11 introduced an AV divergence between the two TFMs; the nupkg ships the netstandard2.0 DLL, so compiled metadata must reference 9.0.0.0 for consumers without binding redirects (e.g. Azure DevOps Distributed Test Agent) to avoid FileLoadException. - Directory.Build.targets: ExcludeAssets=compile on SCI PackageReference + explicit Reference to netstandard2.0 DLL for .NET Framework product projects - Extend binding redirect oldVersion to cover 9.0.0.11 - Add DtaLikeHost test asset and acceptance test Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
624e723 to
e6d27b1
Compare
|
wait we need to ship nestanardd, fixing |
Add explicit System.Collections.Immutable PackageReference to CoreUtilities and ObjectModel for non-.NETCoreApp targets (netstandard2.0 + .NETFramework). This ensures all product assemblies compile against SCI 9.0.0.0 (the netstandard2.0 assembly version), matching the SCI DLL shipped in nupkgs. Extend SCI binding redirect oldVersion range to 9.0.0.11 in app.configs to cover the net462 assembly version from SCI 9.0.11. Suppress MSB3277 for .NETCoreApp targets in Directory.Build.targets since the netstandard2.0 SCI 9.0.0.0 reference is harmless on .NET Core (the runtime loads whatever version is available regardless of the reference). Fix MSBuildWarningsAsMessages inheritance in packaging projects that was overwriting parent values instead of appending. Add acceptance test that validates SCI version alignment in nupkg/VSIX layouts using PEReader/MetadataReader. Fixes microsoft#15718 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…i-main # Conflicts: # test/Microsoft.TestPlatform.Acceptance.IntegrationTests/DistributedTestAgentScenarioTests.cs
The merge with the old PR branch brought back the ExcludeAssets/HintPath workaround that was replaced by the simpler PackageReference approach. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The previous condition '!= .NETCoreApp' included netstandard2.0, which made netstandard2.0 assemblies reference SCI 9.0.0.0. This breaks on .NET 8 where the shared framework only has SCI 8.0.0.0. Change to '== .NETFramework' so only net462 builds get the explicit SCI reference (covered by binding redirects), while netstandard2.0 keeps SCI 8.0.0.0 from the SRM transitive dependency. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The DTA scenario (loading SCI without binding redirects) cannot be fully fixed with SCI 9.0.11 due to assembly version divergence between net462 (9.0.0.11) and netstandard2.0 (9.0.0.0). This will be tracked in a separate issue. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SCI 9.0.11 has different assembly versions for net462 (9.0.0.11) vs netstandard2.0 (9.0.0.0). DTA hosts without binding redirects need exact version match. SCI 10.0.0 has AV 10.0.0.0 for all TFMs. - Bump SystemCollectionsImmutablePackageVersion to 10.0.0 - Update binding redirects to 10.0.0.0 - Restore DTA acceptance test with correct DLL name Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ilds verify-binding-redirects auto-corrected: - System.Memory: 4.0.1.2 -> 4.0.5.0 (Release build ships newer AV) - System.Runtime.CompilerServices.Unsafe: 6.0.0.0 -> 6.0.3.0 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Previous count of 484 was from a dirty build with stale artifacts. Clean Release build produces 389 files, matching CI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
System.Memory 4.6.3 (transitive from SCI 10.0.0) depends on System.Buffers. In Release builds, System.Buffers.dll has AV 4.0.5.0, but MSTest adapter loads against 4.0.3.0. Without a redirect, the CLR cannot resolve the mismatch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fix DTA hosts crashing with
FileLoadExceptiononSystem.Collections.Immutable 8.0.0.0.Why
SCI 9.0.11 has different assembly versions per TFM: net462 gets 9.0.0.11, netstandard2.0 gets 9.0.0.0. Product assemblies (Common.dll etc.) are netstandard2.0 and compile against SCI 8.0.0.0 (from the SRM 8.0.0 transitive dependency). The nupkg ships SCI 9.0.0.0. DTA loads Common.dll without binding redirects, asks for 8.0.0.0, finds 9.0.0.0 — crash.
Changes
MSBuildWarningsAsMessagesinheritance in packaging projects (was overwriting parent values).Fixes #15718
Related PRs: #15722 (rel/18.6), #15723 (rel/18.7)