dotnet / source-build

A repository to track efforts to produce a source tarball of the .NET Core SDK and all its components
MIT License
265 stars 132 forks source link

Improve the UX and guideance around the SourceBuild metadata in the Version.Details.xml #3373

Closed MichaelSimons closed 9 months ago

MichaelSimons commented 1 year ago

There are a couple usability issues around the SourceBuild metadata in the Version.Details.xml that should be addressed.

  1. The SourceBuild metadata is only allowed on one dependency per repo (this is related to https://github.com/dotnet/source-build/issues/2050). When a developer adds multiple, the UX is not intuitive on what is wrong. This is illustrated in https://github.com/dotnet/source-build/issues/3372.
  2. In the past there have been numerous issues at release time when stable versions flow in. When the SourceBuild metadata is placed on a stable versioned dependency this breaks the source-build intermediate loading because the intermediate is non-shipping and usually doesn't get the stable versioning. When this happens the SourceBuild metadata is typically moved to a nonstable versioned dependency.

There are likely several options here. One possible solution is to explicitly add the SourceBuild.Intermediates as dependencies and only mark those as source-build.

mmitche commented 1 year ago

We discussed this in the Unified Build meeting today. Summary:

We should have repos take direct dependencies on the source build intermediate packages. This could take two forms:

After repos completed this work, which is largely mechanical, we would change the SB Version.Details.xml logic to disallow source build elements anywhere else.

This could be completed in parallel with documentation work in https://github.com/dotnet/source-build/issues/3358

ellahathaway commented 9 months ago

Proposal for changes within the scope of this issue:

Introduction:

The purpose of this proposal is to introduce a new element, SourceBuild, to the existing Version.Details.Xml file. This proposal outlines the structure of the element, its integration with Darc, and the rollout plan.

1. Use in Version.Details.xml:

2. Element Details:

3. Example Structure:

<SourceBuild RepoName="source-build-reference-packages" ManagedOnly="true" Version="9.0.0-alpha.1.24072.5"> 
  <Uri>https://github.com/dotnet/source-build-reference-packages</Uri>
  <Sha>2249c9a81ec9b6cd86791c21e6e69fbc07099788</Sha>
</SourceBuild>

4. Integration with Darc:

5. Rollout Plan:

mthalman commented 9 months ago

Location: The SourceBuild elements will be placed at the bottom of the Version.Details.Xml file.

A child of the Dependencies element then? Or within ProductDependencies/ToolsetDependencies?

ManagedOnly: Boolean indicating whether a RID suffix is necessary on the intermediate NuGet package.

I don't know the history of this setting. Frankly I had no idea what it meant. Since we're redesigning the schema, we should update this to a more descriptive name that has better relevancy. Should this just be "IsRidSpecific"? I want the default value to be false and optional. Today we define ManagedOnly="true" all over the place; let's keep it tidy instead.

MichaelSimons commented 9 months ago

One piece I didn't see included is the inclusion of coherency constructs (e.g. CoherentParentDependency="Microsoft.NET.Sdk.WorkloadManifestReader"). It feels like this would be necessary.

This approach is going to be expensive to implement as it is going to require a non-trivial amount of infra changes. This makes it a bit of a hard sell to me given the priority of the other UB work. This is not a criticism that it's a bad approach. I'm just questioning the ROI versus other approaches. @mmitche what are you thoughts?

ellahathaway commented 9 months ago

A child of the Dependencies element then? Or within ProductDependencies/ToolsetDependencies?

Good point. This should've been clarified. I imagine this element acting as if we are "simply taking a dependency on the SB intermediates" but it looks more elegant and is separated from the other dependencies. As such, it would be a child of the "Dependencies" element. Given that some repos have their direct dependencies on SB intermediates in ProductDependencies and other repos have their direct dependencies on SB intermediates in ToolsetDependencies, I think it makes the most sense for this to be separated out from both of those elements for consistency across repos. That said, this does add a layer of complexity to the implementation.

we should update this to a more descriptive name that has better relevancy. Should this just be "IsRidSpecific"?

I agree. I think that "IsRidSpecific" is more self-describing and is a good substitute to the current "IsManagedOnly".

One piece I didn't see included is the inclusion of coherency constructs (e.g. CoherentParentDependency="Microsoft.NET.Sdk.WorkloadManifestReader"). It feels like this would be necessary

The lack of inclusion of this property is likely due to my misunderstanding of it. My understanding of CoherentParentDependency is that it's needed when the repo version and the source-built intermediate package version don't match. I was thus assuming in this case that the CoherentParentDependency could be left on the dependency itself rather than the new SB metadata. I will have to look into this more.

This approach is going to be expensive to implement as it is going to require a non-trivial amount of infra changes. This makes it a bit of a hard sell to me given the priority of the other UB work.

I completely agree. This is definitely an investment. In terms of a cheaper solution, it is always possible to take the intermediates as direct dependencies as @mmitche suggested in a comment above.

mmitche commented 9 months ago

CoherentParentDependency would be required for the SB intermediates. It says that the source build dependency should be taken from another repo's dependency. I'm not totally sure we have a case where it is used in SB, but for non-SB, it's like windowsdeskop saying: "Take the version of winforms that my dependency on wpf has". That ensures dependency compatibility in certain cases.

My general thoughts on this are that we if we are going to make major changes, we should do so with the forward flow/backward source flow for general Unified Build in mind. I would be sure to get some of @premun's input here. The goals would probably be to:

@premun has some initial design here: https://github.com/dotnet/arcade/blob/main/Documentation/UnifiedBuild/VMR-Full-Code-Flow.md#synchronization-configuration

I think in the interim if we wanted to make improvements, the cheap solution is to take explicit dependencies on the source build intermediates. That avoids issues with stable flow. It doesn't solve the first issue, but we could do some minor improvements in the intermediates infra to detect multiple tagged dependencies and error out. That would get caught in most repos' SB legs.

MichaelSimons commented 9 months ago

It doesn't solve the first issue, but we could do some minor improvements in the intermediates infra to detect multiple tagged dependencies and error out. That would get caught in most repos' SB legs.

One of the cheapest ways to fix this UX issue would be to allow the SB metadata on multiple dependencies. The only time this should cause an error is when the dependencies are incoherent (e.g. on different versions/shas).

mmitche commented 9 months ago

It doesn't solve the first issue, but we could do some minor improvements in the intermediates infra to detect multiple tagged dependencies and error out. That would get caught in most repos' SB legs.

One of the cheapest ways to fix this UX issue would be to allow the SB metadata on multiple dependencies. The only time this should cause an error is when the dependencies are incoherent (e.g. on different versions/shas).

While I think this will reduce friction in the short term, during previews, I think this is more likely to cause unexpected errors when we first hit servicing and some repos start incrementally servicing.

premun commented 9 months ago

The new <Source> tag has already been implemented but it's not really used anywhere and can be changed. This just stores the VMR SHA the repository was synchronized from the last time.

<Source Uri="https://github.com/dotnet/dotnet" Sha="86ba5fba7c39323011c2bfc6b713142affc76171" />

To me, this is an unrelated feature to the one proposed by Ella and they can live side by side.

To the UX problem described in the issue:

mmitche commented 9 months ago

The VMR repos may still have coherency attributes, but not for dependencies produced within the VMR.

ellahathaway commented 9 months ago

The cheap solution is to take explicit dependencies on the source build intermediates. Possibly refactoring the XMLs to only have these tags on intermediates is the cheapest best effort thing to do.

The general sentiment seems to be to take explicit dependencies on the intermediates & add the tags on those dependencies. I agree that this is the most cost-effective solution. This work will be coupled with the work I'm currently doing to update the documentation on adding dependencies.

We could do some minor improvements in the intermediates infra to detect multiple tagged dependencies and error out. That would get caught in most repos' SB legs.

I agree with this. I can improve the infra to alert/error when duplicate tags are added. That solves the initial UX problem described in the issue.