-
Notifications
You must be signed in to change notification settings - Fork 565
[hot reload] implement DeployHotReloadAgentConfiguration target
#10699
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
base: main
Are you sure you want to change the base?
[hot reload] implement DeployHotReloadAgentConfiguration target
#10699
Conversation
Context: dotnet/sdk#52492 Context: https://github.com/dotnet/runtime/blob/79609d7dbef01231b0694a356de1d4e5618785cd/src/libraries/System.Private.CoreLib/src/System/StartupHookProvider.cs#L32 Context: https://github.com/dotnet/runtime/blob/79609d7dbef01231b0694a356de1d4e5618785cd/src/mono/mono/metadata/object.c#L8137-L8140 Context: b2332ca Added a new `DeployHotReloadAgentConfiguration` MSBuild target that `dotnet-watch` can use to configure Hot Reload for Android apps. Notes on runtimes: * MonoVM currently passes `string.Empty` to `ProcessStartupHooks` and relies on reading `STARTUP_HOOKS` from `AppContext`. * To fix CoreCLR, we can read `DOTNET_STARTUP_HOOKS` from environment variables, and pass to `ProcessStartupHooks`. * NativeAOT likely works as-is. I added/updated tests to verify these changes.
|
|
||
| <!-- Parse DotNetHotReloadAgentEnvironment (semicolon-separated KEY=VALUE pairs) --> | ||
| <ItemGroup Condition=" '$(DotNetHotReloadAgentEnvironment)' != '' "> | ||
| <_HotReloadEnvironment Include="$(DotNetHotReloadAgentEnvironment)" /> |
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.
I realized it'd be easier to make DotNetHotReloadAgentEnvironment an item group.
<DotNetHotReloadAgentEnvironment Include="<name>" Value="<value>"/>
That way we don't need to worry about escaping = and ;.
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.
Updated to use an item group now.
| @@ -0,0 +1 @@ | |||
| DOTNET_STARTUP_HOOKS=StartupHook | |||
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.
Note this wasn't being used in some of our on-device tests, because we had:
<ItemGroup Condition=" '$(Configuration)' == 'Debug' ">
<!-- trying to track:
JNI ERROR (app bug): accessed deleted Global 0x3056
-->
<AndroidEnvironment Include="env.txt" />I just made a new file for this.
| @@ -0,0 +1,84 @@ | |||
| <!-- | |||
| *********************************************************************************************** | |||
| Microsoft.Android.Sdk.HotReload.targets | |||
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.
@rolfbjarne do you see anything problematic in this file for iOS? $(DotNetHotReloadAgentStartupHook) will have the full path to the .dll.
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.
The solution for iOS will be rather different, but it doesn't look impossible.
Introduces a new test to verify that the DOTNET_STARTUP_HOOKS environment variable is set to 'StartupHook'. Also renames an existing test method for clarity.
| <!-- Set STARTUP_HOOKS via RuntimeHostConfigurationOption for MonoVM and NativeAOT (read via AppContext.GetData) --> | ||
| <RuntimeHostConfigurationOption Include="STARTUP_HOOKS" Value="StartupHook" Condition=" '$(UseMonoRuntime)' == 'true' or '$(PublishAot)' == 'true' " /> |
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.
Filed this to fix NativeAOT later:
| - DotNetHotReloadAgentStartupHook (Property): Path to Microsoft.Extensions.DotNetDeltaApplier.dll | ||
| - DotNetHotReloadAgentEnvironment (ItemGroup): Environment variables as <DotNetHotReloadAgentEnvironment Include="name" Value="value" /> | ||
| --> | ||
| <Target Name="DeployHotReloadAgentConfiguration" |
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.
The caller of this target will have to set the Device property, so that we know which device/simulator to deploy to.
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.
Yes, after we have the initial part working, I hope to share the code with dotnet-run that will prompt for a target framework and device. So, you should get the same prompts and property.
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.
Hot Reload is typically used from an IDE though, and the IDE in question must pass the selected device in this case.
|
|
||
| <!-- Add the Hot Reload agent DLL as a reference so it gets deployed --> | ||
| <ItemGroup> | ||
| <Reference Include="$(DotNetHotReloadAgentStartupHook)" Condition=" Exists('$(DotNetHotReloadAgentStartupHook)') " /> |
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.
Does this still work if the app is trimmed? What if the hot reload agent assembly contains references to BCL APIs that were trimmed away?
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.
They have the BCL part setup here:
I also did this in a test project that seems to work in all runtimes/configurations:
<TrimmerRootAssembly Include="StartupHook" RootMode="All" />We might have to do this, if Debug builds are trimmed on iOS.
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.
Hot Reload isn't supported for trimmed apps.
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.
My usage of @(TrimmerRootAssembly) was to test startup hooks in general.
But yeah, hot reload, would break randomly if they decided to use System.Guid, for example, during a reload and it was trimmed away.
iOS Debug builds run the trimmer, but I bet it is setup to preserve almost everything. We may need to review that behavior.
@rolfbjarne it's also worth noting we are mostly targeting CoreCLR for this -- if that helps anything.
Context: dotnet/sdk#52492
Context: https://github.com/dotnet/runtime/blob/79609d7dbef01231b0694a356de1d4e5618785cd/src/libraries/System.Private.CoreLib/src/System/StartupHookProvider.cs#L32
Context: https://github.com/dotnet/runtime/blob/79609d7dbef01231b0694a356de1d4e5618785cd/src/mono/mono/metadata/object.c#L8137-L8140
Context: b2332ca
Added a new
DeployHotReloadAgentConfigurationMSBuild target thatdotnet-watchcan use to configure Hot Reload for Android apps.Notes on runtimes:
MonoVM currently passes
string.EmptytoProcessStartupHooksand relies on readingSTARTUP_HOOKSfromAppContext.To fix CoreCLR, we can read
DOTNET_STARTUP_HOOKSfrom environment variables, and pass toProcessStartupHooks.NativeAOT likely works as-is.
I added/updated tests to verify these changes.