sdk
sdk copied to clipboard
dotnet
fix: Don't fail metadata updates on missing assemblies
This change is about https://developercommunity.visualstudio.com/t/WSL-HotReload-does-not-invoke-MetadataUp/10643733, where metadata updates may fail when assemblies cannot be found in a cross-platform scenario.
In failure context, an app is built from Visual Studio, then executed on Linux, where WPF assemblies are not present, but some non-loaded assemblies reference WPF. When metadata update gets applied, searching for the MetadataUpdateHandler may fail because some dependencies may not be found when Assembly.GetCustomAttributesData() is called and fails to find the rest of the handlers which stops invoking MetadataUpdateHandler types.
Skipping the assembly entirely allows for hot reload to successfully completely.
This change is technically not useful for dotnet watch, but I understand from @danroth27 that these files are synchronized to Visual Studio, where the above scenario applies.
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.
The sdk-unified-build test failure is unrelated and will go away when you update your branch to target latest main.
It'd be good to add a test. The test can compile a project that has a dependent project and delete the dependent dll from the output directory before making the source change. That should emulate the scenario well. ApplyDeltaTests.HandleTypeLoadFailure is already testing a similar scenario. Perhaps you could parameterize the test to target this scenario as well.
ApplyDeltaTests.HandleTypeLoadFailureis already testing a similar scenario
@tmat I've been trying to enable the test for this specific scenario, but HandleTypeLoadFailure has been disabled recently because of https://github.com/dotnet/sdk/issues/42850 and I can't run this new test for the same reason. I'll make a change that does the same, but do you have a known way to test this specific scenario regardless, or do we need to wait for net10?
@jeromelaban Let me try to reenable the tests. I was on vacation when they got disabled.
@jeromelaban Turns out main is currently transitioning to .NET 10 and has some version inconsistencies between runtime and the SDK, which causes issues with some dotnet-watch tests. See https://github.com/dotnet/sdk/issues/42920 for status.
Would you mind rebasing your changes to release/9.0.1xx branch? Thanks for patience!
@tmat Sure, thanks for the investigation! Do you want me to retarget this PR, or to make another one targeting 9.0.1xx and keep this one on main as well?
@tmat Now that the other PR is merged, does this PR still make sense? I'm not sure how backports work there. Thanks!
@tmat I see that the backport that was done to 9.0.x did not get merged to main. My understanding is that I should update this PR so that it stays in 10..
Also, it seems that this change did not end up being included in VS 17.12, would you know if it will be in 17.13?
Thanks!
@jeromelaban The fix is in 9.0.1xx, 9.0.2xx and main now. You can close this PR now.
Yes, should definitely be in 17.13.