Microsoft.AspNetCore.App.Ref needs to be produced and versioned correctly in servicing for source-build #39471
Comments
TanayParikh
added
area-runtime
area-infrastructure
|
@wtgodbe - I see this is in |
|
We're going to discuss it at standup today. It looks like the change is needed in 6.0, which means the earliest we could do it is likely 6.0.3 (March patch) as the February builds are already underway. Is it also needed in main? We can do it ASAP if there's an additional time constraint beyond the 6.0 releases |
|
Yes a change is needed in 6.0 and it should be backported to main. We need a fix for 6.0.2 but if you have a fix, we can create a source-build patch for 6.0.2. |
|
@MichaelSimons we're actually servicing the ref pack again in 6.0.2, with version 6.0.2 - will that unblock source build? If so we can have the patch ready for 6.0.3 so that it continues producing the 6.0.2 targeting pack for source build. If not we'll get a fix ASAP and see if we can merge it for 6.0.2 |
|
@wtgodbe - That will work for 6.0.2 but source-built Fedora 6.0.1 was unable to release because of this issue. It would be most helpful if you could provide a fix that we could apply as a source-build patch to unblock. |
|
We're confused by this issue @MichaelSimons. The runtime code just sets a property for the previous version of the targeting pack. Does something else rebuild the targeting pack w/ that version in every servicing build (when doing a source build)❔ In any case, we already have a property w/ the version of the last targeting pack -- It would be confusing to look at a build of 6.0.2 and see a 6.0.0 or 6.0.1 targeting pack. I also worry doing that would mess up the release. Is this exclusively about source-builds❔ I guess the general question is why Fedora and so on need a targeting pack every time. I thought their requirement was strictly runtime components❔ |
Yes - When building source-build this prevents SkipBuild from getting set.
This does not work for source-build because it needs to build the full product. Pre-6.0 source-build was able to handle the targeting packs not getting built during servicing by treating them as ref packs (e.g. adding them to https://github.com/dotnet/source-build-reference-packages). With the introduction of the analyzers, this is no longer an option. I have no objection to your proposal to always release a targeting pack. It feels like this behavior is something that would ideally be the same between the runtime and aspnetcore. cc @safern ^
This is specific to source-build at this time. This is something that would be needed if/when we move towards having reproducible builds. Also this would be the default behavior if/when we get to the point where source-build is the open build and then the MSFT build is a closed build. The last two have no concrete plans as of now.
This is not Fedora specific rather it affects all source-build. I won't go into the details on why this is blocking Fedora and not RedHat here as it doesn't feel pertinent. The Microsoft build works because it is able to pull the previously build targeting pack from nuget. Source-build has to build the entire product stand-alone offline so it can't access artifacts from previous releases. cc @crummel, @omajid - to correct anything I may have mis-stated. |
|
🆗 I can see why building our stuff in the aspnetcore repo against the previous targeting pack would break source-build. Do you specifically need
|
We need #2 |
|
Does it need to match the last stable version on nuget.org, or could we create a new non-stable version each month? I lean towards doing the former if it doesn't break maestro, as the latter would mean upstream repos ingesting new non-stable targeting packs every month. If we do the former we'd only do it in source build (@mmitche would that wind up in the asset manifest/dependency flow?) |
|
The other option is to just always and forever ship Microsoft.AspNetCore.App.Ref.{stable version}.nupkg in every servicing release. This essentially ignores the source-build-specificity of the issue and would be incredibly simple to implement (delete one line). |
I agree "could rectify over time" was annoyingly vague 😺 |
I thought the other runtimes explicitly didn't always ship the ref packs. That was at least the design when we did the ref-pack/runtime-pack split. |
|
Yes, the design spec included this:
However I think with source-generators and analyzers, now ref-packs (at least the .NETCore App one) is very likely to be serviced, at least in the early servicing cycles. When the split design spec was written the ref pack only carried ref assemblies and a bug in a ref assembly is pretty uncommon. Given the new shape of ref packs and the likelihood for them to be serviced, I wouldn't mind servicing them every release. @ericstj might have input on this as well. |
|
Ref packs represent all the runtime-agnostic data for a framework. It's reference assemblies, xml docs, data files that describe the runtime content, and now analyzers and source generators. Agree with @safern that the goal is to never change surface area for a given ref-pack, which is easy to do if you don't ship updates. I don't think however we must unnecessarily constrain this: it's not like ref-packs are some magical things that never have bugs. Even before we added analyzers/generators we've had to service ref packs. Sometimes we want to fix docs, sometimes we got the metadata in the reference assemblies wrong. I'm open to always shipping them, especially if it reduces complexity in servicing. Around GA time frame I believe there was a discussion between @Anipik, @dsplaisted, and @mmitche (and maybe some others) and they concluded to continue to make it opt in. I'm not sure why, perhaps to reduce churn in the SDK? I'd want to make sure the SDK team is involved with whatever decision is made here since they are the most impacted by changes. From the framework author side; I think the risk of accidentally shipping surface area changes is easily mitigated with APICompat automation. Similarly we could add automated testing to ensure data files do not change after GA. |
|
I don't have any objections to always shipping the ref pack from aspnetcore - we already have automation that checks for API changes in servicing. @marcpopMSFT @dsplaisted any objections from the SDK side to everybody always shipping ref packs in 6.0+? @merriemcgaw @RussKie @dreddy-work any objections to doing this in WindowsDesktop? |
|
In the previous discussion, I wanted to avoid rebuilding the ref packs for every servicing release, because I felt that after the first few patches the frequency where there were actually changes would go way down. However, if it's causing issues for source-build I suppose we can switch to producing them every patch. That would also have the benefit of not having to manually manage turning it on and off. |
|
Just recording for the record that I'm not against shipping ref packs every release, just wanted to make sure other stakeholders were on board with the decision. |
|
Who else do we need sign-off from before @wtgodbe or I put up a PR for our part in this❔ |
|
I recall we had an issue last year when we accidentally rebuilt and reshipped the ref pack last year, I cannot recall what the issue was though, maybe @leecow or @joeloff can recall. @dougbu if we do ship these each month going forward then do we revision them to match the release and runtime version, or does the version "stick" at 6.0.0? I think we've had other issues where we've rebuilt products but not revisioned. Another thing to keep in mind is we want to be consistent - if we ship the rebuilt ref pack with the SDK bundle then we also want to insert the same into VS, deliver that via MU, etc. |
|
I suggest we change the version in every release of the ref/ packs e.g. Microsoft.AspNetCore.App.Ref as well as the targeting packs. That's what we did in past releases, when we fixed targeting pack bugs.
Does Microsoft.AspNetCore.App.Ref (and similar) or the targeting packs ever release via MU❔ I'd be surprised if Windows had this content. Of course, VS insertions remain a slight issue but is the delta from today (where at least ASP.NET Core always builds and packs everything with the odd exception of Microsoft.AspNetCore.App.Ref and the targeting packs) a burden or an upside❔ |
|
For fun and frolic 😁, I put up a few options as draft PRs. We can choose between these and decide which should go into a 6.0.1 source-build patch, 6.0.3, and main. My preference would be to scorch the earth and do #39568 everywhere. |
|
Also created a teensy option for 6.0.1: Patch for this option, after squashing the two commits and adding |
|
Proposed way forward:
In the meantime,
I'm not sure which of the options will work because I don't see where Arcade-powered source-build loads patch files and am not sure SourceBuild.props gets loaded in a way that would be visible to other files in the inner build. |
|
I have created a source-build PR with this proposed patch along with a patch for a different but related runtime issue. Fedora can pick up these patches themselves to get unblock for 6.0.1. |
|
I'd like to open #39568 for review and close the rest later today. Please respond w/ "yes, do this" or a more nuanced preference in the next few hours. (The conversation above ends mostly sounds more like "well 🆗" 😃 ). |
yes, do this |
|
Can we please get clear responses from runtime, WindowsDesktop and PM representatives❔ |
|
From dotnet/installer#13009, I understand dotnet/aspnetcore and dotnet/runtime are taking different approaches for the 6.0.1 patch. dotnet/aspnetcore will build all packages and installers (including the targeting packs) when the patched branch is built (whether in a source-build or not) but dotnet/runtime will build all packages only in a source-build. That's probably fine but is an inconsistency we should not carry forward to 6.0.3 and beyond. Let's have one plan for all three repos that produce targeting packs. @MichaelSimons also mentioned WindowsDesktop isn't directly impacted by source-build. Nonetheless, Microsoft.WindowsDesktop.App.Ref should be part of the "6.0.3 and beyond" consistency. |
|
@dougbu - I'll be engineering for WindowsDesktop. |
|
Chiming in here from PM standpoint, the plan is understood and looks good to me. |
|
Sign offs (and a nudge 😉):
|
|
Sorry, we also heard an |
|
I think @ericstj counts for runtime? |
|
/fyi I removed the extra PRs, declared #39568 ready for review, and opened |
|
Yes, let's go forward. @safern also agreed above. |
For the background refer to dotnet/aspnetcore#39471.
Always ship ref packs for servicing releases For the background refer to dotnet/aspnetcore#39471.
For the background refer to dotnet/aspnetcore#39471.
|
@MichaelSimons is there anything left to do here❔ If not, please close as done😺 |
ghost
locked as resolved and limited conversation to collaborators
Currently building release/6.0 for 6.0.1 doesn't produce a Microsoft.AspNetCore.App.Ref package. This is because the package wasn't serviced in 6.0.1. 6.0.0 is the active version yet the build doesn't produce it. This breaks source-build which needs to produce the entire product.
In previous versions, source-build handled this by treating the package as a reference package via https://github.com/dotnet/source-build-reference-packages. This is no longer an option in 6.0 with the introduction of roslyn analyzers (e.g source) in the package.
dotnet/runtime made changes in 6.0 to support this here.
The text was updated successfully, but these errors were encountered: