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

[workloads] Add prerelease version to workload manifest location #79086

Closed
lewing opened this issue Dec 1, 2022 · 13 comments
Closed

[workloads] Add prerelease version to workload manifest location #79086

lewing opened this issue Dec 1, 2022 · 13 comments
Assignees
Milestone

Comments

@lewing
Copy link
Member

lewing commented Dec 1, 2022

We can avoid problems in the future by making sure prerelease workload manifests don't end up in the stable band directory and we should do this before net8-preview 1

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Dec 1, 2022
@lewing lewing added area-Build-mono and removed untriaged New issue has not been triaged by the area owner labels Dec 1, 2022
@lewing lewing self-assigned this Dec 1, 2022
@lewing lewing added this to the 8.0.0 milestone Dec 1, 2022
@lewing
Copy link
Member Author

lewing commented Dec 1, 2022

@dsplaisted what do we need to do here on the authoring side?

@dsplaisted
Copy link
Member

The version band is already included in the package ID, for example 7.0.100 in Microsoft.NET.Workload.Mono.ToolChain.net6.Manifest-7.0.100. So you should update the package IDs to include the preview band, for example Microsoft.NET.Workload.Mono.ToolChain.net8.Manifest-8.0.100-preview.1.

Does that help?

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Dec 3, 2022
@radical
Copy link
Member

radical commented Dec 7, 2022

The version band is already included in the package ID, for example 7.0.100 in Microsoft.NET.Workload.Mono.ToolChain.net6.Manifest-7.0.100. So you should update the package IDs to include the preview band, for example Microsoft.NET.Workload.Mono.ToolChain.net8.Manifest-8.0.100-preview.1.

Does that help?

This workload depends on Microsoft.NET.Workload.Emscripten.*. Would the manifests for that also be installed with the same sdk-band - 8.0.0-preview.1?

I'm having some trouble with adapting our tests for this. For our Wasm.Build.Tests, we do:

  1. install the latest 8.0 sdk
  2. Install (copy) the locally built Microsoft.NET.Workload.Mono.ToolChain.Manifest*nupkg manifest
  3. Install its dependency (Microsoft.NET.Workload.Emscripten*) manifests by using the same sdk-band, but a different package version.
  4. Install packs using dotnet workload install ...

This process is breaking down for me at (3). For a local build:

a. I get SdkBandVersion=8.0.0-dev (with https://github.com/dotnet/runtime/pull/79184/files#diff-50e91c82311ea26f2a73202525dfdf2b0a89c178ee666b2bf3ad4c84ac4c06dcR96-R97)
b. our msbuild task finds, and installs the Microsoft.NET.Workload.Mono.ToolChain.Manifest-8.0.100-dev.8.0.0-dev* from artifacts
c. The task tries to now find the Emscripten manifest also, and builds up the package name using SdkBandVersion=8.0.100-dev - Microsoft.NET.Workload.Emscripten.net7.Manifest-8.0.100-dev. But that package doesn't exist, for obvious reasons.

What is the expected behavior here? Does the workload resolver expect to find all the manifests in the same sdk-manifest/8.0.100-dev? Should the emscripten manifest be installed to the same band, somehow?

@radical
Copy link
Member

radical commented Dec 9, 2022

@dsplaisted ping

@steveisok
Copy link
Member

@radical if I understand this correctly, I think the band in the sdk-manifest folder path should be 8.0.100. The manifest inside that folder will contain prerelease info in the name.

@dsplaisted
Copy link
Member

@radical if I understand this correctly, I think the band in the sdk-manifest folder path should be 8.0.100. The manifest inside that folder will contain prerelease info in the name.

This is not correct. The folders under the sdk-manifests folder are the version bands, and may include the prerelease specifier.

What is the expected behavior here? Does the workload resolver expect to find all the manifests in the same sdk-manifest/8.0.100-dev? Should the emscripten manifest be installed to the same band, somehow?

The resolver does not need all of the manifests to be in the same band. If a workload is listed in IncludedWorkloadManifests.txt, then the resolver will first look for the manifest in the current band folder, but if it's not found there will fall back through the earlier version bands in the sdk-manifests folder.

The task tries to now find the Emscripten manifest also, and builds up the package name using SdkBandVersion=8.0.100-dev - Microsoft.NET.Workload.Emscripten.net7.Manifest-8.0.100-dev. But that package doesn't exist, for obvious reasons.

Why doesn't the package exist? Is Emscripten using different preview specifiers that runtime is? If so you may have to have some way for the tests to have a way to find the Emscripten band to use.

@radical
Copy link
Member

radical commented Dec 9, 2022

What is the expected behavior here? Does the workload resolver expect to find all the manifests in the same sdk-manifest/8.0.100-dev? Should the emscripten manifest be installed to the same band, somehow?

The resolver does not need all of the manifests to be in the same band. If a workload is listed in IncludedWorkloadManifests.txt, then the resolver will first look for the manifest in the current band folder, but if it's not found there will fall back through the earlier version bands in the sdk-manifests folder.

Ah, that sounds good!

The task tries to now find the Emscripten manifest also, and builds up the package name using SdkBandVersion=8.0.100-dev - Microsoft.NET.Workload.Emscripten.net7.Manifest-8.0.100-dev. But that package doesn't exist, for obvious reasons.

Why doesn't the package exist? Is Emscripten using different preview specifiers that runtime is? If so you may have to have some way for the tests to have a way to find the Emscripten band to use.

@steveisok @lewing I guess, we need to update emsdk to use an updated sdk band first.

@lewing
Copy link
Member Author

lewing commented Jan 5, 2023

The version band is already included in the package ID, for example 7.0.100 in Microsoft.NET.Workload.Mono.ToolChain.net6.Manifest-7.0.100. So you should update the package IDs to include the preview band, for example Microsoft.NET.Workload.Mono.ToolChain.net8.Manifest-8.0.100-preview.1.

Does that help?

aren't we very short on characters here?

@lewing
Copy link
Member Author

lewing commented Jan 13, 2023

@dsplaisted is it the nupkg name that matters or the msi name? @steveisok has a emsdk update that suggests we can update the nupkg name independent of the msi which might allow us to side step the path length problem

cc @joeloff

@dsplaisted
Copy link
Member

It's the nupkg name that matters. I think the MSI name can be pretty much anything (though there may be some assumptions baked into the MSI generation tooling).

@lewing
Copy link
Member Author

lewing commented Jan 13, 2023

I think we can do the prerelease versioning on the nupkg without causing path problems in that case, so we are unblocked with making changes

@joeloff
Copy link
Member

joeloff commented Jan 13, 2023

Yup, the package ID is what's primarily used when it generates the file names. Some level of uniqueness is required because the files end up in a flat container for the VS insertion (zip file) and on AzDo as a published artifact.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 30, 2023
@lewing lewing closed this as completed Jan 31, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants