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][mt] Fix package microsoft.netcore.app.runtime.mono.multithread.browser-wasm is not found #98083

Merged
merged 3 commits into from
Feb 7, 2024
Merged

[browser][mt] Fix package microsoft.netcore.app.runtime.mono.multithread.browser-wasm is not found #98083

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
61a3792
Invert the logic: prepare MT nuget for ST and ST nuget for MT.
ilonatommy Feb 7, 2024
2202498
We have only 2 runtimes now.
ilonatommy Feb 7, 2024
6fc38a7
Added clarification
ilonatommy Feb 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions eng/testing/workloads-browser.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,10 @@
<Target Name="_GetRuntimePackNuGetsToBuild" Condition="'$(WasmSkipMissingRuntimePackBuild)' != 'true'" Returns="@(_NuGetsToBuild)">
<Error Condition="'$(RIDForWorkload)' == ''" Text="$(RIDForWorkload) is unset" />
<PropertyGroup>
<_BuildVariant Condition="'$(WasmEnableThreads)' == 'true'">multithread</_BuildVariant>
<_Descriptor Condition="'$(WasmEnableThreads)' == 'true'">runtime pack for $(_BuildVariant)</_Descriptor>
<_Descriptor Condition="'$(WasmEnableThreads)' != 'true'">single threaded runtime pack</_Descriptor>
<_IsMTNugetMissing Condition="'$(WasmEnableThreads)' != 'true'">true</_IsMTNugetMissing>
<_BuildVariant Condition="'$(_IsMTNugetMissing)' == 'true'">multithread</_BuildVariant>
<_Descriptor Condition="'$(_IsMTNugetMissing)' == 'true'">runtime pack for $(_BuildVariant)</_Descriptor>
<_Descriptor Condition="'$(_IsMTNugetMissing)' != 'true'">single threaded runtime pack</_Descriptor>
<_DefaultRuntimePackNuGetPath>$([System.String]::Join('.',
$(LibrariesShippingPackagesDir)Microsoft.NETCore.App.Runtime.Mono,
$(_BuildVariant),
Expand All @@ -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)"
Copy link
Member

@pavelsavara pavelsavara Feb 7, 2024 •
edited
Loading

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 ?

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member

@maraf maraf Feb 7, 2024 •
edited
Loading

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

Copy link
Member

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

Copy link
Member

Choose a reason for hiding this comment

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

Hehe, comment already fixed :)

Copy link
Member

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

Copy link
Member Author

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

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. Moving the comment closer to _NuGetsToBuild. We can keep the original one at the beginning of the Task.
  2. 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."

Copy link
Member

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"?

Descriptor="$(_Descriptor)"/>
</ItemGroup>

Expand Down
Loading