-
Notifications
You must be signed in to change notification settings - Fork 417
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
Fix RID regression by adding a task that calculates the best matching RID for platform #5695
Conversation
… RID for each platform.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Looking at the dependencies this adds, I think it would be better to put the I understand a reason not to do this is that you want this to run on the .NET 8 SDK which wouldn't have the new task. I would consider adding the task in .NET 9 and using it from the SDK if running on the .NET 9 SDK. Then when you move to requiring the .NET 9 SDK, you can drop the extra dependencies and the copy of the SDKs code for handling the RID graph. #Resolved |
Thanks for the review folks. After chatting with @dsplaisted more on this offline, we agreed that in order to not have to deal with the task of making sure dependencies used by the task match the ones built-in on the SDK/Visual Studio the best approach should be to run out-of-proc. I will push some more changes here that will essentially move from an msbuild task to a cli tool instead which is invoked by our targets. The long-term goal for 10.0 will be to move this logic into dotnet/sdk instead. #Resolved |
@@ -0,0 +1,16 @@ | |||
<Project Sdk="Microsoft.NET.Sdk"> |
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.
Nesting .csprojs under folders that already contain a .csproj is typically not a good idea.
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, though in this case the parent project is a NoTargets
SDK project meaning it doesn't compile anything, and the child project doesn't have any targets that could be packed by the parent by accident either. Main reason why I made it a child project, is that the parent is the one that packs this. I could change it of course, but this "feels" better. There is also precedent in dotnet/runtime where source generator projects are nested under the package that packs them.
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 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.
in dotnet/runtime, it is the src
project that packs them, which is a sibling, not in a parent directory.
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.
Oh -- fair enough. I don't feel strongly about this but I can change it if you want me to.
src/Aspire.Hosting.Sdk/Aspire.RuntimeIdentifier.Tool/Program.cs
Outdated
Show resolved
Hide resolved
return rootCommand.Parse(args).Invoke(); | ||
} | ||
|
||
private static string[]? ParseSupportedRidsArgument(ArgumentResult result) |
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'm surprised we need to write custom code for this.
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 followed precedent that has been done on other tools: https://github.com/dotnet/sdk/blob/f007eb76461e4fff93c54e3ea2af67ad4eefeb32/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompat.Tool/Program.cs#L362-L371
src/Aspire.Hosting.Sdk/Aspire.RuntimeIdentifier.Tool/Program.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting.Sdk/Aspire.RuntimeIdentifier.Tool/Program.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting.Sdk/Aspire.RuntimeIdentifier.Tool/Aspire.RuntimeIdentifier.Tool.csproj
Show resolved
Hide resolved
/backport to release/8.2 |
Started backporting to release/8.2: https://github.com/dotnet/aspire/actions/runs/10948758588 |
@joperezr backporting to release/8.2 failed, the patch most likely resulted in conflicts: $ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: Fix RID regression by adding a task that calculates the best matching RID for each platform.
.git/rebase-apply/patch:94: trailing whitespace.
<!-- Don't go higher than binding redirects entries in the toolset MSBuild.exe.config file. -->
.git/rebase-apply/patch:104: trailing whitespace.
.git/rebase-apply/patch:111: trailing whitespace.
.git/rebase-apply/patch:116: trailing whitespace.
.git/rebase-apply/patch:127: trailing whitespace.
warning: squelched 8 whitespace errors
warning: 13 lines add whitespace errors.
Using index info to reconstruct a base tree...
M Aspire.sln
M Directory.Build.targets
M Directory.Packages.props
Falling back to patching base and 3-way merge...
Auto-merging Directory.Packages.props
Auto-merging Directory.Build.targets
CONFLICT (content): Merge conflict in Directory.Build.targets
Auto-merging Aspire.sln
CONFLICT (content): Merge conflict in Aspire.sln
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Fix RID regression by adding a task that calculates the best matching RID for each platform.
Error: The process '/usr/bin/git' failed with exit code 128 Please backport manually! |
@joperezr an error occurred while backporting to release/8.2, please check the run log for details! Error: git am failed, most likely due to a merge conflict. |
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.
LGTM. Thanks!
Description
In Aspire, we have always just cross-compiled our Dashbarod and DCP packages for a limited set of RIDs (win-x64, win-x86, win-arm64, linux-x64, linux-arm64, osx-x64, and osx-arm64). When running on platforms that had special RIDs that we weren't cross-compiling for (for example, rhel.8) then we were relying on the SDK's logic to automatically pick the best closest Dashboard and DCP pack for that RID.
However, in Aspire 8.2.0 we removed Dashboard and DCP from the workload and instead just added some sdk targets that were adding these references on the fly, but we were not doing this special matching logic the the SDK used to do and instead we were simply trying to use the current RID, which caused a regression in all of those platforms that have special RIDs which we don't cross-compile for.
In this PR, we are copying the logic that the SDK does for doing that matching, and we are calling it inside of our targets in order to ensure that we are going to get a valid package reference, which will fix the regression in those systems. This has been manually validated in RedHat 8, which was the original report for this regression. This fix, will get backported into release/8.2 branch, and serviced in 8.2.1.
Fixes #5486
Fixes #5674
cc: @DamianEdwards @radical @eerhardt @danegsta @dsplaisted
Checklist
<remarks />
and<code />
elements on your triple slash comments?Microsoft Reviewers: Open in CodeFlow