-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[browser][mt] Fix package microsoft.netcore.app.runtime.mono.multithread.browser-wasm is not found
#98083
Conversation
Tagging subscribers to 'arch-wasm': @lewing Issue DetailsFollow up for #97560. Fixes both problems mentioned in the comments (perf tests and wbt).
The original PR had a misconception that we should create a multithreading nuget for multithreading runtime in
We flip the value of Question:the comment says "3 required runtimes". What is the 3rd one?
|
/azp run runtime-wasm |
Azure Pipelines successfully started running 1 pipeline(s). |
@@ -70,7 +71,7 @@ | |||
<_NuGetsToBuild Include="$(_DefaultRuntimePackNuGetPath)" | |||
Project="$(InstallerProjectRoot)pkg/sfx/Microsoft.NETCore.App/Microsoft.NETCore.App.Runtime.sfxproj" | |||
Dependencies="$(_DefaultRuntimePackNuGetPath)" | |||
Properties="@(_DefaultPropsForNuGetBuild, ';');WasmEnableThreads=$(WasmEnableThreads)" | |||
Properties="@(_DefaultPropsForNuGetBuild, ';');WasmEnableThreads=$(_IsMTNugetMissing)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WasmEnableThreads=!WasmEnableThreads
is a lie. Why do we need it ? Could we do better ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could go back to the original version where we have 2 _NuGetsToBuild
and a condition. One would be run for WasmEnableThreads=true
and pass WasmEnableThreads=false
to the Properties
, the other would do the reverse. However, it's code duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akoeplinger could you please help Ilona to figure this out ?
It's some trick with creating fake nuget, but we don't understand the details.
I worry that this lie will trip-up somebody reading binlog and waste many hours trying to figure it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic is correct and previously there was the similar thing - "build the other flavors". We can put an explanation in a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the comment is already there
For local builds, only one of the 3 required runtime packs might be available. In that case,
build the other nugets with the same runtime but different names.
Maybe we can fix the "3", because there are only "2" currently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hehe, comment already fixed :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment would help a lot. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that there is a comment already (I quote it in the PR description). But the flow is so unintuitive that it did not help me and I made the mistake anyway and then it took me longer than needed to figure out what's wrong. Let's figure out the comment together then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Moving the comment closer to
_NuGetsToBuild
. We can keep the original one at the beginning of the Task. - The comment above
_NuGetsToBuild
:
"We should have both: multithreaded and singlethreaded nugets in local packages. The one that is in line with the runtime type is already there, here we create the missing one."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"We need nugets for all wasm runtime flavors. The one corresponding the current property values is already built, the others needs to be add to the _NuGetsToBuild
"?
/azp run runtime-wasm |
Azure Pipelines successfully started running 1 pipeline(s). |
Thank you! This fixed the issue. |
While this sort of fixed the issue it mentions for me, I still can't build the necessary packs for WBT like I could before, so I can't run WBTs. I get this issue until I build clr+libs subset for linux, when before I didn't need to:
And I can't make this problem go away period, I tried building the mono subset and then built without specifying a subset and it still doesn't work:
Should I file a new issue? |
I looked through the subsets and found an aotcross one, but that doesn't work either. Looks like
EDIT: Manually editing the aot-cross csproj to hard-code browser and wasm just makes the build fail later, it looks like it's trying to do win32 stuff. Maybe I need a windows dev environment?
|
@kg try to look at the commands that
Assuming that
|
That build incantation doesn't work on linux, so it sounds like we're in a windows-only situation right now. |
I think the problem is the makefile targets for WBT, i.e. |
This issue was being fixed and tested on Linux (Codespaces), I did not have such issues. I will try to reproduce rn, maybe something new was changed on main |
The msbuild targets work, it turns out. Only the makefile targets broke. We should probably just get rid of them.
On February 8, 2024 11:53:54 PM PST, Ilona Tomkowicz ***@***.***> wrote:
> That build incantation doesn't work on linux
This issue was being fixed and tested on Linux (Codespaces), I did not have such issues. I will try to reproduce rn, maybe something new was changed on main
--
Reply to this email directly or view it on GitHub:
#98083 (comment)
You are receiving this because you were mentioned.
Message ID: ***@***.***>
- kg (mobile)
|
Follow up for #97560. Fixes both problems mentioned in the comments (perf tests and wbt).
log
The original PR had a misconception that we should create a multithreading nuget for multithreading runtime in
_GetRuntimePackNuGetsToBuild
and same for single thread. However, it's about creating the missing nuget as the comment says:runtime/eng/testing/workloads-browser.targets
Line 44 in 24d149c
We flip the value of
WasmEnableThreads
in the property_IsMTNugetMissing
to achieve that.Question:
the comment says "3 required runtimes". What is the 3rd one?
Answer: it used to be EP runtime, now it does not exist. The comment should be edited.