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

Build the SourceBuilt tarball in the publish job and include the merged vertical manifest in it. #19433

Merged
merged 8 commits into from
Apr 15, 2024

Conversation

jkoritzinsky
Copy link
Member

This (in combination with #19389) will allow us to rely exclusively on asset manifests for Versions.props inputs instead of grepping NuGet packages.

Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

I don't understand why we need to move these targets from installer.proj into publish.proj. Can you please provide some more information on that?

@jkoritzinsky
Copy link
Member Author

We produce the merged manifest in publish.proj, which in my understanding is after installer.proj, so we need to move the production of the tarball to publish.proj so we have the merged manifest available.

@ViktorHofer
Copy link
Member

ViktorHofer commented Apr 12, 2024

so we need to move the production of the tarball to publish.proj so we have the merged manifest available.

Still confused. How is the generation of the Private.SourceBuilt.Artifacts archive connected to the creation of the merged manifest?

@jkoritzinsky
Copy link
Member Author

This PR is putting the merged manifest in the Private.SourceBuilt.Artifacts archive (so in the future I can consume that instead of globbing the packages extracted from the tarball).

@ViktorHofer
Copy link
Member

I see. I think we should put the creation of the Private.SourceBuilt.Artifacts archive into a separate .proj (in the eng folder next to the other ones) and make it depend on the publish.proj via a P2P. We should also rename publish.proj to something like merge-manifests.proj. We can then add that new proj to build.proj and run it after the repo builds.

@jkoritzinsky
Copy link
Member Author

I've moved most of the logic into the already-existing finish-source-only.proj project as that one already produces other tarballs for source-build and is SB-only already. I didn't change the name for publish.proj to limit the changes.

@jkoritzinsky jkoritzinsky marked this pull request as ready for review April 12, 2024 23:34
@jkoritzinsky jkoritzinsky requested review from a team as code owners April 12, 2024 23:34
Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

We discussed in a previous meeting that we want to use the "Request changes" action more often for VMR changes. Here it's just my comment around defining a normal P2P without an extra entry point target that needs to be resolved, otherwise looks great.

And I agree that using the finish-source-only.proj file makes more sense.

src/SourceBuild/content/eng/finish-source-only.proj Outdated Show resolved Hide resolved
Lines="@(VersionFileContent)"
Overwrite="true" />

<!-- Copy the merged asset manifest into the tarball -->
Copy link
Member

Choose a reason for hiding this comment

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

Do you see the manifest replacing the PackageVersions.props entirely at some point? Or another way, do you see the properties removed from the PackageVersions.props that come from the merge manifest?

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 could definitely see us only using the manifest. We'd have to make some changes to the bootstrapping project to handle the ExtraPackageVersionPropsPackageInfo items below itself to avoid breaks.

I have another branch that moves the WritePackageVersionProps below to be based off the vertical manifest and changes the consumption of the previously-source-built assets (when generating Versions.props) to use the asset manifest (which obviously needs to wait for the next rebaselining).

@jkoritzinsky jkoritzinsky enabled auto-merge (squash) April 15, 2024 18:25
@jkoritzinsky jkoritzinsky merged commit eed5184 into dotnet:main Apr 15, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants