-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Don't fail metadata updates on missing assemblies #40725
base: main
Are you sure you want to change the base?
Conversation
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. |
c760626
to
05d3a71
Compare
The sdk-unified-build test failure is unrelated and will go away when you update your branch to target latest main. |
Looks like it did the trick, thanks @ViktorHofer! |
// In such case, we can ignore the assemblies and continue enumerating handlers for | ||
// the rest of the assemblies of current domain. | ||
_log($"'{assembly.FullName}' is not loaded ({e.Message})"); | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: continue is not needed.
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. |
b9912ec
to
5c668c4
Compare
432c7b3
to
33eb9b9
Compare
2ed2655
to
a336fe7
Compare
@tmat I've been trying to enable the test for this specific scenario, but |
@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 #42920 for status. Would you mind rebasing your changes to |
@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? |
Either way works. |
@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! |
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 whenAssembly.GetCustomAttributesData()
is called and fails to find the rest of the handlers which stops invokingMetadataUpdateHandler
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.