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

[workload] Fix the SDK band version and base the WBT on the sdk flow #107899

Closed
wants to merge 17 commits into from

Conversation

lewing
Copy link
Member

@lewing lewing commented Sep 16, 2024

No description provided.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Sep 16, 2024
@akoeplinger
Copy link
Member

hmm I'm not sure this works without some of the other changes from the NET10 PR

@lewing
Copy link
Member Author

lewing commented Sep 17, 2024

hmm I'm not sure this works without some of the other changes from the NET10 PR

It doesn't work with them either, I was trying to illustrate the problem in dotnet/sdk#43070 to @marcpopMSFT

@lewing lewing changed the title Fix the workload band version [workload] Fake the workload band version until we have a proper net10 sdk Sep 17, 2024
@lewing
Copy link
Member Author

lewing commented Sep 17, 2024

hmm I'm not sure this works without some of the other changes from the NET10 PR

It doesn't work with them either, I was trying to illustrate the problem in dotnet/sdk#43070 to @marcpopMSFT

Just to make it more clear the SDK band version we're constructing right now in main is 9.0.100-alpha.1 which is very wrong, but using 10.0.100-alpha.1 won't work properly in a 9.0 sdk (even if we alter wbt to work). This PR now changes it to 9.0.0-rc.1 which should work here and will probably work in the SDK, at least sort of.

We might be able to reenable the multiple band logic and produce a 10.0.100-alpha.1 and a 9.0.100-rc.1 until the flow resolves? I want to make it clear again that putting the band version the package name was a bad choice and complicates all of this.

@marcpopMSFT
Copy link
Member

Why can't you make it 10.0.0-alpha.1 like it's supposed to be? We should probably have already updated the stage0 SDK in the sdk PR to a 10.0.100 version which would make the workloads from runtime work, wouldn't it? Any reason we can't update stage zero sdk there?

@lewing
Copy link
Member Author

lewing commented Sep 17, 2024

Why can't you make it 10.0.0-alpha.1 like it's supposed to be? We should probably have already updated the stage0 SDK in the sdk PR to a 10.0.100 version which would make the workloads from runtime work, wouldn't it? Any reason we can't update stage zero sdk there?

Trying again with a recent 10.0.100 sdk

@lewing lewing changed the title [workload] Fake the workload band version until we have a proper net10 sdk [workload] Fix the SDK band version and base the WBT on the sdk flow Sep 17, 2024
@lewing
Copy link
Member Author

lewing commented Sep 17, 2024

Why can't you make it 10.0.0-alpha.1 like it's supposed to be? We should probably have already updated the stage0 SDK in the sdk PR to a 10.0.100 version which would make the workloads from runtime work, wouldn't it? Any reason we can't update stage zero sdk there?

Trying again with a recent 10.0.100 sdk

@marcpopMSFT current failure is that the alpha.2 SDK only has workload sets which breaks our test setup

@marcpopMSFT
Copy link
Member

marcpopMSFT commented Sep 17, 2024

@marcpopMSFT current failure is that the alpha.2 SDK only has workload sets which breaks our test setup

It has workload sets? You mean the baseline workload set is breaking the test setup somehow? CC @dsplaisted if that's the case to see if he has ideas on how we reversion in the workload set world.

@lewing
Copy link
Member Author

lewing commented Sep 17, 2024

@marcpopMSFT current failure is that the alpha.2 SDK only has workload sets which breaks our test setup

It has workload sets? You mean the baseline workload set is breaking the test setup somehow? CC @dsplaisted if that's the case to see if he has ideas on how we reversion in the workload set world.

So far it was just expecting a directory that wasn't there. Hopefully I was just overly pessimistic, will see if I can get it working here.

@marcpopMSFT
Copy link
Member

Any luck? What folder? Do we have an option of having wasm workloads broken for a short time in sdk so we can get the rest of the repo and net10 work working?

@lewing
Copy link
Member Author

lewing commented Sep 18, 2024

Any luck? What folder? Do we have an option of having wasm workloads broken for a short time in sdk so we can get the rest of the repo and net10 work working?

I had to flow some updates from emsdk which is not fast (see dotnet/dnceng#4111 ) but I'm trying again now

@kasperk81
Copy link
Contributor

I want to make it clear again that putting the band version the package name was a bad choice and complicates all of this.

too late to change it? if it's going to be a continuous pain, perhaps take a hit now?

@lewing lewing requested a review from radical September 19, 2024 18:30
<!-- Required for running apps built with 9.0 sdk, but the sdk does
not yet support *running* with 9.0 sdk -->
<AdditionalSharedFrameworkToInstallArguments Include="-Version latest -Channel 8.0 -Quality daily" />
<!-- Required for running apps built with 10.0 sdk, but the sdk does
Copy link
Member

Choose a reason for hiding this comment

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

Changes in this file look good.

Comment on lines +291 to +296
if (!Directory.Exists(outputDir))
{
Log.LogMessage($"Could not find {name} directory at {outputDir}. Creating it..");
Directory.CreateDirectory(outputDir);
}

Copy link
Member

Choose a reason for hiding this comment

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

Changes in this file look good.

Copy link
Member

@radical radical left a comment

Choose a reason for hiding this comment

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

The workload task+installation specific changes LGTM 👍

@lewing
Copy link
Member Author

lewing commented Sep 20, 2024

I've merged what was remaining of this into #106599 which is farther along now

@am11 am11 added area-Infrastructure and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Sep 21, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

@lewing lewing closed this Sep 25, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Oct 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants