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] Read messages from binlog if process output is missing build finished message #107280

Merged

Conversation

maraf
Copy link
Member

@maraf maraf commented Sep 3, 2024

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
@maraf
Copy link
Member Author

maraf commented Oct 2, 2024

/azp run runtime

Copy link

Pull request contains merge conflicts.

@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
Copy link
Contributor

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

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...

outputBuilder.AppendLine(m.Title);
});

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

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

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

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.

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
24 checks passed
@maraf maraf deleted the WbtDebugMissingMicrosoftNetCoreAppRuntimePackDir branch October 8, 2024 12:16
@github-actions github-actions bot locked and limited conversation to collaborators Nov 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Infrastructure-mono test-enhancement Improvements of test source code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants