Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add hot reload apply changes API: AssemblyExtensions.ApplyUpdate #48366

Merged
merged 8 commits into from
Feb 18, 2021
Merged

Add hot reload apply changes API: AssemblyExtensions.ApplyUpdate #48366

merged 8 commits into from
Feb 18, 2021

Conversation

mikem8361
Copy link
Member

Issue: #45689

Currently Windows only: there will be another PR to enable this API on Linux/MacOS.

The Hot reload API will fail if debugging or if the module isn't editable.

@mikem8361 mikem8361 self-assigned this Feb 16, 2021
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-issue-labeler
Copy link

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.

@ghost
Copy link

ghost commented Feb 16, 2021

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

Issue: #45689

Currently Windows only: there will be another PR to enable this API on Linux/MacOS.

The Hot reload API will fail if debugging or if the module isn't editable.

Author: mikem8361
Assignees: mikem8361
Labels:

area-Diagnostics-coreclr, new-api-needs-documentation
Milestone: -

stephentoub
stephentoub reviewed Feb 16, 2021
View reviewed changes
src/coreclr/System.Private.CoreLib/src/System/Reflection/Metadata/AssemblyExtensions.cs Outdated Show resolved Hide resolved
src/coreclr/System.Private.CoreLib/src/System/Reflection/Metadata/AssemblyExtensions.cs Outdated Show resolved Hide resolved
src/coreclr/System.Private.CoreLib/src/System/Reflection/Metadata/AssemblyExtensions.cs Outdated Show resolved Hide resolved
src/coreclr/System.Private.CoreLib/src/System/Reflection/Metadata/AssemblyExtensions.cs Outdated Show resolved Hide resolved
src/coreclr/vm/encee.cpp Outdated Show resolved Hide resolved
src/libraries/System.Runtime.Loader/ref/System.Runtime.Loader.cs Outdated Show resolved Hide resolved
src/coreclr/System.Private.CoreLib/src/System/Reflection/Metadata/AssemblyExtensions.cs Outdated Show resolved Hide resolved
@jkotas
Copy link
Member

jkotas commented Feb 16, 2021

Any tests to add?

@mikem8361
Copy link
Member Author

mikem8361 commented Feb 16, 2021 •
edited
Loading

We (Aleksey and I) are going to work on some tests. It will take a while to put them together and I didn't want to hold up getting in API so the dotnet-watch tooling (@pranavkm) can start using it.

noahfalk
noahfalk approved these changes Feb 17, 2021
View reviewed changes
Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I anticipate we'll continue looking into the debugger interaction and error handling, but if the code as-is helps unblock anyone it seems like a fine starting point.

@noahfalk
Copy link
Member

fyi @davidwrighton

@jkotas
Copy link
Member

jkotas commented Feb 17, 2021

We (Aleksey and I) are going to work on some tests.

It should be straightforward to add basic tests that verify at least some of the error handling as part of this PR.

src\libraries\System.Runtime.Loader\tests is a good place to add tests like that.

@lambdageek lambdageek mentioned this pull request Feb 17, 2021
Merged
jkotas
jkotas reviewed Feb 18, 2021
View reviewed changes
src/libraries/System.Runtime.Loader/tests/ApplyUpdateTests.cs Outdated
@@ -0,0 +1,35 @@
// Licensed to the .NET Foundation under one or more agreements.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: AssemblyExtensionsTest.cs may be a better name for this file. The convention is to use the same name for the file and the main type in the file.

jkotas
jkotas approved these changes Feb 18, 2021
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@mikem8361 mikem8361 merged commit 3464c88 into dotnet:master Feb 18, 2021
@mikem8361 mikem8361 deleted the hotreloadapi branch February 18, 2021 23:02
@ghost ghost locked as resolved and limited conversation to collaborators Mar 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tmat tmat tmat left review comments

@lambdageek lambdageek lambdageek left review comments

@stephentoub stephentoub stephentoub left review comments

@noahfalk noahfalk noahfalk approved these changes

@jkotas jkotas jkotas approved these changes

@marek-safar marek-safar Awaiting requested review from marek-safar

Assignees

@mikem8361 mikem8361

Labels
area-Diagnostics-coreclr new-api-needs-documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mikem8361 @jkotas @noahfalk @tmat @lambdageek @stephentoub