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

[wasm] Avoid unnecessary relinking when publishing a blazor project for AOT #82748

Merged
merged 11 commits into from
Mar 2, 2023

Conversation

radical
Copy link
Member

@radical radical commented Feb 28, 2023

Publish target triggers Build target to run. When publishing, we don't want
to run relinking step during the Build, as it will be run for Publish
anyway. Earlier there wasn't a good way to differentiate the two cases of
build target - dotnet build, and dotnet publish, but now the sdk sets
$(_IsPublishing)=true, which can be used here.

  • VS uses $(DeployOnBuild), and that is still supported

  • And add tests to run blazor+AOT

@radical radical added arch-wasm WebAssembly architecture area-Build-mono labels Feb 28, 2023
@ghost ghost assigned radical Feb 28, 2023
@ghost
Copy link

ghost commented Feb 28, 2023

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

Issue Details
  • And add tests to run blazor+AOT
Author: radical
Assignees: -
Labels:

arch-wasm, area-Build-mono

Milestone: -

@radical
Copy link
Member Author

radical commented Mar 1, 2023

Depends on dotnet/xharness#1001

radical added 6 commits March 1, 2023 02:14
.. relinking during "Build", when publishing.

`Publish` target triggers `Build` target to run. When publishing, we
don't want to run relinking step during the `Build`, as it will be run
for `Publish` anyway. Earlier there wasn't a good way to differentiate
the two cases of `build` when building, and `build` when publishing, but
now the sdk sets `$(_IsPublishing)=true`, which can be used here.
`dotnet run` uses the regular build output. Published blazor app, like
when using AOT, needs to be run more "manually" by starting a web server
in the publish folder. This is accomplished here by using the new
xharness command `wasm webserver`.
@radical
Copy link
Member Author

radical commented Mar 1, 2023

This adds missing tests that run AOT'ed blazor app. And that repros the currently broken state.

@radical radical marked this pull request as ready for review March 1, 2023 05:48
Copy link
Member

@maraf maraf left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@@ -172,7 +173,7 @@
<!-- Use a unique property, so the already run wasm targets can also run -->
<MSBuild Projects="$(MSBuildProjectFile)"
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 curious how this works.
Do we run separate MSBuild process here (which doesn't share variables and items from parent ?)
Do we still need to have it as separate process ?
Do we still need _WasmInNestedPublish_UniqueProperty_XYZ ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we run separate MSBuild process here (which doesn't share variables and items from parent ?)

No, but it runs it in a fresh context. I will be removing this, but I'm waiting for the blazor<->runtime build changes to happen.

Do we still need to have it as separate process ?

It's not necessarily a separate process. And this PR doesn't change anything about this.

Do we still need _WasmInNestedPublish_UniqueProperty_XYZ ?

Yep

Essentially, this PR doesn't change any behavior w.r.t the nested publish. The nested publish bit was a hacky solution, and will be removed. But I'm holding that off till we get further on the build consolidation with blazor.

Copy link
Member

Choose a reason for hiding this comment

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

@ilonatommy opened issue for ICU related dependency on not nesting it
#82819

Updating 'Microsoft.DotNet.XHarness.CLI': '1.0.0-prerelease.23117.1' => '1.0.0-prerelease.23151.1' (from build '20230301.1' of 'https://github.com/dotnet/xharness')
Updating 'Microsoft.DotNet.XHarness.TestRunners.Common': '1.0.0-prerelease.23117.1' => '1.0.0-prerelease.23151.1' (from build '20230301.1' of 'https://github.com/dotnet/xharness')
Updating 'Microsoft.DotNet.XHarness.TestRunners.Xunit': '1.0.0-prerelease.23117.1' => '1.0.0-prerelease.23151.1' (from build '20230301.1' of 'https://github.com/dotnet/xharness')
@radical
Copy link
Member Author

radical commented Mar 2, 2023

Added the xharness dependency update here, from #82600

@radical
Copy link
Member Author

radical commented Mar 2, 2023

The failures are unrelated and #82868 .

@radical radical requested review from maraf and pavelsavara March 2, 2023 16:51
@radical radical merged commit 8162ac9 into dotnet:main Mar 2, 2023
@radical radical deleted the blazor-aot-main branch March 2, 2023 17:02
@ghost ghost locked as resolved and limited conversation to collaborators Apr 1, 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.

3 participants