Skip to content

[wasm] Read messages from binlog if process output is missing build finished message#107280

Merged
maraf merged 17 commits intodotnet:mainfrom
maraf:WbtDebugMissingMicrosoftNetCoreAppRuntimePackDir
Oct 8, 2024
Merged

[wasm] Read messages from binlog if process output is missing build finished message#107280
maraf merged 17 commits intodotnet:mainfrom
maraf:WbtDebugMissingMicrosoftNetCoreAppRuntimePackDir

Conversation

@maraf
Copy link
Member

@maraf maraf commented Sep 3, 2024 •
edited
Loading

Mitigates #106160

@maraf maraf added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) arch-wasm WebAssembly architecture area-Build-mono labels Sep 3, 2024
@maraf maraf self-assigned this Sep 3, 2024
This was referenced Oct 1, 2024
Closed
Closed
@maraf
Copy link
Member Author

maraf commented Oct 2, 2024

/azp run runtime

@azure-pipelines
Copy link

azure-pipelines bot commented Oct 2, 2024

Pull request contains merge conflicts.

@maraf
Copy link
Member Author

maraf commented Oct 3, 2024

This trick with replacing text process output by messages from binlog works https://helixre107v0xdcypoyl9e7f.blob.core.windows.net/dotnet-runtime-refs-pull-107280-merge-d302933e6fae4921a6/Workloads-NoWebcil-ST-Wasm.Build.Tests.TestAppScenarios.SatelliteLoadingTests/1/console.e29afe23.log?helixlogtype=result

@maraf maraf changed the title [wasm] Debug missing MicrosoftNetCoreAppRuntimePackDir [wasm] Read messages from binlog if process output is missing build finished message Oct 3, 2024
@maraf maraf added test-enhancement Improvements of test source code area-Infrastructure-mono and removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) area-Build-mono labels Oct 3, 2024
@maraf maraf marked this pull request as ready for review October 3, 2024 11:58
@maraf maraf requested a review from ilonatommy as a code owner October 3, 2024 11:58
@dotnet-policy-service
Copy link
Contributor

dotnet-policy-service bot commented Oct 3, 2024

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

@ilonatommy
Copy link
Member

ilonatommy commented Oct 3, 2024 •
edited
Loading

What are these failures? They do not look connected with the change but I cannot find an active issue. And there's a lot of them

Assert.Contains() Failure: Sub-string not found
      String:    "Property reassignment: $(MSBuildProjectEx"···
      Not found: "error : Stopping the build"
      Stack Trace:
        /_/src/mono/wasm/Wasm.Build.Tests/Blazor/WorkloadRequiredTests.cs(177,0): at Wasm.Build.Tests.Blazor.WorkloadRequiredTests.CheckWorkloadRequired(String config, String extraProperties, Boolean workloadNeeded, Boolean publish)
        /_/src/mono/wasm/Wasm.Build.Tests/Blazor/WorkloadRequiredTests.cs(62,0): at Wasm.Build.Tests.Blazor.WorkloadRequiredTests.WorkloadRequiredForPublish(String config, String extraProperties, Boolean workloadNeeded)
           at InvokeStub_WorkloadRequiredTests.WorkloadRequiredForPublish(Object, Span`1)
           at System.Reflection.MethodBaseInvoker.InvokeWithFewArgs(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)

@maraf
Copy link
Member Author

maraf commented Oct 3, 2024

I think it's connected. Probably the build fails there with a different message than I'm expecting...

This was referenced Oct 3, 2024
Closed
Closed
Closed
ilonatommy
ilonatommy reviewed Oct 4, 2024
View reviewed changes
src/mono/wasm/Wasm.Build.Tests/BuildTestBase.cs
outputBuilder.AppendLine(m.Title);
});

res = new CommandResult(res.StartInfo, res.ExitCode, outputBuilder.ToString());
Copy link
Member

@ilonatommy ilonatommy Oct 4, 2024

Choose a reason for hiding this comment

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

Didn't you overwrite the output that is normally returned by this method? We are failing in

runtime/src/mono/wasm/Wasm.Build.Tests/Blazor/WorkloadRequiredTests.cs

Line 171 in 2c5b609

Assert.Contains("following workloads must be installed: wasm-tools", result.Output);

so it looks like it's looking for a line that should be there but is not. Can't we append to the existing output when we get res? And merge that res.Output with the previous values from line 174?

Copy link
Member Author

@maraf maraf Oct 4, 2024

Choose a reason for hiding this comment

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

Normally it would mean that get the output doubled, which might be... weird. My goal (atm) is to override the output only when the process output is incomplete.

ilonatommy
ilonatommy approved these changes Oct 7, 2024
View reviewed changes
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.

Should be fine now

@maraf maraf merged commit 57feb11 into dotnet:main Oct 8, 2024
@maraf maraf deleted the WbtDebugMissingMicrosoftNetCoreAppRuntimePackDir branch October 8, 2024 12:16
@ilonatommy ilonatommy mentioned this pull request Nov 7, 2024
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Nov 8, 2024
@maraf
Copy link
Member Author

maraf commented Apr 10, 2025

/backport to 9.0-staging

@github-actions github-actions bot unlocked this conversation Apr 10, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Apr 10, 2025

Started backporting to 9.0-staging: https://github.com/dotnet/runtime/actions/runs/14373929584

@github-actions
Copy link
Contributor

github-actions bot commented Apr 10, 2025

@maraf an error occurred while backporting to "9.0-staging", please check the run log for details!

The process '/usr/bin/git' failed with exit code 1

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ilonatommy ilonatommy ilonatommy approved these changes

Assignees

@maraf maraf

Labels

arch-wasm WebAssembly architecture area-Infrastructure-mono test-enhancement Improvements of test source code

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@maraf @ilonatommy