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

Generate full platform manifest in official build #1594

Merged
merged 3 commits into from
Jan 10, 2020

Conversation

dagood
Copy link
Member

@dagood dagood commented Jan 9, 2020

Generates a full platform manifest in the official build by downloading all platforms' artifacts from the CoreCLR and Libraries jobs for each Installer job. This is how it's always worked in Core-Setup, it's just easier now to see now how wasteful it is. 🙂

For #1362. Should unblock dotnet/extensions and dotnet/toolset uptake.

Completed mock build for testing: https://dev.azure.com/dnceng/internal/_build/results?buildId=478304&view=results

Example result platform manifest: https://gist.github.com/dagood/378729d986f6d68d6e7dc310658ec321

This PR also removes the no longer built rhel.6-x64 RID from NETCoreApp. Otherwise, the platform manifest infra tries to find it and fails.


A few bad bits of this:

  • It downloads a decent amount more data during the build. This hasn't added more than 3 or so minutes in my testing builds, but could be a problem at some point.
  • There's a redundant download: each Installer job downloads one of the platform's artifacts twice. Once to use to create the runtime being built on that machine, again specifically for platform manifest harvesting.
  • The full manifest infra isn't tested in PR validation. The main runtime pipeline doesn't currently include all the platforms necessary to assemble a full platform manifest.
  • It adds 21 build steps to each installer job. 1 to download all the artifacts, 20 to extract each artifact using the built-in unzip step while preserving folder names.
  • It is difficult to run locally. You have to download every artifact manually, unzip/untargz them, and arrange them properly. (Let alone trying to run every local build yourself on a bunch of your own machines!)

I think that we should be careful spending time addressing those. We should use a baseline/template approach to avoid this flow altogether and fix source-build's platform manifest. Implementing it via template throws away any improvements to the harvesting approach. (See #1362.)

@dagood dagood marked this pull request as ready for review January 10, 2020 05:42
@dagood dagood requested a review from a team January 10, 2020 05:42
@safern
Copy link
Member

safern commented Jan 10, 2020

Thanks @dagood for the deep explanation and the fix.

Is it under your plans to optimize the generation in some way? Can't we have a step on each libraries and coreclr build to generate a minimal PlatformManifest.txt, then upload that, and just download the .txt file from each step (or consume it if doing it locally) and merge all the Manifest.txt files into one?

@dagood
Copy link
Member Author

dagood commented Jan 10, 2020

Is it under your plans to optimize the generation in some way? Can't we have a step [...]

I don't plan to do any build time optimization, the platform manifest is still broken under source-build and I don't think it's worthwhile to create small-scale optimizations that will need to be thrown away (as far as we know at this point). It also doesn't really look like the steps take much time, so this kind of thing would save a few minutes on a >3hr build. That's what I mean about being careful in this bit:

I think that we should be careful spending time addressing those. We should use a baseline/template approach to avoid this flow altogether and fix source-build's platform manifest. Implementing it via template throws away any improvements to the harvesting approach.

@safern
Copy link
Member

safern commented Jan 10, 2020

Right, but with a simpler approach we could definitely guard it on PR builds, no?

@dagood
Copy link
Member Author

dagood commented Jan 10, 2020

I don't see a significant relation there, what we need to run it in PR validation is to have the full list of platforms:

  • The full manifest infra isn't tested in PR validation. The main runtime pipeline doesn't currently include all the platforms necessary to assemble a full platform manifest.

@dagood
Copy link
Member Author

dagood commented Jan 10, 2020

a simpler approach

Your suggestion is good to reduce the amount of data being uploaded/downloaded, but I would argue it is significantly more complex because we have to create a format to store the partial data, add infra to generate/upload/download it, and augment the platform manifest generation code (in Arcade) to be able to merge it in.

@dagood
Copy link
Member Author

dagood commented Jan 10, 2020

PR validation failures are an infra issue happening across most PRs now: https://github.com/dotnet/core-eng/issues/8591. I think the risk if we merge this anyway is very low: the Helix runs are before Installer does anything, and the impactful liveBuilds.targets changes only activate when an installer-specific property is passed by the official build.

Review appreciated. 😄

@safern
Copy link
Member

safern commented Jan 10, 2020

Your suggestion is good to reduce the amount of data being uploaded/downloaded, but I would argue it is significantly more complex because we have to create a format to store the partial data, add infra to generate/upload/download it, and augment the platform manifest generation code (in Arcade) to be able to merge it in.

Makes sense, but let's keep track of discussing this and probably improving (could be part of fixing it for source-build).

<LibrariesArtifactsPath Condition="'$(LibrariesArtifactsPath)' == ''">$([MSBuild]::NormalizeDirectory('$(RepoRoot)', 'artifacts'))</LibrariesArtifactsPath>
<LibrariesAllConfigurationsArtifactsPath Condition="'$(LibrariesAllConfigurationsArtifactsPath)' == ''">$([MSBuild]::NormalizeDirectory('$(RepoRoot)', 'artifacts'))</LibrariesAllConfigurationsArtifactsPath>
Copy link
Member

Choose a reason for hiding this comment

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

Should we also normalize the all configurations path if it is passed down?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't need to split this prop into LibrariesAllConfigurationsOverridePath and LibrariesAllConfigurationsArtifactsPath, because we don't have multiple AllConfigurations runs and the platform manifest doesn't need AllConfigurations artifacts anyway.

For readability though, doing this here too makes sense. (Removes the need to ask "why are there Override props for these two artifact paths and not this other one?") Filed #1615 for that. (Avoids respinning CI.)

@dagood
Copy link
Member Author

dagood commented Jan 10, 2020

Filed #1616 for the list of known badness, and #1615 for LibrariesAllConfigurationsOverridePath which I guess I'll submit a PR for now. (#1620)

@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants