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

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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 15 commits into from
+31 −20

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
@lewing lewing mentioned this pull request Sep 16, 2024
Open
@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 •
edited
Loading

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 •
edited
Loading

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?

@build-analysis build-analysis bot mentioned this pull request Sep 17, 2024
Open
3 tasks
@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 •
edited by lewing
Loading

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

This was referenced Sep 17, 2024
Open
Open
Open
Open
@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

@lewing lewing mentioned this pull request Sep 18, 2024
Open
3 tasks
This was referenced Sep 19, 2024
Open
Open
@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
radical
radical reviewed Sep 19, 2024
View reviewed changes
eng/testing/workloads-testing.targets
<!-- 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.

radical
radical reviewed Sep 19, 2024
View reviewed changes
src/tasks/WorkloadBuildTasks/InstallWorkloadFromArtifacts.cs
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.

radical
radical approved these changes Sep 19, 2024
View reviewed changes
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 👍

@build-analysis build-analysis bot mentioned this pull request Sep 19, 2024
Open
3 tasks
@lewing
Copy link
Member Author

lewing commented Sep 20, 2024 •
edited
Loading

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@radical radical radical approved these changes

@steveisok steveisok steveisok approved these changes

@akoeplinger akoeplinger Awaiting requested review from akoeplinger akoeplinger is a code owner

@marcpopMSFT marcpopMSFT Awaiting requested review from marcpopMSFT

Assignees

@lewing lewing

Labels
needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lewing @akoeplinger @marcpopMSFT @kasperk81 @radical @steveisok