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

[browser] Wasm SDK packed as a nuget package #84082

Merged
merged 18 commits into from
Apr 12, 2023
Merged

[browser] Wasm SDK packed as a nuget package #84082

merged 18 commits into from
Apr 12, 2023

Conversation

maraf
Copy link
Member

@maraf maraf commented Mar 29, 2023 •
edited
Loading

Produce a Microsoft.NET.Sdk.WebAssembly.Pack package containing most of the Wasm SDK bits.

Contributes to #81367

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

ghost commented Mar 29, 2023

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

Issue Details
  • TBD
Author: maraf
Assignees: maraf
Labels:

arch-wasm, area-Build-mono
Milestone: 8.0.0

@maraf maraf mentioned this pull request Mar 30, 2023
Closed
49 tasks
@steveisok steveisok self-requested a review March 30, 2023 16:18
This was referenced Mar 31, 2023
Open
Closed
@build-analysis build-analysis bot mentioned this pull request Apr 4, 2023
Closed
This was referenced Apr 6, 2023
Open
Closed
@lewing lewing closed this Apr 11, 2023
@lewing lewing reopened this Apr 11, 2023
@lewing lewing requested a review from pavelsavara April 11, 2023 05:16
@lewing
Copy link
Member

lewing commented Apr 11, 2023

kicked this over now that the baseline is green. Cursory reading looks fine, what is left to do?

@maraf maraf marked this pull request as ready for review April 11, 2023 11:31
@radical
Copy link
Member

radical commented Apr 11, 2023

Just curious - why do we have Pack in the name?

radical
radical reviewed Apr 11, 2023
View reviewed changes
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.

Just some comments at a glance. I'm guessing that most of the code is being moved from elsewhere (could you point to the original sources?), so I didn't really review in detail.

src/tasks/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks/AssetsComputingHelper.cs Outdated Show resolved Hide resolved
src/tasks/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks/ComputeWasmBuildAssets.cs Outdated Show resolved Hide resolved
src/tasks/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks/ComputeWasmBuildAssets.cs Outdated Show resolved Hide resolved
src/tasks/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks/ComputeWasmBuildAssets.cs Outdated
[Output]
public ITaskItem[] FilesToRemove { get; set; }

public override bool Execute()
Copy link
Member

Choose a reason for hiding this comment

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

nit: I would suggest renaming this to ExecuteInternal, called by Execute which has:

runtime/src/tasks/WasmAppBuilder/ManagedToNativeGenerator.cs

Lines 55 to 64 in 19ff978

try
{
ExecuteInternal();
return !Log.HasLoggedErrors;
}
catch (LogAsErrorException e)
{
Log.LogError(e.Message);
return false;
}

This would make the main method simpler, and allow use of

throw new LogAsErrorException("Invalid number of project assemblies '{0}'", string.Join("," + Environment.NewLine, ProjectAssembly.Select(a => a.ItemSpec)));

Copy link
Member Author

@maraf maraf Apr 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'll do it in follow-up PR. I would like to focus on the functional part at the moment

src/tasks/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks/FileHasher.cs Outdated Show resolved Hide resolved
src/mono/nuget/Microsoft.NET.Sdk.WebAssembly.Pack/Microsoft.NET.Sdk.WebAssembly.Pack.pkgproj Outdated Show resolved Hide resolved
src/tasks/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks/BootJsonData.cs Outdated Show resolved Hide resolved
radical
radical reviewed Apr 11, 2023
View reviewed changes
...o/nuget/Microsoft.NET.Sdk.WebAssembly.Pack/build/Microsoft.NET.Sdk.WebAssembly.Browser.props
Comment on lines +14 to +15
<!-- Avoid having the rid show up in output paths -->
<AppendRuntimeIdentifierToOutputPath>false</AppendRuntimeIdentifierToOutputPath>
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's taken from blazor. I would like to be as similar as possible

@maraf
Copy link
Member Author

maraf commented Apr 12, 2023

Just curious - why do we have Pack in the name?

The Microsoft.NET.Sdk.WebAssembly is taken by tasks in the SDK repo https://github.com/dotnet/sdk/blob/767d13a675a2b03f22d36c1ae62a242cc9523603/src/WasmSdk/Tasks/Microsoft.NET.Sdk.WebAssembly.Tasks.csproj.

Just some comments at a glance. I'm guessing that most of the code is being moved from elsewhere (could you point to the original sources?), so I didn't really review in detail.

This is the original PR dotnet/sdk#31154 that split common bits from Blazor SDK

@maraf
Copy link
Member Author

maraf commented Apr 12, 2023

Nupkg built on CI is correct

@lewing @steveisok @radical @pavelsavara Any more feedback? This is needed to unblock SDK change dotnet/sdk#31519

@maraf maraf requested review from radical and lewing April 12, 2023 09:26
steveisok
steveisok approved these changes Apr 12, 2023
View reviewed changes
Copy link
Member

@steveisok steveisok 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!

@maraf maraf merged commit 063adfe into dotnet:main Apr 12, 2023
@maraf maraf deleted the WasmSdkPackaged branch April 12, 2023 15:38
@sbomer sbomer mentioned this pull request Apr 12, 2023
Closed
lewing added a commit that referenced this pull request Apr 13, 2023
@lewing
Revert "[browser] Wasm SDK packed as a nuget package (#84082)"
a3a0ea5 
This reverts commit 063adfe.
@lewing lewing mentioned this pull request Apr 13, 2023
Merged
ViktorHofer pushed a commit that referenced this pull request Apr 13, 2023
@lewing
Revert "[browser] Wasm SDK packed as a nuget package (#84082)" (#84743)
d0a287a 
This reverts commit 063adfe.
maraf added a commit to maraf/runtime that referenced this pull request Apr 13, 2023
@maraf
[browser] Wasm SDK packed as a nuget package (dotnet#84082)
3fc842b
@maraf maraf mentioned this pull request Apr 13, 2023
Merged
lewing pushed a commit that referenced this pull request Apr 14, 2023
@maraf
[browser] Wasm SDK packed as a nuget package (#84761)
27b8a24 
* [browser] Wasm SDK packed as a nuget package (#84082)

* Update pattern for matching RidAgnosticNupkgToPublishFile to include Microsoft.NET.Sdk.WebAssembly.Pack
@ghost ghost locked as resolved and limited conversation to collaborators May 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@steveisok steveisok steveisok approved these changes

@pavelsavara pavelsavara pavelsavara approved these changes

@lewing lewing lewing approved these changes

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

@radical radical Awaiting requested review from radical

Assignees

@maraf maraf

Labels
arch-wasm WebAssembly architecture area-Build-mono
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@maraf @lewing @radical @pavelsavara @steveisok