[browser] Fix ConvertDllsToWebCil stack dump on bad input to be error message with input path #101162

danmoseley · 2024-04-17T04:52:46Z

It looks like there are no unit tests for this assembly. Should we have them? It is exercised as a whole as part of all the Wasm.Build.Tests.

View diff with whitespace disabled: https://github.com/dotnet/runtime/pull/101162/files?diff=split&w=1

danmoseley · 2024-04-17T06:07:32Z

Added a basic test. It has to be run manually right now, but it's a start and improvement over none.

lewing · 2024-04-17T19:27:32Z

The test assemblies are changing the prebuilts

danmoseley · 2024-04-18T02:19:18Z

The test assemblies are changing the prebuilts

Not sure what that means. Is there an issue with this PR?

lewing · 2024-04-19T14:44:15Z

The test assemblies are changing the prebuilts

Not sure what that means. Is there an issue with this PR?

I was referring to the failure of runtime-dev-innerloop (Build Source-Build (Linux_x64)) lane.

.packages/microsoft.dotnet.arcade.sdk/9.0.0-beta.24217.1/tools/SourceBuild/AfterSourceBuild.proj(81,5): error : (NETCORE_ENGINEERING_TELEMETRY=AfterSourceBuild) 14 new pre-builts discovered! Detailed usage report can be found at /__w/1/s/artifacts/sb/prebuilt-report/baseline-comparison.xml.
See https://aka.ms/dotnet/prebuilts for guidance on what pre-builts are and how to eliminate them.
Package IDs are:
Castle.Core.5.1.1
Microsoft.CodeCoverage.17.4.0-preview-20220707-01
Microsoft.NET.Test.Sdk.17.4.0-preview-20220707-01
Microsoft.TestPlatform.ObjectModel.17.4.0-preview-20220707-01
Microsoft.TestPlatform.TestHost.17.4.0-preview-20220707-01
Moq.4.18.4
System.Diagnostics.EventLog.6.0.0
xunit.analyzers.1.4.0
xunit.assert.2.6.7-pre.5
xunit.core.2.6.7-pre.5
xunit.extensibility.core.2.6.7-pre.5
xunit.extensibility.execution.2.6.7-pre.5
xunit.runner.console.2.6.7-pre.5
xunit.runner.visualstudio.2.5.7-pre.8

ViktorHofer · 2024-04-24T13:02:40Z

src/tasks/tests/tests.sln

@@ -0,0 +1,31 @@
+


Please don't check in solution files unless they are necessary.

Generally folks will want both tests and impl projects open in VS. what's the recommended way to do that - create a solution manually?

ViktorHofer · 2024-04-24T13:05:08Z

src/tasks/tests/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks.Tests.csproj

@@ -0,0 +1,18 @@
+<Project Sdk="Microsoft.NET.Sdk">


If we do this, then I would move the src under src/tasks/src same as we do libraries under "src/libraries".

ViktorHofer · 2024-04-24T13:06:59Z

src/tasks/tests/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks.Tests.csproj

+  <ItemGroup>
+    <PackageReference Include="Microsoft.Build.Utilities.Core" Version="$(MicrosoftBuildFrameworkVersion)" />
+    <PackageReference Include="Moq" Version="$(MoqVersion)" />
+  </ItemGroup>


Because of this, this project gets always built, even in an official build which doesn't run tests. We want to minimize the external dependencies that we restore and use during an official build.

I think tasks.proj should be changed so that task tests are only built when requested.

What's the right way to do that?

You would need to define a new subset in Subsets.props: tasks.tests. Then if that subset is passed in you can pass an additional property to tasks.proj:

runtime/Build.proj

Line 15 in 56ee7c5

<MSBuild Projects="$(RepoTasksDir)tasks.proj"

i.e.

<Properties Condition="...">IncludeTaskTests=true</Properties>

Then in tasks.proj you would change this line:

runtime/src/tasks/tasks.proj

Line 3 in 56ee7c5

<ProjectReference Include="$(MSBuildThisFileDirectory)**\*.csproj" />

