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

[browser] Unify boot config schema and output layout #86255

Merged
merged 89 commits into from
Jun 28, 2023

Conversation

maraf
Copy link
Member

@maraf maraf commented May 15, 2023

  • Update WasmAppBuilder to produce the same boot config schema as GenerateWasmBootJson
  • Update AppBundle layout to match blazor layout.
    • All the runtime files (js, wasm, icu) are deployed to _framework folder.
    • This behavior can be overriden by setting <WasmRuntimeAssetsLocation> property. The empty value is overriden to default _framework, but ./ can be used to flatten the output structure.
    • WBT test with flat output
    • The only remaining difference is AppBundle vs wwwrooot folder name.
  • JSHost.ImportAsync now requires the same relative paths as in blazor (./module.js => ../module.js)

Contributes to #70762

@maraf maraf added arch-wasm WebAssembly architecture area-Build-mono labels May 15, 2023
@maraf maraf added this to the 8.0.0 milestone May 15, 2023
@maraf maraf self-assigned this May 15, 2023
@ghost
Copy link

ghost commented May 15, 2023

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

Unify scheme between GenerateWasmBootJson and WasmAppBuilder

Author: maraf
Assignees: maraf
Labels:

arch-wasm, area-Build-mono

Milestone: 8.0.0

@radical
Copy link
Member

radical commented Jun 23, 2023

@ilonatommy can you please check the icu bits in WasmAppBuilder?

@maraf
Copy link
Member Author

maraf commented Jun 23, 2023

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@radical
Copy link
Member

radical commented Jun 24, 2023

I tried to verify this with a dotnet-runtime-perf run, but that timed out because of resource issues. Try running some benchmarks (even one will do) using instructions from https://github.com/dotnet/performance/blob/main/docs/benchmarking-workflow-dotnet-runtime.md#dotnet-runtime-testing-for-wasm .

Copy link
Member

@ilonatommy ilonatommy left a comment

Choose a reason for hiding this comment

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

ICU related part looks good

@maraf
Copy link
Member Author

maraf commented Jun 27, 2023

@radical dotnet/performance is missing --module when running V8. If I hacked around with --wasmArgs I was able to convince it to run with test-main.js from this PR.

Do you have more feedback?

@radical
Copy link
Member

radical commented Jun 27, 2023

@radical dotnet/performance is missing --module when running V8. If I hacked around with --wasmArgs I was able to convince it to run with test-main.js from this PR.

Do you have more feedback?

This is handled for CI, and the docs for manual running need to be updated.
But I just realized that it will be problematic with the file (test-main.mjs) being renamed only in the scripts. I will try a simple fix in perf repo, and open a PR for it, and then you can update this PR.

@maraf
Copy link
Member Author

maraf commented Jun 27, 2023

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@radical
Copy link
Member

radical commented Jun 27, 2023

@radical dotnet/performance is missing --module when running V8. If I hacked around with --wasmArgs I was able to convince it to run with test-main.js from this PR.
Do you have more feedback?

This is handled for CI, and the docs for manual running need to be updated. But I just realized that it will be problematic with the file (test-main.mjs) being renamed only in the scripts. I will try a simple fix in perf repo, and open a PR for it, and then you can update this PR.

The fixes will take a little longer. They are blocked by other things, for example dotnet/BenchmarkDotNet#2346 .

@radical
Copy link
Member

radical commented Jun 27, 2023

@maraf I've pushed a commit to add --module for v8.

The reason the break shows up in this PR, and not on main is:

-                File.Copy(Path.Combine(AppContext.BaseDirectory,
-                                        options.TargetFramework == "net8.0" ? "test-main.js" : "data/test-main-7.0.js"),
-                            Path.Combine(_projectDir, "test-main.js"));
+                File.Copy(
+                    Path.Combine(
+                        AppContext.BaseDirectory,
+                        string.IsNullOrEmpty(options.TargetFramework) || options.TargetFramework == "net8.0"
+                            ? "test-main.js"
+                            : "data/test-main-7.0.js"
+                    ),
+                    Path.Combine(_projectDir, "test-main.js")
+                );

In the options.TargetFramework case it was using the older test-main-7.0.js. Testing wise it is not a big issue since this is essentially testing for library tests, but your changes fixes it to use the current file.

In my upcoming PRs I will be moving most of the WBT tests away from test-main*, to use templates instead.

Copy link
Member

@radical radical left a comment

Choose a reason for hiding this comment

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

LGTM! Great job 👍

@radical radical merged commit f623089 into dotnet:main Jun 28, 2023
@maraf
Copy link
Member Author

maraf commented Jun 28, 2023

@maraf I've pushed a commit to add --module for v8.

The reason the break shows up in this PR, and not on main is:

-                File.Copy(Path.Combine(AppContext.BaseDirectory,
-                                        options.TargetFramework == "net8.0" ? "test-main.js" : "data/test-main-7.0.js"),
-                            Path.Combine(_projectDir, "test-main.js"));
+                File.Copy(
+                    Path.Combine(
+                        AppContext.BaseDirectory,
+                        string.IsNullOrEmpty(options.TargetFramework) || options.TargetFramework == "net8.0"
+                            ? "test-main.js"
+                            : "data/test-main-7.0.js"
+                    ),
+                    Path.Combine(_projectDir, "test-main.js")
+                );

In the options.TargetFramework case it was using the older test-main-7.0.js. Testing wise it is not a big issue since this is essentially testing for library tests, but your changes fixes it to use the current file.

In my upcoming PRs I will be moving most of the WBT tests away from test-main*, to use templates instead.

Thank you for tracking it down!

@maraf maraf deleted the WasmUnifyBootConfigSchema branch June 28, 2023 06:03
maraf pushed a commit that referenced this pull request Jun 30, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Build-mono
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants