Use an MSBuild Task for RID selection instead of shelling out #8686
There are no files selected for viewing
This file contains hidden or 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 hidden or 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 hidden or 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 hidden or 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
File renamed without changes.
This file contains hidden or 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 |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using Microsoft.Build.Framework; | ||
| using Aspire.Hosting.Sdk; | ||
| using NuGet.RuntimeModel; | ||
|
|
||
| namespace Aspire.RuntimeIdentifier.Tool; | ||
|
|
||
| /// <summary> | ||
| /// This task uses the given RID graph in a given SDK to pick the best match from among a set of supported RIDs for the current RID | ||
| /// </summary> | ||
| public sealed class PickBestRid : Microsoft.Build.Utilities.Task | ||
| { | ||
| /// <summary> | ||
| /// The path to the RID graph to read | ||
| /// </summary> | ||
| [Required] | ||
| public string RuntimeGraphPath { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// The RID of the current process | ||
| /// </summary> | ||
| [Required] | ||
| public string CurrentRid { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// All of the RIDs that Aspire supports | ||
| /// </summary> | ||
| [Required] | ||
| public string[]? SupportedRids { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// The solution to the puzzle | ||
| /// </summary> | ||
| [Output] | ||
| public string MatchingRid { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Computes the thing | ||
| /// </summary> | ||
| /// <returns></returns> | ||
| public override bool Execute() | ||
| { | ||
| if (!File.Exists(RuntimeGraphPath)) | ||
| { | ||
| Log.LogError("File {0} does not exist. Please ensure the runtime graph path exists.", RuntimeGraphPath); | ||
| return !Log.HasLoggedErrors; | ||
| } | ||
|
|
||
| RuntimeGraph graph = JsonRuntimeFormat.ReadRuntimeGraph(RuntimeGraphPath); | ||
| string? bestRidForPlatform = NuGetUtils.GetBestMatchingRid(graph, CurrentRid, SupportedRids, out bool wasInGraph); | ||
|
|
||
| if (!wasInGraph) | ||
| { | ||
| Log.LogError("Unable to find a matching RID for {0} from among {1} in graph {2}", CurrentRid, string.Join(",", SupportedRids), RuntimeGraphPath); | ||
| return !Log.HasLoggedErrors; | ||
| } | ||
|
|
||
| MatchingRid = bestRidForPlatform; | ||
| return true; | ||
| } | ||
| } |
This file contains hidden or 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 |
|---|---|---|
|
|
@@ -23,7 +23,7 @@ | |
| This defaulting needs to happen in the SDK targets so this metadata affects NuGet Restore. | ||
| --> | ||
| <ItemGroup Condition="'$(IsAspireHost)' == 'true'"> | ||
|
|
||
| <ProjectReference Update="@(ProjectReference)"> | ||
| <IsAspireProjectResource Condition="'%(IsAspireProjectResource)' != 'false'">true</IsAspireProjectResource> | ||
|
|
||
|
|
@@ -43,11 +43,7 @@ | |
| <Warning Code="ASPIRE003" Text="$(MSBuildProjectName) is a .NET Aspire AppHost project that requires Visual Studio version 17.10 or above to work correctly. You are using version $(MSBuildVersion)." /> | ||
| </Target> | ||
|
|
||
| <UsingTask AssemblyFile="$(MSBuildThisFileDirectory)../tasks/netstandard2.0/Aspire.AppHost.Tasks.dll" TaskName="PickBestRid" /> | ||
|
|
||
| <!-- This target extracts the version of Aspire.Hosting.AppHost referenced by the project, and adds | ||
| a reference to Aspire.Dashboard.Sdk and Aspire.Hosting.Orchestration for the build-time platform using | ||
|
|
@@ -66,7 +62,7 @@ | |
| <_AppHostVersion>%(_AppHostPackageReference.Version)</_AppHostVersion> | ||
| </PropertyGroup> | ||
|
|
||
| <!-- If the version is still empty, then we assume the project is using Central Package Management, | ||
| and we try to extract the PackageVersion Items. --> | ||
| <ItemGroup Condition="'$(_AppHostVersion)' == ''"> | ||
| <_AppHostPackageReference /> | ||
|
|
@@ -91,52 +87,19 @@ | |
| </PropertyGroup> | ||
|
|
||
| <!-- At this point, we should have the version either by CPM or PackageReference, so we fail if not. --> | ||
| <Error Condition="'$(_AppHostVersion)' == ''" | ||
| Text="$(MSBuildProjectName) is a .NET Aspire AppHost project that needs a package reference to Aspire.Hosting.AppHost version 8.2.0 or above to work correctly." /> | ||
|
|
||
| <!--The following is the list of the RIDs that we currently multitarget for DCP and Dashboard packages--> | ||
| <ItemGroup> | ||
| <_DashboardAndDCPSupportedTargetingRIDs Include="win-x64;win-x86;win-arm64;linux-x64;linux-arm64;osx-x64;osx-arm64" /> | ||
|
There was a problem hiding this comment. this is a nitpick kind of thing - when the items are declared inline like this they show as one 'thing' in the binlog viewer: this is easier to look at than N separate items. |
||
| </ItemGroup> | ||
|
|
||
|
Comment on lines
-108
to
-122
There was a problem hiding this comment. none of this is necessary if we're not spawning binaries anymore in the SDK-delivered targets |
||
| <!-- Compute the RID for Dashboard and DCP packages --> | ||
| <PickBestRid RuntimeGraphPath="$(BundledRuntimeIdentifierGraphFile)" CurrentRid="$(NETCoreSdkRuntimeIdentifier)" SupportedRids="@(_DashboardAndDCPSupportedTargetingRIDs)"> | ||
|
There was a problem hiding this comment. Could There was a problem hiding this comment. maybe? I was mostly focused on translating the behaviors that existed before. I like than an explicit lookup will have a clearer error message than a missing implicitly-defined PackageReference. There was a problem hiding this comment. Most use cases are expressed with portable rids. Either the portable rid comes from the user, or it's the one from the SDK. For the latter, rather than mapping NETCoreSdkRuntimeIdentifier via the rid graph, NETCoreSdkPortableRuntimeIdentifier can be used. There was a problem hiding this comment. On a linux-musl-x64 system, should this use linux-x64 assets? Or would those be binary incompatible? There was a problem hiding this comment. We can't use |
||
| <Output TaskParameter="MatchingRid" PropertyName="_DashboardAndDcpRID" /> | ||
| </PickBestRid> | ||
|
|
||
| <!-- Now that we have the version, we add the package references --> | ||
| <ItemGroup> | ||
| <PackageReference Include="Aspire.Dashboard.Sdk.$(_DashboardAndDcpRID)" Version="$(_AppHostVersion)" IsImplicitlyDefined="true" /> | ||
|
|
||
This file contains hidden or 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
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.
I chose ns2.0 so that we should work in VS and in CLI. I've tested both and that does seem to be the case.