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.

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

Hot Reload: test on WebAssembly #53050

Merged
merged 3 commits into from
May 25, 2021
Merged

Conversation

lambdageek
Copy link
Member

  1. Always build the assemblies in the ApplyUpdate/ subdirectory without
    optimization, with debug info. Hot reload depends on it.
  2. Pass the required environment variable to the runtime via xharness to enable
    support for applying updates.

1. Always build the assemblies in the ApplyUpdate/ subdirectory without
optimization, with debug info.  Hot reload depends on it.
2. Pass the required environment variable to the runtime via xharness to enable
support for applying updates.
@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.

Comment on lines +49 to +61
<Target Name="IncludeDeltasInWasmBundle"
BeforeTargets="PrepareForWasmBuildApp"
Condition="'$(TargetOS)' == 'Browser'">
<ItemGroup>
<!-- FIXME: this belongs in eng/testing/tests.wasm.targets -->
<!-- FIXME: Can we do something on the Content items in the referenced projects themselves to get this for free? -->
<WasmFilesToIncludeInFileSystem Include="@(PublishItemsOutputGroupOutputs)"
Condition="$([System.String]::new('%(PublishItemsOutputGroupOutputs.Identity)').EndsWith('.dmeta'))" />
<WasmFilesToIncludeInFileSystem Include="@(PublishItemsOutputGroupOutputs)"
Condition="$([System.String]::new('%(PublishItemsOutputGroupOutputs.Identity)').EndsWith('.dil'))" />
<WasmFilesToIncludeInFileSystem Include="@(PublishItemsOutputGroupOutputs)"
Condition="$([System.String]::new('%(PublishItemsOutputGroupOutputs.Identity)').EndsWith('.dpdb'))" />
</ItemGroup>
Copy link
Member Author

@lambdageek lambdageek May 20, 2021

Choose a reason for hiding this comment

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

@radical help! I don't like how I did this. For some reason the <Content> that my referenced projects are creating (using the hot reload build tool, below) don't get copied into the wasm filesystem.

https://github.com/dotnet/hotreload-utils/blob/6a82938e7b952f014a08816e871e06741feabb14/src/Microsoft.DotNet.HotReload.Utils.Generator.BuildTool/build/Microsoft.DotNet.HotReload.Utils.Generator.BuildTool.targets#L59-L68

I'm not looking forward to doing this for Android and iOS, too. Maybe there's some general metadata I can set on those items so the publish process will put them in the right place in the wasm app bundle? (They're loaded using File.ReadAllBytes so putting them in \supportFiles actually works out. Just wish they'd get there automatically)

Copy link
Member

Choose a reason for hiding this comment

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

IIUC:

  • @(Content) is added in project (*ApplyUpdate*), and the main test project references that

  • tests.wasm.targets adds @(ContentWithTargetPath) to the wasm vfs

    • but that gets populated only by the @(Content) items from the test project itself
    • Out of all full set of copy-local items from referenced projects, I couldn't figure out a way to differentiate the ones that came from @(Content)[1]
    • it wouldn't make sense to add all the copy-local files to the vfs though!
  • It would be nice to have some idiom for what gets automatically added to the vfs, and maybe WasmApp.targets can use that

  • Maybe, XI/XA have a solution for this? @rolfbjarne @jonathanpeppers

  • Otherwise, I think, at least for now, your approach is fine 😬

--

  1. um there is a property that would allow retaining the original metadata from the Content, so that could be helpful here, but it sounds hacky!

Copy link
Member Author

Choose a reason for hiding this comment

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

um there is a property that would allow retaining the original metadata from the Content, so that could be helpful here, but it sounds hacky!

To be honest filtering by metadata sounds better to me than going by file extensions.

If I put DeltaOutputType metadata on the Content in the references (it's already there on the _HotReloadDeltaGeneratorOutputs, but I forgot to tack on the KeepMetadata thing), then, hopefully, that will propagate to the parent project's PublishItemsOutputGroupOutputs?
And then I would feel less bad about adding them to WasmFilesToIncludeInFileSystem in eng/testing/tests.wasm.targets.

Copy link
Member

Choose a reason for hiding this comment

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

On Android we don't actually use @(Content) at all and emit warnings. This is just because of the way .apk files are compressed on disk, we can't put a file somewhere where File.Open() would even work.

You might be able to do something like this to grab files from all @(ProjectReference), though:

<MSBuild Projects="@(ProjectReference)" Targets="HotReloadDeltaGeneratorContentForOutputs">
  <Output TaskParameter="TargetOutputs" ItemName="_ContentFromProjectReference" />
</MSBuild>

Not sure if that helps or not.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not quite sure I understand what you need to do. It seems that you're trying to build an app that contains both the original assemblies + the modified assemblies that should be loaded through hot reload, and then those modified assemblies should be in a specific location within the app bundle, is that right?

@ghost
Copy link

ghost commented May 20, 2021

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

Issue Details
  1. Always build the assemblies in the ApplyUpdate/ subdirectory without
    optimization, with debug info. Hot reload depends on it.
  2. Pass the required environment variable to the runtime via xharness to enable
    support for applying updates.
Author: lambdageek
Assignees: -
Labels:

area-System.Runtime

Milestone: -

@lambdageek lambdageek added the arch-wasm WebAssembly architecture label May 20, 2021
@lambdageek
Copy link
Member Author

/cc @mikem8361 for the hot reload tests, the assemblies that will be modified are now always compiled with debug info. Don't think this will affect anything for CoreCLR testing, but just a heads up.

@lambdageek
Copy link
Member Author

The Browser wasm Release AllSubsets_Mono_EAT test failure is relevant. What's EAT? looks like the assemblies are being rewritten. Probably need to skip the EnC tests - they really can't deal with any sort of rewriting being done to the baseline assemblies.

@radical
Copy link
Member

radical commented May 20, 2021

EAT = EnableAggressiveTrimming. You could preserve the assembly that you don't want trimmed, eg. with <TrimmerRootAssembly Include=".." />

@lambdageek lambdageek merged commit 6b37d9b into dotnet:main May 25, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-System.Runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants