-
Notifications
You must be signed in to change notification settings - Fork 694
Update Aspire.AppHost.Sdk to support simpler file-based app AppHosts #11445
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 11445 Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 11445" |
bda9216
to
112e4e9
Compare
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.
Pull Request Overview
This PR implements automatic importing of the base .NET SDK from the Aspire.AppHost.Sdk when it's not already imported, enabling single-file app host templates to only reference the Aspire SDK while automatically getting .NET SDK functionality.
- Adds conditional import logic to automatically include Microsoft.NET.Sdk when not present
- Moves property defaults (PublishAot=false, etc.) from template to SDK for centralized management
- Adds implicit PackageReference for Aspire.Hosting.AppHost when not already referenced
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
apphost.cs | Removes explicit Microsoft.NET.Sdk, package reference, and property declarations from single-file template |
Sdk.in.props | Adds conditional Microsoft.NET.Sdk import logic and default property values |
Sdk.in.targets | Adds Microsoft.NET.Sdk targets import and new targets for AOT defaults and implicit package references |
<AspireRidToolExecutable>$(AspireRidToolDirectory)Aspire.RuntimeIdentifier.Tool.dll</AspireRidToolExecutable> | ||
</PropertyGroup> | ||
|
||
<Target Name="DisablePublishAotForAspireHost" BeforeTargets="_ComputeToolPackInputsToProcessFrameworkReferences;_CollectTargetFrameworkForTelemetry;ProcessFrameworkReferences;Restore;CollectPackageReferences" Condition="'$(IsAspireHost)' == 'true' and '$(DisableAspirePublishAotDefaults)' != 'true'"> |
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.
[nitpick] The target name 'DisablePublishAotForAspireHost' doesn't accurately reflect what the target does - it sets multiple AOT and trimming properties, not just PublishAot. Consider renaming to 'SetAspireHostAotDefaults' or 'ConfigureAspireHostPublishSettings' for better clarity.
<Target Name="DisablePublishAotForAspireHost" BeforeTargets="_ComputeToolPackInputsToProcessFrameworkReferences;_CollectTargetFrameworkForTelemetry;ProcessFrameworkReferences;Restore;CollectPackageReferences" Condition="'$(IsAspireHost)' == 'true' and '$(DisableAspirePublishAotDefaults)' != 'true'"> | |
<Target Name="SetAspireHostAotDefaults" BeforeTargets="_ComputeToolPackInputsToProcessFrameworkReferences;_CollectTargetFrameworkForTelemetry;ProcessFrameworkReferences;Restore;CollectPackageReferences" Condition="'$(IsAspireHost)' == 'true' and '$(DisableAspirePublishAotDefaults)' != 'true'"> |
Copilot uses AI. Check for mistakes.
<PublishAot>false</PublishAot> | ||
<PublishTrimmed>false</PublishTrimmed> | ||
<EnableAotAnalyzer>false</EnableAotAnalyzer> | ||
<EnableTrimAnalyzer>false</EnableTrimAnalyzer> | ||
<EnableSingleFileAnalyzer>false</EnableSingleFileAnalyzer> |
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.
These property defaults are duplicated in both Sdk.in.props (lines 7-11) and in the DisablePublishAotForAspireHost target in Sdk.in.targets (lines 56-60). This duplication could lead to inconsistencies. Consider consolidating these settings in one location, preferably in the target where they can be conditionally applied.
<PublishAot>false</PublishAot> | |
<PublishTrimmed>false</PublishTrimmed> | |
<EnableAotAnalyzer>false</EnableAotAnalyzer> | |
<EnableTrimAnalyzer>false</EnableTrimAnalyzer> | |
<EnableSingleFileAnalyzer>false</EnableSingleFileAnalyzer> |
Copilot uses AI. Check for mistakes.
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.
Yep, doing it in both places until dotnet/sdk#50885 is merged and flows through to rc.2 builds.
<IsAspireHost>true</IsAspireHost> | ||
<PublishAot>false</PublishAot> | ||
<PublishTrimmed>false</PublishTrimmed> | ||
<EnableAotAnalyzer>false</EnableAotAnalyzer> |
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.
Instead of setting all of these it may be easier to set IsAotCompatible
and IsTrimmable
to false - these are an implicit condition of all 3 of the analyzers, and they also tie into disabling the ProjectCapability Items you remove later on:
- https://github.com/dotnet/sdk/blob/b64c645566412038bd42f080f27525f07d88ba8b/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L23-L24
- https://github.com/dotnet/sdk/blob/f9cf0b926534cdb941c876cbf021288198e673c9/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.Analyzers.targets#L76-L96
- https://github.com/dotnet/sdk/blob/b64c645566412038bd42f080f27525f07d88ba8b/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L35-L42
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.
Let me try.
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.
Actually, I think still need to set these explicitly don't I because they'll be set an evaluation due to the file-based apps virtual project setting PublishAot
to true?
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.
Also AppHost projects aren't publishable so the publish targets shouldn't matter in this case.
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.
mm not sure I love setting these in the SDK, where other projects that are not file-based apps would be impacted. Is there a way to fix this via the virtual project somehow? For instance, if it sees that the aspire sdk is set then set these to false inside the virtual 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.
Yeah I'm not a fan of hard-coding stuff like that in file-based apps directly, that's backwards :)
SDKs should be able to impart changes on projects that reference them, that's not an issue. This specific implementation of doing it in the targets is a temp workaround for file-based apps which we can remove once that's fixed. Then we'll just be setting the default values of these in our Sdk.in.props
file.
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.
Is there a property that we can chain of only defined in virtual projects so we can add a condition to these? If not, I think at the very least we should add conditions to all of these to check if they were not already defined (meaning only set them if they are not set already)
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 think at the very least we should add conditions to all of these to check if they were not already defined (meaning only set them if they are not set already)
This specifically won't work because of where they have to run, that's the issue.
We can condition this so that it only applies to file-based apps (there's a property) if we really want to isolate it, but I'm not convinced it matters. What's the concern?
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.
put the other way - are there actual valid use cases for an Aspire AppHost to be trimmed/AOT? If not, why even allow it?
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.
There are not, it's dev time only and we block publishing.
<AspireRidToolExecutable>$(AspireRidToolDirectory)Aspire.RuntimeIdentifier.Tool.dll</AspireRidToolExecutable> | ||
</PropertyGroup> | ||
|
||
<Target Name="DisablePublishAotForAspireHost" BeforeTargets="_ComputeToolPackInputsToProcessFrameworkReferences;_CollectTargetFrameworkForTelemetry;ProcessFrameworkReferences;Restore;CollectPackageReferences" Condition="'$(IsAspireHost)' == 'true' and '$(DisableAspirePublishAotDefaults)' != 'true'"> |
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.
There's a part of me that wants to try and trim this dependency list - e.g. Restore is implied by CollectPackageReferences, so it could be safe to drop Restore directly.
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.
The intent is for this to go away completely once dotnet/sdk#50885 is in anyway.
</PropertyGroup> | ||
|
||
<Target Name="DisablePublishAotForAspireHost" BeforeTargets="_ComputeToolPackInputsToProcessFrameworkReferences;_CollectTargetFrameworkForTelemetry;ProcessFrameworkReferences;Restore;CollectPackageReferences" Condition="'$(IsAspireHost)' == 'true' and '$(DisableAspirePublishAotDefaults)' != 'true'"> | ||
<PropertyGroup> |
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.
Why do you need this target if these are defined in Sdk.props already statically?
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.
Because file-based apps resets the PublishAot
property (and thus everything that is evaluated off of its value) directly in the virtual project today, so setting it in our props gets overwritten. Doing it in this target is required to ensure it overrides what is set in the project. This whole target will be removed once dotnet run
is fixed to only set the implicit project-level stuff if a value isn't already explicitly set (dotnet/sdk#50885).
<ItemGroup> | ||
<!-- Add the reference to the Aspire.Hosting.AppHost package if not already referenced --> | ||
<PackageReference Include="Aspire.Hosting.AppHost" Version="$(_ImplicitAppHostVersion)" IsImplicitlyDefined="true" | ||
Condition="'@(PackageReference->WithMetadataValue('Identity','Aspire.Hosting.AppHost'))' == '' And '@(PackageVersion->WithMetadataValue('Identity','Aspire.Hosting.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.
NIT: why do you need to check PackageVersion too? Even if using CPM, wouldn't we want to do this even if there was a PackageVersion for it?
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 think that's copilot being helpful 😄
I think it's safe to remove the check for PackageVersion like you say.
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.
Although the challenge there is, if using CPM, we don't want to implicitly add the package with a version right? Because that version should now be controlled by CPM, not the PackageReference we add. Or does the IsImplicitlyDefined="true"
effectively turn off CPM for that PackageReference?
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.
Yes, when you mark the package as implicitly defined then you can pass a version.
</PropertyGroup> | ||
<ItemGroup> | ||
<!-- Add the reference to the Aspire.Hosting.AppHost package if not already referenced --> | ||
<PackageReference Include="Aspire.Hosting.AppHost" Version="$(_ImplicitAppHostVersion)" IsImplicitlyDefined="true" |
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.
One thing I don't like about this change, is that it may cause people to remove the PackageReference from their project, and therefore never finding out if there is a newer version of Aspire given Msbuild SDKs don't have update notifications. If possible, I'd also condition this to only happen for file-based apps.
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.
We don't have to advertise it for now and the templates aren't being changed. Plus we have aspire update
(assuming that still works 😄). If we're really super concerned about this, we can limit this to file-based AppHosts only.
We need to make sure that |
We don't need to advertise this being doable for non-file-based AppHosts for now. Indeed this doesn't change the current templates to remove the package reference. But if it's simply ensuring that <Project Sdk="Aspire.AppHost.Sdk/9.5.0-pr.11445.979496fd"> |
That being the case I'm more supportive of this change. When we get to Aspire vNext we'll just need to make sure we update |
…file-based apps only
@joperezr @baronfel @mitchdenny I've updated this so that it only applies to file-based apps to limit the blast radius. We can expand this to regular AppHosts when we're ready. |
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 just cross tested this with #11451 just to make sure that it continued to work with the incoming single file support for |
/backport to release/9.5 |
Started backporting to release/9.5: https://github.com/dotnet/aspire/actions/runs/17817878589 |
Description
Updates the Aspire.AppHost.Sdk to auto-import the Microsoft.NET.Sdk if it detects it isn't already referenced. It also updates the Aspire.AppHost.Sdk to auto include a package reference to Aspire.Hosting.AppHost with the same version as the SDK if it isn't already referenced.
The Aspire.AppHost.Sdk was also updated to ensure the various markers for native AOT and trimming are forcibly disabled so that file-based AppHosts (and others) default to
PublishAot=false
. This can be simplified once dotnet/sdk#50885 is merged.The enables an AppHost project or file-based app to just reference the Aspire.AppHost.Sdk SDK. The single-file AppHost template has been updated in this PR to reflect this:
The changes are gated to only apply to file-based app AppHosts for now.
Checklist