to something like

<ProjectReference Include="$(MSBuildThisFileDirectory)**\*.csproj" /> <ProjectReference Remove="$(MSBuildThisFileDirectory)tests\**\*.csproj" Condition="'$(IncludeTaskTests)' != 'true'" />

ViktorHofer

We don't have test infrastructure support for tasks. We would need to change Subsets.props and/or YML for that. It might be easier to just move the entire task and the tests under src/libraries.

cc @ericstj @jkoritzinsky @akoeplinger

radical · 2024-04-24T17:04:59Z

old related issue - #79010

danmoseley · 2024-04-24T20:42:42Z

I didn't set out to tackle the problem of creating test infrastructure here and probably don't have time. Thoughts about just checking in the fix without the test? Reasoning: none of the others have tests; this test passes when run explicitly.

@lewing ? I can create an issue to create test infra if I do this.

maraf · 2024-04-25T09:48:33Z

I didn't set out to tackle the problem of creating test infrastructure here and probably don't have time. Thoughts about just checking in the fix without the test? Reasoning: none of the others have tests; this test passes when run explicitly.

I sounds good to me.

lewing · 2024-04-26T01:34:39Z

I didn't set out to tackle the problem of creating test infrastructure here and probably don't have time. Thoughts about just checking in the fix without the test? Reasoning: none of the others have tests; this test passes when run explicitly.

@lewing ? I can create an issue to create test infra if I do this.

The fix is fine, and while tests for the tasks feel simple sometimes the things that feel simple are actually harder than their benefit.

This reverts commit 9d15d5d.

danmoseley · 2024-04-30T06:12:29Z

OK, I reverted the tests but left in history in this PR so they can be grabbed in future when someone sets up unit tests for this assembly.

lewing · 2024-05-03T02:52:17Z

merging main again because this should pass now and I'd like to see the results if it doesn't

danmoseley · 2024-05-03T04:03:44Z


[03:45:34] info: Starting:    managed/Invariant.Tests.dll
[03:45:34] info: Error: failed to run main module `dotnet.wasm`
[03:45:34] info: 
[03:45:34] info: Caused by:
[03:45:34] info:     0: failed to invoke command default
[03:45:34] info:     1: error while executing at wasm backtrace:
[03:45:34] info:            0: 0x1084b49 - <unknown>!corlib_System_RuntimeType_CreateInstanceImpl_System_Reflection_BindingFlags_System_Reflection_Binder_object___System_Globalization_CultureInfo
[03:45:34] info:            1: 0x11510e7 - <unknown>!corlib_System_Activator_CreateInstance_System_Type_System_Reflection_BindingFlags_System_Reflection_Binder_object___System_Globalization_CultureInfo_object__
[03:45:34] info:            2: 0x11507de - <unknown>!corlib_System_Activator_CreateInstance_System_Type_object__

lewing · 2024-05-03T04:12:25Z

@ilonatommy please take a look

… message with input path (dotnet#101162)

dotnet-issue-labeler bot added the area-Build-mono label Apr 17, 2024

dotnet-policy-service bot assigned danmoseley Apr 17, 2024

danmoseley force-pushed the fixwebcilex branch from f0a7f3f to ac61c3a Compare April 17, 2024 06:07

danmoseley requested a review from radical April 17, 2024 06:08

danmoseley force-pushed the fixwebcilex branch from 19c0e5c to fafc902 Compare April 17, 2024 06:21

lewing requested a review from maraf April 17, 2024 18:09

lewing approved these changes Apr 17, 2024

View reviewed changes

lewing mentioned this pull request Apr 17, 2024

[wasm] Timeout in System.Text.Json.Tests #101204

Open

maraf approved these changes Apr 19, 2024

View reviewed changes

maraf changed the title ~~Fix ConvertDllsToWebCil stack dump on bad input to be error message with input path~~ [browser] Fix ConvertDllsToWebCil stack dump on bad input to be error message with input path Apr 19, 2024

maraf added arch-wasm WebAssembly architecture os-browser Browser variant of arch-wasm labels Apr 19, 2024

maraf added this to the 9.0.0 milestone Apr 19, 2024

build-analysis bot mentioned this pull request Apr 19, 2024

[wasm] System.Text.Json.Tests running out of memory #98578

Closed

danmoseley closed this Apr 22, 2024

danmoseley reopened this Apr 22, 2024

ViktorHofer reviewed Apr 24, 2024

View reviewed changes

This was referenced Apr 26, 2024

WasmTestOnChrome-System.Runtime.Tests.WorkItemExecution fails with exit code 3221226505 #101464

Closed

Dead lettering tests #101524

Closed

[WASI][AOT] LLVM ERROR: out of memory #101533

Closed

danmoseley added 3 commits April 30, 2024 00:11

Fix ConvertDllsToWebCil message

7d42fec

Add basic tests

9d15d5d

Revert "Add basic tests"

5801429

This reverts commit 9d15d5d.

danmoseley force-pushed the fixwebcilex branch from fedd0f8 to 5801429 Compare April 30, 2024 06:11

Merge branch 'main' into fixwebcilex

4d0a556

Merge branch 'main' into fixwebcilex

44f4e9f

danmoseley merged commit 358b0a4 into dotnet:main May 4, 2024
33 checks passed

michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this pull request May 9, 2024

[browser] Fix ConvertDllsToWebCil stack dump on bad input to be error…

c2fcae6

… message with input path (dotnet#101162)

Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024

[browser] Fix ConvertDllsToWebCil stack dump on bad input to be error…

fd36202

… message with input path (dotnet#101162)

github-actions bot locked and limited conversation to collaborators Jun 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[browser] Fix ConvertDllsToWebCil stack dump on bad input to be error message with input path #101162

[browser] Fix ConvertDllsToWebCil stack dump on bad input to be error message with input path #101162

danmoseley commented Apr 17, 2024 •

edited

Loading

danmoseley commented Apr 17, 2024

lewing commented Apr 17, 2024

danmoseley commented Apr 18, 2024

lewing commented Apr 19, 2024

ViktorHofer Apr 24, 2024 •

edited

Loading

danmoseley Apr 24, 2024

ViktorHofer Apr 24, 2024

ViktorHofer Apr 24, 2024

danmoseley Apr 24, 2024

ViktorHofer Apr 24, 2024 •

edited

Loading

ViktorHofer left a comment

radical commented Apr 24, 2024

danmoseley commented Apr 24, 2024

maraf commented Apr 25, 2024

lewing commented Apr 26, 2024

danmoseley commented Apr 30, 2024

lewing commented May 3, 2024

danmoseley commented May 3, 2024

lewing commented May 3, 2024

[browser] Fix ConvertDllsToWebCil stack dump on bad input to be error message with input path #101162

[browser] Fix ConvertDllsToWebCil stack dump on bad input to be error message with input path #101162

Conversation

danmoseley commented Apr 17, 2024 • edited Loading

danmoseley commented Apr 17, 2024

lewing commented Apr 17, 2024

danmoseley commented Apr 18, 2024

lewing commented Apr 19, 2024

ViktorHofer Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

danmoseley Apr 24, 2024

Choose a reason for hiding this comment

ViktorHofer Apr 24, 2024

Choose a reason for hiding this comment

ViktorHofer Apr 24, 2024

Choose a reason for hiding this comment

danmoseley Apr 24, 2024

Choose a reason for hiding this comment

ViktorHofer Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

ViktorHofer left a comment

Choose a reason for hiding this comment

radical commented Apr 24, 2024

danmoseley commented Apr 24, 2024

maraf commented Apr 25, 2024

lewing commented Apr 26, 2024

danmoseley commented Apr 30, 2024

lewing commented May 3, 2024

danmoseley commented May 3, 2024

lewing commented May 3, 2024

danmoseley commented Apr 17, 2024 •

edited

Loading

ViktorHofer Apr 24, 2024 •

edited

Loading

ViktorHofer Apr 24, 2024 •

edited

Loading