Refactor ProjectReferences in AppHost #1726
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,18 +12,22 @@ | |
| <ProjectCapability Include="Aspire" Condition=" '$(IsAspireHost)' == 'true' " /> | ||
| </ItemGroup> | ||
|
|
||
| <Target Name="_CreateServiceAppProjects"> | ||
|
|
||
| <ItemGroup> | ||
| <_ServiceAppProject Include="@(ProjectReference->WithMetadataValue('_IsServiceApp', 'true'))" /> | ||
| </ItemGroup> | ||
|
|
||
| </Target> | ||
|
|
||
| <!-- Generate the data structures for doing the codegen for service references --> | ||
| <Target Name="CreateServiceMetadataSources" | ||
| DependsOnTargets="_CreateServiceAppProjects"> | ||
eerhardt marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| <ItemGroup> | ||
| <ServiceMetadataSource Include="@(_ServiceAppProject)" Condition="'@(_ServiceAppProject)' != ''"> | ||
| <ClassName Condition="%(_ServiceAppProject.ServiceNameOverride) == ''">$([System.IO.Path]::GetFileNameWithoutExtension(%(_ServiceAppProject.Identity)).Replace(".", "_").Replace("-","_").Replace(" ","_"))</ClassName> | ||
|
There was a problem hiding this comment. Should we update this from There was a problem hiding this comment. Or maybe just There was a problem hiding this comment. What about I don't like calling it "ResourceName" because that is what we call the string parameter that pass in to
In MSBuild, I haven't seen setting something to empty and not setting it being different behavior. Usually checking for There was a problem hiding this comment. For Items we do have the 'HasMetadata' item function, but aside from this it can be hard to distinguish between missing and empty. There was a problem hiding this comment. I don't like setting this metadata to empty to mean something completely different than what the metadata is intended for.
I do think we should rename There was a problem hiding this comment. 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. |
||
| <ClassName Condition="%(_ServiceAppProject.ServiceNameOverride) != ''">$([System.String]::Copy(%(_ServiceAppProject.ServiceNameOverride)).Replace(".", "_").Replace("-","_").Replace(" ","_"))</ClassName> | ||
| <ProjectPath>$([System.IO.Path]::GetFullPath(%(_ServiceAppProject.Identity)))</ProjectPath> | ||
| </ServiceMetadataSource> | ||
| </ItemGroup> | ||
| </Target> | ||
|
|
@@ -32,33 +36,27 @@ | |
| <ItemGroup> | ||
| <ServiceMetadataSource Update="@(ServiceMetadataSource)"> | ||
| <Source> | ||
| <![CDATA[namespace Projects%3B | ||
|
|
||
| [global::System.Diagnostics.DebuggerDisplay("Type = {GetType().Name,nq}, ProjectPath = {ProjectPath}")] | ||
| public class ]]>%(ClassName)<![CDATA[ : global::Aspire.Hosting.IServiceMetadata | ||
eerhardt marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| public string ProjectPath => """]]>%(ProjectPath)<![CDATA["""%3B | ||
| }]]> | ||
| </Source> | ||
| </ServiceMetadataSource> | ||
| </ItemGroup> | ||
|
|
||
| <WriteLinesToFile File="$(_AspireIntermediatePath)references\%(ServiceMetadataSource.ClassName).ServiceMetadata.g.cs" | ||
| Overwrite="true" | ||
| Lines="%(ServiceMetadataSource.Source)" | ||
| Encoding="Unicode" | ||
| Condition="%(ServiceMetadataSource.ClassName) != ''" | ||
| WriteOnlyWhenDifferent="true" /> | ||
| <ItemGroup> | ||
| <FileWrites Include="$(_AspireIntermediatePath)references\%(ServiceMetadataSource.ClassName).ServiceMetadata.g.cs" /> | ||
| <Compile Include="$(_AspireIntermediatePath)references\%(ServiceMetadataSource.ClassName).ServiceMetadata.g.cs" | ||
| Condition="%(ServiceMetadataSource.ClassName) != ''" /> | ||
| </ItemGroup> | ||
| </Target> | ||
|
|
||
|
|
@@ -75,9 +73,7 @@ public class ]]>%(ClassName)<![CDATA[ : IServiceMetadata | |
| <ItemGroup> | ||
| <ProjectMetadataSource Update="@(ProjectMetadataSource)"> | ||
| <Source> | ||
| <![CDATA[namespace Projects%3B | ||
|
|
||
| internal static class ]]>%(ClassName)<![CDATA[ | ||
| { | ||
|
|
@@ -87,13 +83,11 @@ internal static class ]]>%(ClassName)<![CDATA[ | |
| </ProjectMetadataSource> | ||
| </ItemGroup> | ||
|
|
||
| <WriteLinesToFile File="$(_AspireIntermediatePath)references\_AppHost.ProjectMetadata.g.cs" | ||
| Overwrite="true" | ||
| Lines="%(ProjectMetadataSource.Source)" | ||
| Encoding="Unicode" | ||
| WriteOnlyWhenDifferent="true" /> | ||
| <ItemGroup> | ||
| <FileWrites Include="$(_AspireIntermediatePath)references\_AppHost.ProjectMetadata.g.cs" /> | ||
| <Compile Include="$(_AspireIntermediatePath)references\_AppHost.ProjectMetadata.g.cs" /> | ||
|
|
@@ -104,14 +98,83 @@ internal static class ]]>%(ClassName)<![CDATA[ | |
| <Warning Code="ASPIRE001" Text="The '$(Language)' language isn't fully supported by Aspire - some code generation targets will not run, so will require manual authoring." /> | ||
| </Target> | ||
|
|
||
| <!-- | ||
| Gets the references in 'AppProjectTargetFramework' that aren't executable projects. | ||
| --> | ||
| <UsingTask TaskName="GetNonExecutableReferences" | ||
| TaskFactory="RoslynCodeTaskFactory" | ||
| AssemblyFile="$(MSBuildToolsPath)\Microsoft.Build.Tasks.Core.dll"> | ||
| <ParameterGroup> | ||
| <AppProjectTargetFramework ParameterType="Microsoft.Build.Framework.ITaskItem[]" Required="true" /> | ||
| <NonExecutableReferences ParameterType="Microsoft.Build.Framework.ITaskItem[]" Output="true" /> | ||
| </ParameterGroup> | ||
| <Task> | ||
| <Using Namespace="System.Xml.Linq" /> | ||
| <Code Type="Fragment" Language="cs"> | ||
| <![CDATA[ | ||
eerhardt marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| HashSet<ITaskItem> nonExecutableReferences = new HashSet<ITaskItem>(); | ||
|
|
||
| foreach (var appProject in AppProjectTargetFramework) | ||
| { | ||
| var additionalProperties = appProject.GetMetadata("AdditionalPropertiesFromProject"); | ||
| if (string.IsNullOrEmpty(additionalProperties)) | ||
| { | ||
| // Skip any projects that don't contain the right metadata | ||
| continue; | ||
| } | ||
|
|
||
| var additionalPropertiesXml = XElement.Parse(additionalProperties); | ||
| foreach (var targetFrameworkElement in additionalPropertiesXml.Elements()) | ||
| { | ||
| var isExe = targetFrameworkElement.Element("_IsExecutable"); | ||
| if (isExe != null && !string.Equals(isExe.Value, "true", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| nonExecutableReferences.Add(appProject); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| NonExecutableReferences = nonExecutableReferences.ToArray(); | ||
| ]]> | ||
| </Code> | ||
| </Task> | ||
| </UsingTask> | ||
|
|
||
| <!-- | ||
| Validates that all the ProjectReferences of an Aspire AppHost project are executables and | ||
| informs the developer to set 'AspireHostLibrary=true' if they really intended on ProjectReferencing a library. | ||
| --> | ||
| <Target Name="_ValidateAspireHostServiceReferences" | ||
joperezr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| DependsOnTargets="_CreateServiceAppProjects" | ||
| Condition="'$(IsAspireHost)' == 'true' and '$(SkipValidateAspireHostServiceReferences)' != 'true'"> | ||
|
|
||
| <MSBuild Projects="@(_ServiceAppProject)" | ||
| Targets="GetTargetFrameworks" | ||
| BuildInParallel="$(BuildInParallel)" | ||
| Properties="%(_ServiceAppProject.SetConfiguration); %(_ServiceAppProject.SetPlatform)" | ||
| ContinueOnError="!$(BuildingProject)" | ||
| RemoveProperties="%(_ServiceAppProject.GlobalPropertiesToRemove);TargetFramework;RuntimeIdentifier;SelfContained;$(_GlobalPropertiesToRemoveFromProjectReferences)" | ||
| SkipNonexistentTargets="true"> | ||
| <Output TaskParameter="TargetOutputs" ItemName="_ServiceAppProjectTargetFramework" /> | ||
| </MSBuild> | ||
|
|
||
| <GetNonExecutableReferences AppProjectTargetFramework="@(_ServiceAppProjectTargetFramework)"> | ||
| <Output TaskParameter="NonExecutableReferences" ItemName="_NonExecutableServiceApps" /> | ||
| </GetNonExecutableReferences> | ||
|
|
||
| <Warning Code="ASPIRE004" | ||
joperezr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Condition="'@(_NonExecutableServiceApps)' != ''" | ||
| Text="'%(_NonExecutableServiceApps.OriginalItemSpec)' is referenced by an Aspire AppHost project, but it is not an executable. Did you mean to set AspireHostLibrary="true"?" /> | ||
| </Target> | ||
|
|
||
| <PropertyGroup> | ||
| <!-- Easy extension point for adding new languages' write support. --> | ||
| <WriteServiceMetadataSourcesDependsOn>_CSharpWriteServiceMetadataSources;_CSharpAppHostProjectMetadataSources;_WarnOnUnsupportedLanguage;_ValidateAspireHostServiceReferences</WriteServiceMetadataSourcesDependsOn> | ||
| </PropertyGroup> | ||
|
|
||
| <!-- The purpose of this target is to take all of the generated service metadata and write them to the intermediate build directory | ||
| and reference them for compilation. There will be a ClassName.ServiceMetadata.g.cs file for each referenced project. --> | ||
| <Target Name="WriteServiceMetadataSources" DependsOnTargets="$(WriteServiceMetadataSourcesDependsOn)" BeforeTargets="CoreCompile" /> | ||
|
|
||
| <!-- This target registers the location of the Aspire orchestration dependencies --> | ||
| <Target Name="SetOrchestrationDiscoveryAttributes" BeforeTargets="GetAssemblyAttributes"> | ||
|
|
@@ -157,11 +220,10 @@ internal static class ]]>%(ClassName)<
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
AddReferenceToAssemblyor something that is more obvious.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 haveServiceNameOverrideif you want to specify the name of theProjectsclass. SettingAspireServiceApp=falsewould mean "this isn't an application, so reference it like a normal ProjectReference".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nit: Prefixing the name with
Is*would make it more readable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I think initially I didn't add
Is*because the other ProjectReference metadata didn't follow this pattern:But these are more "should" and not "is", so I think it is a different scenario.
So should we have:
IsAspireHostLibrary=trueIsAspireServiceApp=false?
I think I'm leaning towards
IsAspireHostLibrary=truebecause the naming aligns withIsAspireHost=truefor the whole project.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really know yet all the special stuff done for these projects, so not sure what a
ServiceAppvsLibrarywould mean.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So:
ServiceApp- special project that the AppHost project will use in some way, maybe post-process etc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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").I don't know of this case. In my head there are mainly 2 cases: ServiceApps and "normal" ProjectReferences to libraries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My misunderstanding then.
I would vote for
IsAspireServiceApp=falsebecause we are changing the defaultProjectReferenceto mean a ServiceApp reference which once you know it, makes sense. IOW,AppHostproject referencesServiceApps by default. In that case for any other project references an opt-out of that default would make sense.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussing this more with @DamianEdwards, we decided to go with the name:
IsAspireProjectResource=falseto align with the "Resource" terminology in the app model.
We can then do some more renames:
ServiceNameOverride=>AspireProjectMetadataTypeNameIServiceMetadata=>IProjectMetadata