Refactor ProjectReferences in AppHost #1726

eerhardt · 2024-01-19T22:25:53Z

At a high-level there are 2 problems with how we are using ProjectReference to reference the individual "service application" projects in the AppHost.

The AppHost may not have a compatible TFM with the referenced application project. For example if the referenced application project was targeting net8-windows.
When we are generating IServiceMetadata code, we are using ReferencePathWithRefAssemblies items, which means if the ProjectReference has ReferenceOutputAssembly="false" on it, no IServiceMetadata code will be generated.

To resolve these issues, we are taking the following approach:

By default ProjectReferences in the AppHost will mean a reference to a "service application", and not a regular project reference. If an AppHost project truly wants a ProjectReference to another project, it can be marked as <ProjectReference Include="..." AppHostLibrary="true" />.
ProjectReferences to service applications will be marked ReferenceOutputAssembly=false, SkipGetTargetFrameworkProperties=true, ExcludeAssets=all. This will cause the AppHost to not reference the service app projects.
Refactor the IServiceMetadata code gen to act directly on ProjectReference items.

Fix #1074
Fix #1563

Microsoft Reviewers: Open in CodeFlow

baronfel · 2024-01-19T22:28:12Z

Looking forward to checking this out! One scenario I wanted to make sure we could handle is an Aspire AppHost referencing a self contained app - that currently breaks today.

eerhardt · 2024-01-19T23:02:39Z

One scenario I wanted to make sure we could handle is an Aspire AppHost referencing a self contained app - that currently breaks today.

Do you have repro steps I can try?

I took eshop, made one of the projects <SelfContained>true</SelfContained> and it still works.

baronfel · 2024-01-19T23:10:08Z

Perfect - that should do it. @bradygaster did that the other day to workaround a containers bug and he had to transitively flow SelfContained to the Aspire AppHost to get everything to continue working.

DamianEdwards · 2024-01-19T23:22:00Z

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

+  -->
+  <ItemGroup Condition="'$(IsAspireHost)' == 'true'">
+
+    <ProjectReference Update="@(ProjectReference)">


Should we add a global opt-out property for this behavior? e.g. EnableAspireProjectReferenceProcessing

Can you think of a good reason why?

Still slightly uneasy about taking over ProjectReferences and it's common for MSBuild defaults to have an opt-out. But we can wait for feedback.

What's the consensus here? I can add a property if people think it is valuable. (I also think we can add it later if we get feedback as well.)

DamianEdwards · 2024-01-19T23:26:09Z

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

+      <_ServiceAppProject Include="@(ProjectReference->WithMetadataValue('_IsServiceApp', 'true'))" />
+
+      <ServiceMetadataSource Include="@(_ServiceAppProject)" Condition="'@(_ServiceAppProject)' != ''">
+        <ClassName Condition="%(_ServiceAppProject.ServiceNameOverride) == ''">$([System.IO.Path]::GetFileNameWithoutExtension(%(_ServiceAppProject.Identity)).Replace(".", "_").Replace("-","_").Replace(" ","_"))</ClassName>


Should we update this from ServiceName to ResourceName now, i.e. ResourceNameOverride?

Or maybe just AspireResourceName and that can be used as the opt-out too by setting it to empty?

What about ProjectsClassName? Or even ProjectsName?

I don't like calling it "ResourceName" because that is what we call the string parameter that pass in to AddProject, which could be a different name than this name.

and that can be used as the opt-out too by setting it to empty?

In MSBuild, I haven't seen setting something to empty and not setting it being different behavior. Usually checking for == '' means "it isn't set".

For Items we do have the 'HasMetadata' item function, but aside from this it can be hard to distinguish between missing and empty.

I don't like setting this metadata to empty to mean something completely different than what the metadata is intended for.

<ProjectReference Include=.. /> - means this is a service app and don't reference it, use the default name for the Projects class name

<ProjectReference Include=.. AspireResourceName="foo" /> - means this is a service app and don't reference it, use "foo" for the Projects class name

<ProjectReference Include=.. AspireResourceName="" /> - means this isn't a service app and should be referenced

I do think we should rename ServiceNameOverride, but I don't know of a good name. If people think AspireResourceName is OK, I can rename it. I just have the reservations I listed above.

Strong agree that "empty metadatum" and "metadatum not specified" should not mean different things--in particular because there's no way to remove metadata other than setting it to empty.

tests/Aspire.Hosting.Tests/Aspire.Hosting.Tests.csproj

mitchdenny · 2024-01-21T23:11:52Z

playground/eShopLite/AppHost/AppHost.csproj

-    <ProjectReference Include="..\..\..\src\Aspire.Hosting\Aspire.Hosting.csproj" />
-    <ProjectReference Include="..\..\..\src\Aspire.Hosting.Azure\Aspire.Hosting.Azure.csproj" />
-    <ProjectReference Include="..\..\..\src\Aspire.Hosting.Azure.Provisioning\Aspire.Hosting.Azure.Provisioning.csproj" />
+    <ProjectReference Include="..\..\..\src\Aspire.Hosting\Aspire.Hosting.csproj" AppHostLibrary="true" />


So this will be required anytime you want to add a reference to a library and not treat it like a resource project?

Correct.

@rainersigwald suggested that we could add a build warning if we detect a <ProjectReference to a Library project and tell you that it won't get referenced by default, suggesting to add AppHostLibrary="true" to the ProjectReference. I think that is a good idea and am going to implement it here.

BTW - anyone have a better name than AppHostLibrary for this property?

How solid is the term AppHost? It feels pretty broad - my gut is that the property should be scoped with Aspire in some way to narrow it somewhat. Maybe AspireHostLibrary? Total gut feel though.

How solid is the term AppHost?

The name of the MSBuild property that "makes it an app host" is called <IsAspireHost>true</IsAspireHost>. I guess we just use .AppHost in the template.

Maybe AspireHostLibrary?

I like that. It fits with IsAspireHost.

@rainersigwald suggested that we could add a build warning if we detect a <ProjectReference to a Library project and tell you that it won't get referenced by default, suggesting to add AppHostLibrary="true" to the ProjectReference. I think that is a good idea and am going to implement it here.

I have added this build warning. I needed to write a C# task, but we don't have a task library today to put it in. So I used a RoslynCodeTaskFactory to get unblocked here. Once this is merged I plan on logging an issue to move this code into a proper Tasks .dll. I'm trying to not get blocked by adding that infrastructure here.

At a high-level there are 2 problems with how we are using ProjectReference to reference the individual "service application" projects in the AppHost. 1. The AppHost may not have a compatible TFM with the referenced application project. For example if the referenced application project was targeting `net8-windows`. 2. When we are generating IServiceMetadata code, we are using ReferencePathWithRefAssemblies items, which means if the ProjectReference has ReferenceOutputAssembly="false" on it, no IServiceMetadata code will be generated. To resolve these issues, we are taking the following approach: - By default ProjectReferences in the AppHost will mean a reference to a "service application", and not a regular project reference. If an AppHost project truly wants a ProjectReference to another project, it can be marked as `<ProjectReference Include="..." AppHostLibrary="true" />`. - ProjectReferences to service applications will be marked `ReferenceOutputAssembly=false`, `SkipGetTargetFrameworkProperties=true`, `ExcludeAssets=all`. This will cause the AppHost to not reference the service app projects. - Refactor the IServiceMetadata code gen to act directly on ProjectReference items. Fix dotnet#1074 Fix dotnet#1563

…respected by NuGet Restore

… the projects build even if the workload is not installed.

radical · 2024-01-22T21:05:25Z

To resolve these issues, we are taking the following approach:

By default ProjectReferences in the AppHost will mean a reference to a "service application", and not a regular project reference. If an AppHost project truly wants a ProjectReference to another project, it can be marked as .

Reversing this behavior might make more sense. The default would be a regular ProjectReference, as one would expect. And for a service-application-project-reference you can set some ServiceApplicationLibrary="true". That would be less surprising.

… library in the AppHost, but didn't set AspireHostLibrary=true.

eerhardt · 2024-01-23T00:17:59Z

Reversing this behavior might make more sense.

That would fall into the same category as approach (1) in #1563 (comment) - adding a new item type like AspireProjectResource. The problem with those approaches is that tooling doesn't know how to add a new item, or set this special metadata on ProjectReference. We would need to teach the tooling, for example add a new dotnet add aspire-reference command.

The 80+% case here is that when an AppHost has a ProjectReference, it is to an executable that it wants to start when the AppHost starts. That's why I think using an empty ProjectReference for this case makes the most sense.

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

joperezr · 2024-01-23T20:34:43Z

playground/CosmosEndToEnd/CosmosEndToEnd.AppHost/CosmosEndToEnd.AppHost.csproj

@@ -9,8 +9,9 @@
  </PropertyGroup>

  <ItemGroup>
-    <ProjectReference Include="..\..\..\src\Aspire.Hosting.Azure\Aspire.Hosting.Azure.csproj" />
-    <ProjectReference Include="..\..\..\src\Aspire.Hosting\Aspire.Hosting.csproj" />
+    <ProjectReference Include="..\..\..\src\Aspire.Hosting.Azure\Aspire.Hosting.Azure.csproj" AspireHostLibrary="true" />


NIT: Not really a suggestion, but I'm not sure I love the name of the metadata. From a user's POV it is not obvious what this means until you understand the context. I wonder if we should instead use something more explicit, like AddReferenceToAssembly or something that is more obvious.

I agree that I'm not super happy about this name, but I haven't found one I like better yet.

The metadata does more than just take away the reference to the assembly, it is really the switch between "is this an app that you want to orchestrate?" or "is this a library that has AppHost extension logic you want to use?".

Another idea is to flip it to be AspireServiceApp=false. We already have ServiceNameOverride if you want to specify the name of the Projects class. Setting AspireServiceApp=false would mean "this isn't an application, so reference it like a normal ProjectReference".

super nit: Prefixing the name with Is* would make it more readable.

super nit: Prefixing the name with Is* would make it more readable.

Agreed. I think initially I didn't add Is* because the other ProjectReference metadata didn't follow this pattern:

ReferenceOutputAssembly

SkipGetTargetFrameworkProperties

ExcludeAssets

But these are more "should" and not "is", so I think it is a different scenario.

So should we have:

IsAspireHostLibrary=true

-or-

IsAspireServiceApp=false
?

I think I'm leaning towards IsAspireHostLibrary=true because the naming aligns with IsAspireHost=true for the whole project.

I don't really know yet all the special stuff done for these projects, so not sure what a ServiceApp vs Library would mean.

ServiceApp is literally any type of project which represents a microservice that you want to launch via the AppHost. This means the AppHost will orchestrate those types of projects.

The AppHost, while it is in reality an orchestrator, in the end is just a C# project, so you could think of scenarios where people want to add project references to libraries too (which aren't really microservices). For example, imagine a company that has several Aspire apps, and they want to share some of the logic they have in their AppHost projects. For this, they would build a shared library, which AppHost projects would all reference. This library, is not really a service, but instead just a library used by the AppHost.

So:

ServiceApp - special project that the AppHost project will use in some way, maybe post-process etc.

Other case - simple library projects, and AppHost will reference the assemblies from these, and use them

Wasn't there another case of referencing projects that are simply meant as a build dependency, and should not affect AppHost project's build itself?

ServiceApp - special project that the AppHost project will use in some way, maybe post-process etc.

It isn't a "special" project. But ServiceApps are just regular ASP.NET apps - like a web api, or blazor server app. They can also be "Worker" apps, if they don't need to expose an HTTP endpoint.

The AppHost's Program.cs files uses these apps by calling builder.AppProject<ServiceApp>("serviceName").

Wasn't there another case of referencing projects that are simply meant as a build dependency, and should not affect AppHost project's build itself?

I don't know of this case. In my head there are mainly 2 cases: ServiceApps and "normal" ProjectReferences to libraries.

I don't know of this case. In my head there are mainly 2 cases: ServiceApps and "normal" ProjectReferences to libraries.

My misunderstanding then.

I would vote for IsAspireServiceApp=false because we are changing the default ProjectReference to mean a ServiceApp reference which once you know it, makes sense. IOW, AppHost project references ServiceApps by default. In that case for any other project references an opt-out of that default would make sense.

Discussing this more with @DamianEdwards, we decided to go with the name:

IsAspireProjectResource=false

Setting this to false opts you into being an "AppHost library", you don't get "Projects" code generated, and you start referencing the project

to align with the "Resource" terminology in the app model.

We can then do some more renames:

ServiceNameOverride => AspireProjectMetadataTypeName

IServiceMetadata => IProjectMetadata

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

tests/Aspire.Hosting.Tests/Aspire.Hosting.Tests.csproj

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

joperezr · 2024-01-23T22:00:09Z

tests/Aspire.Hosting.Tests/MSBuildTests.cs

+            var outputDone = new ManualResetEvent(false);
+            var process = new Process();
+            // set '-nodereuse:false -p:UseSharedCompilation=false' so the MSBuild and Roslyn server processes don't hang around, which may hang the test in CI
+            process.StartInfo = new ProcessStartInfo("dotnet", $"build -nodereuse:false -p:UseSharedCompilation=false")


The only thing that worries me from this is how dotnet resolution might differ in different machines and CI. I wonder if we should get .dotnet by analyzing the CurrentProcess arg0, so that we use the same dotnet as the one that was launched for running these tests.

For running tests on the build machine, setting that or explicitly using dotnet from .dotnet should be enough.
On helix there will only be the dotnet with version matching the one in global.json, so it shouldn't be an issue there.

Local runs can do the same thing - compute a path to .dotnet/dotnet, and use that.

or explicitly using dotnet from .dotnet should be enough.

I don't think we can rely on this. If the machine already has the SDK listed in global.json globally installed, then the build will skip restoring it and creating a .dotnet folder, and instead will just use the global one. We've been bit by this in the past.

Good point. I guess in that case use dotnet from PATH, which then should be the correct version?

I think that only works when you set it on the ProcessStartInfo in the first place and used that to start the process?

alternative: Process.GetCurrentProcess().MainModule.FileName;

These approaches don't work because the process being run is:

C:\git\aspire\artifacts\bin\Aspire.Hosting.Tests\Debug\net8.0\testhost.exe

We are already getting the "repo root" in this test. So the only viable options I see are:

Doing what I'm doing here and using the dotnet on the path.

Using {repoRoot}\.dotnet\dotnet.

Thoughts on those 2 options? (or are there others?)

You can't depend on (2) because of the reason @joperezr mentioned where your system dotnet version matches the one in global.json, thus you don't get any .dotnet.

One possible thing is to add $(DotnetTool) from msbuild to PATH when running the tests, which should make the expected dotnet available in PATH.

One possible thing is to add $(DotnetTool) from msbuild to PATH when running the tests

How would one do that? I'm running the tests in VS using the Test Explorer.

Either way, this wouldn't change the test code - we would just invoke dotnet and let the PATH take care of it.

forgot about VS! 😬

tests/Aspire.Hosting.Tests/MSBuildTests.cs

joperezr · 2024-01-23T22:30:00Z

Overall comment: Correct me if I'm wrong, but I don't believe this change is braking for most of the existing Aspire apps, right? meaning, for all of the customers that created Aspire apps using preview2, and will now want to upgrade their workload and references to packages, they won't need additional changes for this to work. (unless of course they are using libraries from their apphost)

eerhardt · 2024-01-24T03:36:26Z

Overall comment: Correct me if I'm wrong, but I don't believe this change is braking for most of the existing Aspire apps, right? meaning, for all of the customers that created Aspire apps using preview2, and will now want to upgrade their workload and references to packages, they won't need additional changes for this to work. (unless of course they are using libraries from their apphost)

Correct this is not breaking for normal Aspire apps. It is breaking for people using ProjectReferences and needing to reference those projects from their AppHost. In that case, they will need to set AspireHostLibrary=true.

- Rename AppHostLibrary to IsAppHostLibrary - Dispose of Process in test

eerhardt · 2024-01-24T18:48:33Z

I plan on merging this today in order to get it in for preview3 with enough bake time. Let me know if you have any feedback that would block merging this. We can tweak it as we go after merge as well.

- IsAspireProjectResource=false - Setting this to false opts you into being an "AppHost library", you don't get "Projects" code generated, and you start referencing the project - ServiceNameOverride => AspireProjectMetadataTypeName This also allows us to rename IServiceMetadata to IProjectMetadata.

eerhardt requested review from davidfowl, DamianEdwards, baronfel, rainersigwald and joperezr January 19, 2024 22:25

dotnet-issue-labeler bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Jan 19, 2024

DamianEdwards reviewed Jan 19, 2024

View reviewed changes

tests/Aspire.Hosting.Tests/Aspire.Hosting.Tests.csproj Outdated Show resolved Hide resolved

mitchdenny reviewed Jan 21, 2024

View reviewed changes

mitchdenny assigned eerhardt Jan 21, 2024

mitchdenny added this to the preview 3 (Feb) milestone Jan 21, 2024

eerhardt added 6 commits January 22, 2024 11:55

Move ProjectReference defaulting logic into the SDK targets so it is …

dc6235b

…respected by NuGet Restore

Fix test AppHost projects to also import the SDK.targets

935095c

Suppress importing the orchestration SDK targets in test AppHosts, so…

8290c72

… the projects build even if the workload is not installed.

Update CosmosEndToEnd app

fdb8f60

Rename AppHostLibrary => AspireHostLibrary

9f8a853

eerhardt force-pushed the FixProjectReference branch from e0a57dd to 9f8a853 Compare January 22, 2024 19:08

Add a build warning informing developers when they ProjectReference a…

3917b31

… library in the AppHost, but didn't set AspireHostLibrary=true.

danmoseley reviewed Jan 23, 2024

View reviewed changes

src/Aspire.Hosting/build/Aspire.Hosting.targets Show resolved Hide resolved

davidfowl approved these changes Jan 23, 2024

View reviewed changes

Add test to ensure warning is emitted.

4fc8333