-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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][workload] Move emscripten to its own manifest for versioning reasons #54349
Conversation
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
@@ -1,5 +1,8 @@ | |||
{ | |||
"version": "${WorkloadVersion}", | |||
"depends-on": { | |||
"Microsoft.NET.Workload.Emscripten": "${EmscriptenVersion}" |
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.
I think this is fine, but we would need to stand up the same setup tasks we're planning for dotnet/runtime in dotnet/emsdk to produce installers and generate VS authoring
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.
If it isn't required we don't need to do it but It sounds like we'll have the versioning problems otherwise?
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.
No, I think you're right. Let say EMSDK produces 6.0.0-preview7.123 and that flows to runtime. Runtime produces preview7.456 and we build MSIs for these and wrap them in nuget packages and push them.
But now runtime rebuilds for something else and produces 457. EMSDK hasn't changed, but we rebuild the closure to make sure everything is in sync for VS and CLI based installs. We'd end up trying to push the 123 package again which is already on the feed.
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.
One thing that isn't clear to me here is should this name include the .Manifest-6.0.100 band or not? I assume so but I'm not sure what that implies about or later style band versioning
cc @dsplaisted
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.
No, the workload IDs in the workload manifest do not include band numbers. The NuGet packages that are created will.
b4056e2
to
dfb8adf
Compare
No need to list them all (thank goodness)
Emsdk side dotnet/emsdk#29