Remove DCP and Dashboard from the .NET Aspire SDK Workload #4708

joperezr · 2024-06-27T21:15:07Z

Fixes #5185

cc: @davidfowl @danegsta @mhutch @DamianEdwards @marcpopMSFT @eerhardt @danmoseley @maddymontaquila @radical

Microsoft Reviewers: Open in CodeFlow

docs/specs/future-of-workload.md

danegsta · 2024-06-27T21:37:48Z

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. #Resolved

joperezr · 2024-06-27T22:35:43Z

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. #Resolved

docs/specs/future-of-workload.md

…version

src/Aspire.Hosting.Sdk/SDK/Sdk.targets

joperezr · 2024-08-21T17:53:33Z

Ok folks, this is ready to go. Can I get a final review so this can get merged? cc: @DamianEdwards, @eerhardt, @radical, @danegsta #Resolved

docs/specs/future-of-workload.md

eng/Build.props

eng/dashboardpack/buildMultiTargeting/Aspire.Dashboard.Sdk.in.props

eng/dashboardpack/buildMultiTargeting/Aspire.Dashboard.Sdk.in.targets

eng/dashboardpack/buildTransitive/Aspire.Dashboard.Sdk.in.props

src/Aspire.Hosting.Sdk/SDK/Sdk.targets

tests/Shared/Aspire.Workload.Testing.targets

tests/Directory.Build.props

src/Aspire.Hosting.Sdk/SDK/Sdk.targets

radical · 2024-08-21T21:23:44Z

Most of my feedback can be addressed in a follow up. #Resolved

src/Aspire.Hosting.Sdk/SDK/Sdk.targets

radical · 2024-08-22T00:10:08Z

src/Aspire.Hosting.Sdk/SDK/Sdk.targets

+    </PropertyGroup>
+
+    <!-- At this point, we should have the version either by CPM or PackageReference, so we fail if not.  -->
+    <Error Condition="'$(AppHostVersion)' == '' or $([MSBuild]::VersionLessThan('$(AppHostVersion)', '8.2.0'))" 


nit: It might be useful to split this into two separate error messages, to help with debugging when this fails.

AppHostVersion=='', we could not figure out the version to use, and have a better error message in case of CPM.

AppHostVersion < 8.2.0

We would know at that point. If 1., then the warning will not have the version you are using (see line 90 above). If 2 then you'd see which version you are using in the error message.

radical

This is great work! LGTM! 👍

eerhardt · 2024-08-21T18:38:58Z

docs/specs/future-of-workload.md

+
+.NET Aspire currently ships as a .NET SDK Workload. This workload currently includes the following components:
+
+- Aspire.Hosting.SDK (MSBuild SDK): This SDK contains some props and targets with the required logic to properly handle `ProjectReferences` in the .NET Aspire AppHost projects, which then drives the source generation that allows the AppHost project to have strongly typed references to project resources. This component cannot be changed into a regular NuGet package, as the logic it carries neeeds to be executed before the initial restore.


Suggested change

- Aspire.Hosting.SDK (MSBuild SDK): This SDK contains some props and targets with the required logic to properly handle `ProjectReferences` in the .NET Aspire AppHost projects, which then drives the source generation that allows the AppHost project to have strongly typed references to project resources. This component cannot be changed into a regular NuGet package, as the logic it carries neeeds to be executed before the initial restore.

- Aspire.Hosting.SDK (MSBuild SDK): This SDK contains some props and targets with the required logic to properly handle `ProjectReferences` in the .NET Aspire AppHost projects. This component cannot be changed into a regular NuGet package, as the logic it carries neeeds to be executed before the initial restore.

This bit makes it sound like the source generation comes from this SDK, but it doesn't. I would just drop it to avoid confusion.

eerhardt · 2024-08-21T18:39:41Z

docs/specs/future-of-workload.md

+- Aspire.Hosting.SDK (MSBuild SDK): This SDK contains some props and targets with the required logic to properly handle `ProjectReferences` in the .NET Aspire AppHost projects, which then drives the source generation that allows the AppHost project to have strongly typed references to project resources. This component cannot be changed into a regular NuGet package, as the logic it carries neeeds to be executed before the initial restore.
+- Aspire.ProjectTemplates (.NET Template Pack): This is a set of project templates that allow users to create new .NET Aspire projects using `dotnet new` commands and Visual Studio "New Project" dialog.
+- Aspire.Hosting.Orchestration (MSBuild SDK): This SDK contains two main things: the Developer Control Plane executable (DCP) as well as a targets file which defines properties for the AppHost to be able to find the DCP executable and run it.
+- Aspire.Dashboard.SDK (MSBuild SDK): Similar to the Orchestration SDK, this one provides the .NET Aspire Dashboard application (self-contained app that depends on .NET 8 runtime) and the targets file that defines properties for the AppHost to be able to find the Dashboard executable and launch it.


Suggested change

- Aspire.Dashboard.SDK (MSBuild SDK): Similar to the Orchestration SDK, this one provides the .NET Aspire Dashboard application (self-contained app that depends on .NET 8 runtime) and the targets file that defines properties for the AppHost to be able to find the Dashboard executable and launch it.

- Aspire.Dashboard.SDK (MSBuild SDK): Similar to the Orchestration SDK, this one provides the .NET Aspire Dashboard application (a framework-dependent app that depends on .NET 8 runtime) and the targets file that defines properties for the AppHost to be able to find the Dashboard executable and launch it.

This isn't a self-contained app.

eerhardt · 2024-08-21T18:40:07Z

docs/specs/future-of-workload.md

+- Aspire.ProjectTemplates (.NET Template Pack): This is a set of project templates that allow users to create new .NET Aspire projects using `dotnet new` commands and Visual Studio "New Project" dialog.
+- Aspire.Hosting.Orchestration (MSBuild SDK): This SDK contains two main things: the Developer Control Plane executable (DCP) as well as a targets file which defines properties for the AppHost to be able to find the DCP executable and run it.
+- Aspire.Dashboard.SDK (MSBuild SDK): Similar to the Orchestration SDK, this one provides the .NET Aspire Dashboard application (self-contained app that depends on .NET 8 runtime) and the targets file that defines properties for the AppHost to be able to find the Dashboard executable and launch it.
+- Aspire.Hosting (Regular NuGet Library): This package isn't really required to be in the workload, as this is a PackageReference that all AppHost projects will have. The advantage that we get currently from having it in the workload is that this would install it into the global NuGet cache when the workload is installed, so that the AppHost projects can find it without needing to restore it from the NuGet feed. This is helpful especially for offline scenarios. The package itself provides the Aspire.Hosting library which contains all of the main logic in Aspire for building and running .NET Aspire applications.


Suggested change

- Aspire.Hosting (Regular NuGet Library): This package isn't really required to be in the workload, as this is a PackageReference that all AppHost projects will have. The advantage that we get currently from having it in the workload is that this would install it into the global NuGet cache when the workload is installed, so that the AppHost projects can find it without needing to restore it from the NuGet feed. This is helpful especially for offline scenarios. The package itself provides the Aspire.Hosting library which contains all of the main logic in Aspire for building and running .NET Aspire applications.

- Aspire.Hosting.AppHost (Regular NuGet Library): This package isn't really required to be in the workload, as this is a PackageReference that all AppHost projects will have. The advantage that we get currently from having it in the workload is that this would install it into the global NuGet cache when the workload is installed, so that the AppHost projects can find it without needing to restore it from the NuGet feed. This is helpful especially for offline scenarios. The package itself provides the Aspire.Hosting library which contains all of the main logic in Aspire for building and running .NET Aspire applications.

The Aspire.Hosting.AppHost is the nuget package that is referenced by all AppHost projects. It has a dependency on Aspire.Hosting.

I did actually mean Aspire.Hosting as that is the one that we had in the workload, but the wording is wrong here I agree, so I'll fix that.

eng/Build.props

eerhardt · 2024-08-21T18:46:52Z

src/Aspire.Hosting.Sdk/SDK/Sdk.targets

+       a reference to Aspire.Dashboard.Sdk and Aspire.Hosting.Orchestration for the build-time platform using
+       the same version. This is done here dynamically to avoid having to pull in DCP and Dashboard packages
+       for all of the platforms. This mechanism can be disabled by setting `SkipAddReferenceToDashboardAndDCP` to `true` -->
+  <Target Name="AddReferenceToDashboardAndDCP" BeforeTargets="Restore;CollectPackageReferences" Condition="'$(SkipAddReferenceToDashboardAndDCP)' != 'true'">


Suggested change

<Target Name="AddReferenceToDashboardAndDCP" BeforeTargets="Restore;CollectPackageReferences" Condition="'$(SkipAddReferenceToDashboardAndDCP)' != 'true'">

<Target Name="AddReferenceToDashboardAndDCP" BeforeTargets="Restore;CollectPackageReferences" Condition="'$(SkipAddAspireDefaultReferences)' != 'true'">

Since this is more-or-less a public facing MSBuild property, I think we should leave names like "DCP" out of it.

SkipImplicitAspireReferences?

or AspireSkipImplicitReferences

eerhardt · 2024-08-21T18:48:13Z

playground/Directory.Build.props

+      Playground applications have their own way of referencing Aspire.Hosting.AppHost, as well as DCP and Dashboard, so we disable
+      the Aspire.Hosting.SDK targets that will automatically add these references to projects. 
+    -->
+    <SkipAddReferenceToDashboardAndDCP Condition="'$(TestsRunningOutsideOfRepo)' != 'true'">true</SkipAddReferenceToDashboardAndDCP>


@radical - above we are using Condition="'$(RepoRoot)' != ''". Is one check better than the other? Should we be consistent?

eerhardt · 2024-08-21T18:49:22Z

src/Aspire.Hosting.AppHost/build/Aspire.Hosting.AppHost.targets

+  <Target Name="__ErrorOnMininumWorkloadVersionMissing" BeforeTargets="PrepareForBuild"
+    Condition="'$(IsAspireHost)' == 'true' and ('$(AspireHostingSDKVersion)' == '' or $([MSBuild]::VersionLessThan('$(AspireHostingSDKVersion)', '8.2.0')))">
+    <Error Code="ASPIRE005"
+      Text="$(MSBuildProjectName) project requires a newer version of the .NET Aspire Workload to work correctly. Please run `dotnet workload update`." />


Should this message say what version is required?

Do we need a test for this?

Should this message say what version is required?

I'm not so sure about this one, but I am open to suggestions. The thinking is that this minimum version will change over time, so I'm not sure if we want consistent messages, especially when the fix is to run dotnet workload update which you can't really use to select which version to move to (as it always goes to latest).

Do we need a test for this?

I do think this would be good and I thought about it, but the problem is that it won't be trivial as I don't think we have a way of running tests with an SDK without the workload, or with an older workload, but @radical can correct me if I'm wrong.

Do we need a test for this?

I do think this would be good and I thought about it, but the problem is that it won't be trivial as I don't think we have a way of running tests with an SDK without the workload, or with an older workload, but @radical can correct me if I'm wrong.

We do have a way to run tests with a SDK that has the updated manifest from artifacts, but does not have the workload installed. I can add that test.

with an older workload
.. this would need some work. Do we want this one?

eerhardt · 2024-08-21T18:50:22Z

src/Aspire.Hosting.AppHost/Aspire.Hosting.AppHost.csproj

@@ -20,6 +20,10 @@
  </ItemGroup>

  <ItemGroup>
+    <!-- 
+        Note that this package won't add a transitive dependency to DCP and Dashboard, as those will dynamically be added by the SDK.


Suggested change

Note that this package won't add a transitive dependency to DCP and Dashboard, as those will dynamically be added by the SDK.

Note that this package won't add a transitive dependency to DCP and Dashboard, as those will dynamically be added by the Aspire.Hosting.Sdk.

eerhardt · 2024-08-21T18:56:21Z

tests/Shared/RepoTesting/Aspire.RepoTesting.targets

@@ -143,4 +143,9 @@
    <Import Project="$(RepoRoot)src\Aspire.Hosting.Sdk\SDK\Sdk.targets" Condition="Exists('$(RepoRoot)src\Aspire.Hosting.Sdk\SDK\Sdk.targets')" />
  </ImportGroup>

+  <PropertyGroup Condition="'$(RepoRoot)' != 'null' and '$(TestsRunningOutsideOfRepo)' != 'true' and '$(IsAspireHost)' == 'true'">


'$(RepoRoot)' != 'null'

is that the right condition?

I used the same condition than the one used by the itemgroup above (as I want it to apply exactly in the same situation). If we are changing one, we should change both.

eerhardt · 2024-08-21T19:11:29Z

eng/dashboardpack/Aspire.Dashboard.Sdk.linux-arm64.csproj

@@ -0,0 +1,9 @@
+<Project>


Are we creating all these RID specific packages for the Dashboard just so we can get RID specific app hosts? 99% of the nuget package is identical. The only difference is the executable that launches the app?

Could we make it more like a .NET global tool and only have to ship 1 nuget package?

We could, I believe. We actually no longer need to have all of these separate projects anymore, so in a follow up PR I am going to explore how to reduce and simplify this.

joperezr added 5 commits June 26, 2024 22:46

WIP: Switch from workload to instead be MSBuild SDKs for Aspire

e73ec3d

Change Dashboard to be a NuGet package dependency

ed4ec96

Bring back Aspire.Hosting.Sdk into the workload for P2P references

8667f40

Change DCP to be a NuGet package dependency

743ba94

Add proposal doc for the .NET Aspire Workload

8c3a92b

dotnet-issue-labeler bot added the area-codeflow for labeling automated codeflow. intentionally a different color! label Jun 27, 2024

joperezr commented Jun 27, 2024

View reviewed changes

docs/specs/future-of-workload.md Show resolved Hide resolved

joperezr added the area-acquisition label Jun 27, 2024

DamianEdwards reviewed Jun 27, 2024

View reviewed changes

docs/specs/future-of-workload.md Outdated Show resolved Hide resolved

DamianEdwards reviewed Jun 27, 2024

View reviewed changes

docs/specs/future-of-workload.md Outdated Show resolved Hide resolved

DamianEdwards reviewed Jun 27, 2024

View reviewed changes

docs/specs/future-of-workload.md Outdated Show resolved Hide resolved

marcpopMSFT reviewed Jun 28, 2024

View reviewed changes

docs/specs/future-of-workload.md Show resolved Hide resolved

danmoseley removed the area-codeflow for labeling automated codeflow. intentionally a different color! label Jul 9, 2024

joperezr added 6 commits August 7, 2024 09:57

Merge branch 'main' into workloads/MSBuildSDKs

c040418

WIP

6c84434

Onboarding into CPM

4b604ab

Playground applications building and running

fd82056

Test projects building

aa5ae81

Fix failing MSBuild test

015bd6b

build-analysis bot mentioned this pull request Aug 9, 2024

[tests] Flaky failures withApplication orchestrator dependency check returned an error: The operation has timed out #5195

Open

Produce 8.2.0 versions and add error in case of insufficient AppHost …

1a0b6a9

…version

danegsta reviewed Aug 9, 2024

View reviewed changes

src/Aspire.Hosting.Sdk/SDK/Sdk.targets Outdated Show resolved Hide resolved

build-analysis bot mentioned this pull request Aug 9, 2024

Failing test WithDockerfileTests.ContainerBuildLogsAreStreamedToAppHost #5246

Open

joperezr added 5 commits August 9, 2024 16:37

Add error when insufficient version of workload is detected.

ccf0468

Fix build breaks due to new Error

d5036f4

Fix msbuild test

3ec11e5

Only skip Dashboard and DCP references when not running outside of repo

bbfe505

Merge branch main

f52a55a

Fix DCP path on Aspire.Hosting.*.Tests

c3cdb9b

joperezr marked this pull request as ready for review August 16, 2024 22:08

joperezr requested review from mitchdenny and ReubenBond as code owners August 16, 2024 22:08

build-analysis bot mentioned this pull request Aug 17, 2024

Some tests timing out on helix #5247

Open

This comment was marked as outdated.

Sign in to view

build-analysis bot mentioned this pull request Aug 20, 2024

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

3 tasks

WIP: Test dcp path

bd4ea6f

joperezr force-pushed the workloads/MSBuildSDKs branch from 9c49d04 to bd4ea6f Compare August 20, 2024 19:50

joperezr added 2 commits August 20, 2024 15:38

Fix Workload failing tests

207d4a9

Fixing up docs, addressing feedback and final cleanup.

7c48160

Adding Workload test for CPM validation

a5479c9

radical reviewed Aug 21, 2024

View reviewed changes

This comment was marked as off-topic.

Sign in to view

radical reviewed Aug 22, 2024

View reviewed changes

src/Aspire.Hosting.Sdk/SDK/Sdk.targets Outdated Show resolved Hide resolved

radical reviewed Aug 22, 2024

View reviewed changes

src/Aspire.Hosting.Sdk/SDK/Sdk.targets Outdated Show resolved Hide resolved

radical reviewed Aug 22, 2024

View reviewed changes

joperezr added 2 commits August 22, 2024 10:14

Address PR feedback

54340cf

Make items private

063472a

radical approved these changes Aug 22, 2024

View reviewed changes

joperezr enabled auto-merge (squash) August 22, 2024 18:31

