-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Join assets from verticals in the final join point #43627
Join assets from verticals in the final join point #43627
Conversation
We definitely need to filter the verticals list before we merge, since we have some VMR legs we don't intend to publish. We also seem to have some cases where the wrong RID is being used, which is a bit scary, and we're going for a first-come-first-served approach on downloads, so:
That's scary. a) the RID is wrong, it should be linux-musl-x64 (not your fault, that's a VMR bug) b) actually we don't want to use any of the |
I filed dotnet/source-build#4628 for the issue in (a) |
would it make sense to list the verticals to join implicitly as dependencies of the final join job? We were thinking about that option but I was under the impression that all verticals need to be joined and that maintaining the list would be difficult |
src/SourceBuild/content/eng/tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks/JoinVerticals.cs
Outdated
Show resolved
Hide resolved
Perhaps an exclusion works better. We don't want certain verticals, but we want all by default? |
src/SourceBuild/content/eng/tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks/JoinVerticals.cs
Outdated
Show resolved
Hide resolved
src/SourceBuild/content/eng/tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks/JoinVerticals.cs
Outdated
Show resolved
Hide resolved
We may need to use it for both. the join 2nd pass joins will probably have some overlapping artifacts. /cc @directhex |
The main thing #43288 did was allow a given job to depend on multiple other jobs' nupkg output. For example, workload creation requires all component nupkgs to exist on disk, so we need the results of the existing Windows_x64 vertical, but also the new vertical dotnet/source-build#3891 would add. The implementation is done in AzDO tasks, sequentially downloading every nupkg from the named jobs into the join point job's main packages folder. It's not entirely obvious to me how that behaviour would be expected to intersect with this one (e.g. if a vertical creates runtime packs and uploads them, a second vertical creates aot compilers and uploads them, and a third vertical downloads the previous two uploads, adds in more packages, then uploads the full set of both the new packages and the ingested packages from earlier verticals, then the expected behaviour of the final join point isn't clear to me) |
Some subset of the functionality is necessary. If you need Windows x86 and Window x64 as inputs to product a given vertical in pass 2, there is a very good chance that there will be some overlap of artifacts between the input verticals. We need a selection algorithm for that. The output vertical manifest is not the interesting output. The selection of inputs is. |
@mmitche I'm working on checking for duplicate assets in non-primary verticals and it turns out there a lot of them. I ran the task locally on artifacts from https://dev.azure.com/dnceng/internal/_build/results?buildId=2544654&view=results and there is a lot of overlap between So far I'm excluding everything containing "Mono", "Pgo", "Shortstack" and "DevVersions", let me know if that's correct |
Okay super interesting. I'll take a look at the list in a few. I think for now let's print this list as info, pick the first, and open a work item to drive it down. I'm not sure how valid it will be that we run the join job in the normal PR job. Or at least, we may want to exclude some legs entirely from the join since some portion of the legs are for validation purposes and may have expected overlap. They may not represent the official build. For instance, the DevVersions and normal ubuntu legs would be expected to produce the same assets, just at different versions. |
This test run prints a list https://dev.azure.com/dnceng/internal/_build/results?buildId=2547353&view=logs&j=012d8f49-bbe9-5cff-0c2d-eb37469742f3&t=a088a11a-052c-5a48-307b-2470eb67bf36&l=3378 |
} | ||
else | ||
{ | ||
fileItem = matchingFilePaths |
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'm not sure I understand the purpose of this. It seems to be roughly redundant with the code just above https://github.com/dotnet/sdk/pull/43627/files/82c18d04336d56e52f0df8fe7f18e40999216437..717bfcb8325cd132be588c243981e9d30031f53f#diff-0bb02635d68274012f0c2cabbf5fa8e513c3472b0d93e8bd5b1f14a379741fb5R215
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's not redundant, can you add a comment as to its purpose?
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 added a comment, this is for the sdk zip which exists in 2 places in the artifact - for example Windows_x64_Artifacts/assets/Release/dotnet-sdk-*-win-x64.zip
and Windows_x64_Artifacts/assets/Release/Sdk/*/dotnet-sdk-*-win-x64.zip
but it's listed only once in the vertical manifest
I assume it's getting copied somewhere after the manifest creation but couldn't find where
Comparing the full path solves the issue
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.
Gotcha, and my guess is that it's not enough to initially compare with the full path in all cases?
We need to add some tests for this merging code to ensure that the desired selection is being made: dotnet/source-build#4661
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 found some places where Contains
doesn't work - both Microsoft.DotNet.Wpf.GitHub.9.0.0-rc.1.24413.1.nupkg
and runtime.win-x86.Microsoft.DotNet.Wpf.GitHub.9.0.0-rc.1.24413.1.nupkg
contain the same package name
So we'd need to check both that the filename fully matches and compare the path, since we need to check the path only in few cases it made sense to do it this way
Co-authored-by: Viktor Hofer <[email protected]>
Co-authored-by: Viktor Hofer <[email protected]>
Co-authored-by: Viktor Hofer <[email protected]>
I'd like to get #44153 merged in first and then react to that in this PR (should just be 1-2 conflicts). Mainly because testing the join point is very difficult and time consuming. Hope that's OK with everyone. |
Issue: dotnet/source-build#4199
JoinVerticals
task that merges the packages and blobs from all verticals and creates aMergedManifest.xml
JoinVerticals
task after all build passes are completedTest run: https://dev.azure.com/dnceng/internal/_build/results?buildId=2544654&view=results
Files from verticals are downloaded by individual requests to AzDO api; files from the first vertical are copied directly