Remove DCP and Dashboard from the .NET Aspire SDK Workload #4708
Conversation
dotnet-issue-labeler
bot
added
the
area-codeflow
| @@ -0,0 +1,43 @@ | |||
| # Design proposal for the .NET Aspire workload going forward | |||
There was a problem hiding this comment.
For some reason, in the PR review I'm not getting the images loaded. If you want to visualize the doc, you can do so here: https://github.com/joperezr/aspire/blob/workloads/MSBuildSDKs/docs/specs/future-of-workload.md
|
|
||
| ## Proposed solution | ||
|
|
||
| Given the above, we propose to move the DCP and Dashboard components out of the workload, and into NuGet packages that are referenced as dependencies of the Aspire.Hosting.AppHost package. This will allow the AppHost project to reference the version of Aspire.Hosting that it wants, and then the DCP and Dashboard will be installed as dependencies of that version of Aspire.Hosting. This will allow the AppHost project to have full control over the version of the DCP and Dashboard that it wants to use, and will also allow customers to have multiple AppHost projects that reference different versions of Aspire.Hosting without any issues. This will also allow us to update the DCP and Dashboard independently of the workload, which will allow us to ship updates to these components more frequently and with less risk, with a far less complex compatibility matrix. |
There was a problem hiding this comment.
What would happen if we were to remove the Aspire.Hosting.SDK completely, or update it to not rely on being loaded before restore? I understand that today it does some things based on those assumptions, but I'm interested in what in the experience would change if we removed it from the workload completely. There's also the option of a NuGet-based SDK (which I've resisted so far but I think we should explore all options).
There was a problem hiding this comment.
These are the contents of Aspire.Hosting.SDK in question:
aspire/src/Aspire.Hosting.Sdk/SDK/Sdk.targets
Lines 9 to 36 in 88c5c19
The most important part of this that needs to be evaluated before NuGet restore is the bit that is updating ProjectReference items. That code is what tells MSBuild that these aren't real project references, so it shouldn't treat them as such in restore. Without these, you'd have problems like restore graph of the AppHost also restoring packages coming from those project references (which could lift up dependencies, cause incompatible issues when packages are not applicable to some other project type, etc.), and it would also cause failures when those project references are targeting incompatible frameworks (as MSBuild would prevent that from working). For instance, if you had a net8.0 AppHost referencing a net9.0 frontend: this is allowed today with no problems, but wouldn't be if this logic is not an MSBuild SDK any longer. @eerhardt can likely provide many more examples as he was the original one that added this logic and after different attempts found that the only way forward (if we wanted to use ProjectReference item) would be to use MSBuild SDKs.
There are alternatives though, that we can consider:
- We could decide to remove Aspire.Hosting.SDK from the workload, and instead have it be an SDK that is imported from the AppHost project itself (via
<Sdk Name="Aspire.Hosting.Sdk" Version="version" />). The drawback of that is that it breaks existing customers as they need to change their projects, as well as MSBuild SDKs don't have a way to notify if there is an update, so people would likely get stuck on an old version of the SDK for a long time. - We could decide not to use ProjectReference at all, and instead use a different Item for this. This would also break existing customers that would have to redefine how their apphost project references project resources, and would likely also impact some of the gestures that you get (especially in VS) automatically with Project References (e.g. drag and drop projects to automatically reference them,
dotnet add reference ..support, etc.)
Overall, the reason why I picked this as the proposal is that the code in that SDK is very minimal and unlikely to change going forward (proof of it being it hasn't changed since it was added), hence in favor of not breaking existing customers and not regress some of the scenarios, it feels like keeping that single SDK in the workload is a good tradeoff.
There was a problem hiding this comment.
Thanks for the extra detail. I think it's important this proposal has details of all the possible approaches and the trade-offs with each so we can make an informed decision.
The drawback of that is that it breaks existing customers as they need to change their projects, as well as MSBuild SDKs don't have a way to notify if there is an update, so people would likely get stuck on an old version of the SDK for a long time.
I've been very resistant to use NuGet-resolved MSBuild SDKs in the past but the reasons cited here seem like that could be reasonably mitigated with targets in Aspire.Hosting.AppHost that check the SDK and version being used, and if necessary emit a warning advising the user what action to take to bring them up to date. For example, assuming we make the changes in 8.2:
- If a user updates
Aspire.Hosting.AppHostto8.2but doesn't change anything else about the project, the targets within can detect that (IsAspireHost==true,UsingAspireNuGetSDK!=true) and instruct the user to remove theIsAspireHostproperty (which triggers the workload detection today) and change the project SDK to<Project SDK="Aspire.Hosting.SDK/8.2.0">. - Moving forward, if a new version of
Aspire.Hosting.AppHostintroduces a requirement for a new SDK min version, that can be detected by the targets and emit a warning instructing the user to update the SDK reference to the new version.
The initial project change is one-time only, and after that a change to SDK version in the project is only required when needed (likely rarely) and the user can get clear instructions on what to do. As it is today, all new releases require users to perform a workload update anyway, so I think it's reasonable to move to a model where they only have to update the SDK version their projects use when a new SDK is released, and they can get instruction of that directly after they update the package reference for Aspire.Hosting.AppHost.
I'm still keen to keep using ProjectReference for the reasons you cited (i.e. existing tooling gestures just work) which means keeping the requirement of an MSBuild SDK so using NuGet-resolved MSBuild SDKs seems like the approach we should consider (and scrutinize) more closely.
There was a problem hiding this comment.
Just to make sure I'm following, the alternative you are suggesting is different from my proposal in that you want to remove Aspire.Hosting.SDK from the workload, and instead move it to be an SDK reference coming from AppHost project itself, correct?
There was a problem hiding this comment.
Correct. In short, I want to ensure we explore options that remove the need for the workload to be installed at all.
There was a problem hiding this comment.
If the problem you refer to is that they get restored in the UI thread, then I don't think that's fixed. However, the Hosting SDK being just a single targets file with less than 50 lines of code I'm not too concerned. (I'd be much more concerned if we were restoring Dashboard and DCP as MSBuild SDKs which at some point I tried in my prototype)
There was a problem hiding this comment.
RE this
MSBuild SDKs don't have a good update mechanism, meaning if we ever wanted to ship a new update, consumers wouldn't be notified in any way, so they would have to know to go an update their csproj to use the new versions on their own. Having it on the workload, on the other hand, at least will still make it that running workload update will bring the updates, and it also means consumers will get the notification in the console and VS when their workloads have updates.
As I lay out in https://github.com/dotnet/aspire/pull/4708/files#r1657925144 I think we can easily improve this by adding checks in the targets brought in my Aspire.Hosting.AppHost.
There was a problem hiding this comment.
Yup, that's a fair point. I think that in the future if we want to not have a workload at all then having this be an MSBuild SDK would be relatively easy and we can use what you suggest to keep it up-to-date when needed. I would just rather do this change in stages, where we first focus on removing DCP and Dashboard, and then later we can move Project Templates to IDE/SDK and Hosting.SDK to be an MSBuild sdk.
There was a problem hiding this comment.
I can confirm that msbuild SDKs are still loaded on the UI thread. Jeff did a bunch of work to fix deadlocks but depending in network connectivity and size, there can be impact. msbuild sdks are fairly commonly used though and much of the perf/hang complaints come from folks with fairly complex builds and larger sdks.
There was a problem hiding this comment.
NuGet SDKs are used "a lot" (for some value of "a lot"), but I think nowhere near as much as we expect Aspire to be used. If we move all of the large components out of the workload, then I would consider moving what's left (mainly just props and targets I think) into the .NET SDK itself rather than out into a NuGet MSBuild SDK. You would lose the ability to version those components separately from the SDK, but I think that's actually probably an advantage, as there wouldn't be an extra, nonstandard version in the project file that had to be managed.
|
|
||
| ### Cons | ||
|
|
||
| - The main drawback of removing DCP and Dashboard from the workload, is the potential impacts it may have with restore times and local package caches. DCP especially is not small in size (~150mb) and it is also a native executable (written in Go), so we currently have separate packages each carrying the executable for each individual platform. One thing we were getting for free with DCP being part of the workload, is that the SDK authoring would automatically only install the DCP package for the platform that the SDK is being installed on. NuGet doesn't have great support today for platform-specific dependencies, which means that with the above proposal the Aspire.Hosting.AppHost package would depend on all of the DCP packages for all platforms, which would increase the size of the things that need to be restored, as well as the time it takes to do it. |
There was a problem hiding this comment.
Could we not utilize the ref-split packages pattern to achieve the same outcome of only downloading the runtime packages required for the current runtime? IIUC this is exactly how the .NET SDK itself avoids downloading runtime packages for shared frameworks until specifically required.
There was a problem hiding this comment.
Sorry, I'm not sure I'm following here. Shared frameworks are kind of special here as they are built-in on the SDK itself, and one of the "implicit" references, so the SDK has special logic to deal with that and not have to pull in all different platform specific runtime packs. For regular NuGet packages, AFAIK there isn't a NuGet built-in way of restoring platform-specific packages unless you restore using a RID, which IMO is not something we want to enforce on our customers. That's how the whole runtime.json stuff for RID specific dependencies worked, it basically relied on people doing dotnet restore -r win-x64 so the right section of the runtime.json could be picked.
There was a problem hiding this comment.
I just meant using the pattern of splitting different platform implementations into runtime.[rid]. prefixed packages and then having our targets in Aspire.Hosting take care of restoring them and injecting the appropriate paths into the AppHost assembly. I think this is very similar to how things like the ILCompiler toolchain works today, e.g. runtime.win-x64.Microsoft.DotNet.ILCompiler
There was a problem hiding this comment.
(Note: I'm not an expert here, so I could be wrong)
The ILCompiler and crossgen are "special" because they are known by the SDK explicitly.
And then that **RID** gets replaced with the correct RID in a C# task:
I'm not sure we could take this same approach outside of the SDK.
There is/was a feature in NuGet that we used to use called runtime.json, but AFAIK it was never officially supported outside of System.* packages (which stopped using the feature).
See:
- [Question] How to create separate NuGet packages for OS specific native components NuGet/Home#6083
- Improve handling of native packages (Support RID specific dependencies) NuGet/Home#10571
- Nuget needs to support dependencies specific to target runtime NuGet/Home#1660
- Cross Platform: Add support for Anchor/Complementary Packages. NuGet/Home#3114
There was a problem hiding this comment.
I also believe this is not something that any package can do, but I'll look into it.
There was a problem hiding this comment.
There is https://learn.microsoft.com/en-us/nuget/consume-packages/packagedownload-functionality also. This will just download the specific nuget as part of the regular restore.
dotnet/runtime uses this for wasm in a couple of places.
There was a problem hiding this comment.
Is it as simple as having our SDK targets set to "true" before restore, and having Aspire.Hosting.AppHost depend on Aspire.Hosting.Orchestration, which in turn contains a runtime.json file that defines the runtime-specific packages to bring in?
I don't think we want to use runtime.json. See dotnet/runtime#49137 where the runtime is trying to get off that feature.
There was a problem hiding this comment.
CC @dsplaisted who's our expert on packagedownload as well as the special processing we do for RIDs for framework packages.
There was a problem hiding this comment.
I don't believe the existing NuGet runtime.json / RID-specific functionality would work for this at all (even if we weren't trying to avoid using it in general). That feature lets you get different packages based on the RuntimeIdentifier the app is using. However, what we want here is to get the DCP etc. packages for the RuntimeIdentifier of the host. If we support cross-targeting (ie building an Aspire app for linux-x64 from a windows-x64 machine), then NuGet's runtime.json functionality doesn't help.
It may be possible to avoid downloading DCP etc. for all platforms with logic in the Aspire SDK targets. There could be logic that detected a PackageReference to Aspire.Hosting.AppHost, and if it was found would add PackageReference or PackageDownload items to the corresponding platform-specific versions of the DCP packages. That might have some issues and wouldn't work for transitive references to Aspire.Hosting.AppHost, but if I understand correctly there may not be scenarios where that package is transitively referenced.
There was a problem hiding this comment.
If we support cross-targeting (ie building an Aspire app for linux-x64 from a windows-x64 machine), then NuGet's runtime.json functionality doesn't help.
The AppHost project is never deployed so I don't think this is an issue.
It may be possible to avoid downloading DCP etc. for all platforms with logic in the Aspire SDK targets. There could be logic that detected a PackageReference to Aspire.Hosting.AppHost, and if it was found would add PackageReference or PackageDownload items to the corresponding platform-specific versions of the DCP packages. That might have some issues and wouldn't work for transitive references to Aspire.Hosting.AppHost, but if I understand correctly there may not be scenarios where that package is transitively referenced.
This is the approach being proposed now I believe.
| ### Cons | ||
|
|
||
| - The main drawback of removing DCP and Dashboard from the workload, is the potential impacts it may have with restore times and local package caches. DCP especially is not small in size (~150mb) and it is also a native executable (written in Go), so we currently have separate packages each carrying the executable for each individual platform. One thing we were getting for free with DCP being part of the workload, is that the SDK authoring would automatically only install the DCP package for the platform that the SDK is being installed on. NuGet doesn't have great support today for platform-specific dependencies, which means that with the above proposal the Aspire.Hosting.AppHost package would depend on all of the DCP packages for all platforms, which would increase the size of the things that need to be restored, as well as the time it takes to do it. | ||
| - Additionally, workloads also have a way of keeping a reference count on workload packs, so it is able to clean them up when workloads don't reference it any more (for example, if you update your workload, then the old DCP package will be cleaned up automatically). This doesn't happen with regular NuGet packages, where customers are responsible for cleaning up their NuGet package caches themselves. |
There was a problem hiding this comment.
I'm not super concerned about this as managing the package restore location is already a scenario users have to manage.
There was a problem hiding this comment.
Fair enough, I still wanted to mention it because technically it is a drawback, but I agree with you that is not a major concern and it still seems like the right move.
There was a problem hiding this comment.
It's definitely a concern we should bring back up with the nuget folks (cough cough @JonDouglas ) because it's an eternal pain point lol. And, in the future, if Aspire is used by people who aren't primarily .NET devs they might not know to clear it. But agree not a blocker, especially for now!
There was a problem hiding this comment.
Yes, please discuss your NuGet needs for this work long term with us. Whether that's moving away from workloads or not, we'd love to help.
|
We should test the DCP nuget packages on Mac/Linux to make sure there won't be any issues with the executable binaries; the SDK/workloads have a special mechanism for applying executable permissions (the UnixFilePermissions.xml file) and I'm not sure if simple nuget files support the same behavior. |
Good point. I did test this on Linux and Windows, and all scenarios I tested work as expected, but I didn't test on Mac yet. |
|
|
||
| Given the above, we propose to move the DCP and Dashboard components out of the workload, and into NuGet packages that are referenced as dependencies of the Aspire.Hosting.AppHost package. This will allow the AppHost project to reference the version of Aspire.Hosting that it wants, and then the DCP and Dashboard will be installed as dependencies of that version of Aspire.Hosting. This will allow the AppHost project to have full control over the version of the DCP and Dashboard that it wants to use, and will also allow customers to have multiple AppHost projects that reference different versions of Aspire.Hosting without any issues. This will also allow us to update the DCP and Dashboard independently of the workload, which will allow us to ship updates to these components more frequently and with less risk, with a far less complex compatibility matrix. | ||
|
|
||
| Also, there is no real need of including the Aspire.Hosting package as part of the workload, as there is no guarantee that this is the version that the AppHost project will want to use, therefore we would also remove this package from the workload. In the end, the workload will only contain the Aspire.Hosting.SDK and Aspire.ProjectTemplates components, which are the ones that can be truly considered as tooling components, and in both cases you would want to always be in the latest version. These two are also not tightly coupled to the version of the Aspire.Hosting package that the AppHost project references, so they can be updated independently of the AppHost. |
There was a problem hiding this comment.
The tight version coupling that DCP and Dashboard have makes sense in the explanation above. For Aspire.Hosting, I wasn't exactly clear. Is it just that it's not needed to load before restore so it doesn't need to be a workload so just include it in the set of packages?
There was a problem hiding this comment.
Is it just that it's not needed to load before restore so it doesn't need to be a workload so just include it in the set of packages?
Correct. Aspire.Hosting is just a normal package in that sense.
This is still a prototype and marked as a draft, so currently only looking for feedback on the proposal document being added here.
cc: @davidfowl @danegsta @mhutch @DamianEdwards @marcpopMSFT @eerhardt @danmoseley @maddymontaquila @radical
Microsoft Reviewers: Open in CodeFlow