joperezr disabled auto-merge August 22, 2024 18:31

joperezr enabled auto-merge (squash) August 22, 2024 18:31

joperezr merged commit 4f5c1c3 into dotnet:main Aug 22, 2024
11 checks passed

eerhardt reviewed Aug 22, 2024

View reviewed changes

joperezr mentioned this pull request Aug 22, 2024

Addressing follow-up feedback from the workload PR. #5388

Merged

16 tasks

github-actions bot locked and limited conversation to collaborators Sep 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove DCP and Dashboard from the .NET Aspire SDK Workload #4708

Remove DCP and Dashboard from the .NET Aspire SDK Workload #4708

joperezr commented Jun 27, 2024 •

edited

Loading

danegsta commented Jun 27, 2024 •

edited by joperezr

Loading

joperezr commented Jun 27, 2024 •

edited

Loading

This comment was marked as outdated.

This comment was marked as outdated.

joperezr commented Aug 21, 2024 •

edited

Loading

radical commented Aug 21, 2024 •

edited by joperezr

Loading

This comment was marked as off-topic.

radical Aug 22, 2024

joperezr Aug 22, 2024

radical left a comment

eerhardt Aug 21, 2024

eerhardt Aug 21, 2024

eerhardt Aug 21, 2024

joperezr Aug 22, 2024

eerhardt Aug 21, 2024

radical Aug 22, 2024

radical Aug 22, 2024

eerhardt Aug 21, 2024

eerhardt Aug 21, 2024

eerhardt Aug 21, 2024

joperezr Aug 22, 2024

radical Aug 22, 2024

radical Aug 22, 2024

eerhardt Aug 21, 2024

eerhardt Aug 21, 2024

joperezr Aug 22, 2024

eerhardt Aug 21, 2024

joperezr Aug 22, 2024


		.NET Aspire currently ships as a .NET SDK Workload. This workload currently includes the following components:

		- Aspire.Hosting.SDK (MSBuild SDK): This SDK contains some props and targets with the required logic to properly handle `ProjectReferences` in the .NET Aspire AppHost projects, which then drives the source generation that allows the AppHost project to have strongly typed references to project resources. This component cannot be changed into a regular NuGet package, as the logic it carries neeeds to be executed before the initial restore.

	- Aspire.Dashboard.SDK (MSBuild SDK): Similar to the Orchestration SDK, this one provides the .NET Aspire Dashboard application (self-contained app that depends on .NET 8 runtime) and the targets file that defines properties for the AppHost to be able to find the Dashboard executable and launch it.
	- Aspire.Dashboard.SDK (MSBuild SDK): Similar to the Orchestration SDK, this one provides the .NET Aspire Dashboard application (a framework-dependent app that depends on .NET 8 runtime) and the targets file that defines properties for the AppHost to be able to find the Dashboard executable and launch it.

	<Target Name="AddReferenceToDashboardAndDCP" BeforeTargets="Restore;CollectPackageReferences" Condition="'$(SkipAddReferenceToDashboardAndDCP)' != 'true'">
	<Target Name="AddReferenceToDashboardAndDCP" BeforeTargets="Restore;CollectPackageReferences" Condition="'$(SkipAddAspireDefaultReferences)' != 'true'">

	Note that this package won't add a transitive dependency to DCP and Dashboard, as those will dynamically be added by the SDK.
	Note that this package won't add a transitive dependency to DCP and Dashboard, as those will dynamically be added by the Aspire.Hosting.Sdk.

Remove DCP and Dashboard from the .NET Aspire SDK Workload #4708

Remove DCP and Dashboard from the .NET Aspire SDK Workload #4708

Conversation

joperezr commented Jun 27, 2024 • edited Loading

Microsoft Reviewers: Open in CodeFlow

danegsta commented Jun 27, 2024 • edited by joperezr Loading

joperezr commented Jun 27, 2024 • edited Loading

This comment was marked as outdated.

This comment was marked as outdated.

joperezr commented Aug 21, 2024 • edited Loading

radical commented Aug 21, 2024 • edited by joperezr Loading

This comment was marked as off-topic.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radical left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joperezr commented Jun 27, 2024 •

edited

Loading

danegsta commented Jun 27, 2024 •

edited by joperezr

Loading

joperezr commented Jun 27, 2024 •

edited

Loading

joperezr commented Aug 21, 2024 •

edited

Loading

radical commented Aug 21, 2024 •

edited by joperezr

Loading