Separate regular arguments from launch tool ("entrypoint") arguments - #18999
Karol Zadora-Przylecki (karolz-ms) merged 13 commits into
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 18999Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 18999" |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Separates tool-entrypoint arguments from program arguments, removing debug registration-order sensitivity while preserving accurate app-model command lines.
Changes:
- Adds entrypoint argument annotations and evaluation.
- Updates DCP launch/fallback composition.
- Migrates Go, Python, and .NET integrations with regression tests.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/Aspire.Hosting/ResourceBuilderExtensions.cs |
Adds entrypoint argument APIs. |
src/Aspire.Hosting/ApplicationModel/EntrypointArgsCallbackAnnotation.cs |
Models entrypoint arguments. |
src/Aspire.Hosting/ApplicationModel/ArgumentsExecutionConfigurationGatherer.cs |
Prepends entrypoint arguments. |
src/Aspire.Hosting/SupportsDebuggingAnnotation.cs |
Removes rewrite tracking. |
src/Aspire.Hosting/Utils/ExtensionUtils.cs |
Matches entrypoints to launch types. |
src/Aspire.Hosting/Dcp/ExecutableCreator.cs |
Composes IDE/process arguments and fallback. |
src/Aspire.Hosting/Dcp/DcpExecutor.cs |
Invalidates entrypoint callback caches. |
src/Aspire.Hosting.Go/GoHostingExtensions.cs |
Separates Go tool and program arguments. |
src/Aspire.Hosting.Python/PythonAppResourceBuilderExtensions.cs |
Separates Python entrypoints. |
src/Aspire.Hosting.Dotnet/DotnetProjectHostingExtensions.cs |
Adapts .NET launch scaffolding. |
tests/Aspire.Hosting.Tests/ExecutableResourceBuilderExtensionTests.cs |
Tests entrypoint semantics. |
tests/Aspire.Hosting.Tests/Dcp/DcpExecutorTests.cs |
Tests DCP composition and fallback. |
tests/Aspire.Hosting.Go.Tests/AddGoAppTests.cs |
Tests Go argument ordering. |
tests/Aspire.Hosting.Python.Tests/AddPythonAppTests.cs |
Tests Python entrypoints. |
tests/Aspire.Hosting.Dotnet.Tests/DotnetProjectResourceTests.cs |
Tests custom .NET entrypoints. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Does this overlap with #18904 - e.g. should How does this overlap with how the dashboard renders args - for dotnet tool resoruce I had to jump through some hoops to hide those arguments, which resulted in some special casing. Would the dashboard still show these exec arguments, or would they be hidden on the main view, and only viewable on the detailed resource properties pane. aspire/src/Aspire.Hosting/Dcp/ExecutableCreator.cs Lines 670 to 678 in 30190f9 |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/Aspire.Hosting/Dcp/ExecutableCreator.cs:280
- This still disables the DCP Process fallback whenever a non-empty entrypoint is withheld. That leaves the affected Go/Python-style resources with
FallbackExecutionTypes = null, contradicting the PR description's claim that separating these arguments fixes the permanently forfeited Process fallback. Please retain a separate process-form argument list (or otherwise teach DCP to compose the full command on fallback); otherwise an IDE launch rejection still cannot fall back to Process.
if (spec.ExecutionType != ExecutionType.IDE || omittedEntrypointArgumentCount > 0)
{
return false;
src/Aspire.Hosting/ApplicationModel/ArgumentsExecutionConfigurationGatherer.cs:57
- This suppresses entrypoint arguments for every
ContainerResource, even though the new publicWithEntrypointArgs<T>API accepts anyIResourceWithArgs—including ordinary containers—and documents that the prefix is present outside the owning IDE launch. A direct call onAddContainer(...)therefore succeeds but silently contributes no arguments. Please scope this workaround specifically to containers synthesized byPublishAsDockerFile(for example via an internal marker), or reject container builders at the API boundary instead of dropping their configuration.
if (resource is ContainerResource)
{
return;
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (5)
src/Aspire.Hosting/ApplicationModel/ArgumentsExecutionConfigurationGatherer.cs:57
WithEntrypointArgsis available onContainerResourcethroughIResourceWithArgs, but this unconditional return discards explicitly declared entrypoint arguments for every container in both run and publish modes. It also contradicts the new API contract that these arguments participate in process/publish command lines. Skip only annotations inherited byPublishAsDockerFile(or remove that inherited annotation during conversion), rather than disabling the feature for all containers.
if (resource is ContainerResource)
{
return;
src/Aspire.Hosting/ResourceBuilderExtensions.cs:4784
- This constraint exposes the API on
ProjectResourceandDotnetProjectResource, but their non-debug process paths still emit Aspire's defaultdotnet run --project/--filescaffold. BecauseSupportsDebuggingis false outside a debug session, a matching custom entrypoint is composed together with that scaffold instead of replacing it, so it is not the leading tool invocation promised here and can produce a brokendotnetcommand. Suppress the project scaffold whenever the matching entrypoint declaration owns it in process mode as well, and cover both project resource shapes without a debug session.
public static IResourceBuilder<T> WithEntrypointArgs<T>(this IResourceBuilder<T> builder, string launchConfigurationType, Action<CommandLineArgsCallbackContext> callback)
where T : IResourceWithArgs
src/Aspire.Hosting/ApplicationModel/EntrypointArgsCallbackAnnotation.cs:32
- Resource dependency discovery still evaluates only
CommandLineArgsCallbackAnnotation(ResourceExtensions.cs:1601-1625). Values such asEndpointReferenceorReferenceExpressionadded through this new callback therefore disappear fromGetResourceDependenciesAsync; for containers, that can prevent required host-tunnel setup. Include the active entrypoint callback in raw argument dependency discovery and add coverage for anIValueWithReferencesentrypoint argument.
internal sealed class EntrypointArgsCallbackAnnotation : IResourceAnnotation, IArgCallbackAnnotation
src/Aspire.Hosting/ApplicationModel/ArgumentsExecutionConfigurationGatherer.cs:80
- The shipped
ProcessArgumentValuesAsyncpath still callsGatherArgumentValuesAsync, which enumerates onlyCommandLineArgsCallbackAnnotation(ResourceExtensions.cs:267-288). After Go and Python move their prefixes to this new gatherer path, that public helper silently reports only program arguments instead of the resource's full command line. Route the legacy helper throughExecutionConfigurationBuilderor teach it to prepend the active entrypoint callback result.
var entrypointArgs = await entrypointAnnotation.AsCallbackAnnotation().EvaluateOnceAsync(entrypointContext).ConfigureAwait(false);
if (entrypointArgs.Count == 0)
{
return;
}
context.Arguments.InsertRange(0, entrypointArgs);
context.AddAdditionalData(new EntrypointArgumentsData(entrypointArgs.Count));
src/Aspire.Hosting/ResourceBuilderExtensions.cs:4780
- This new public fluent API has no
<example>/<code>section, despite the repository XML documentation standard requiring examples for extension methods and the PR checklist stating that one was added. Add a concrete example showing the tool prefix declared separately from ordinaryWithArgsvalues.
/// <para>
/// Calling this method more than once is allowed; the most recent declaration wins.
/// </para>
/// </remarks>
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
7e541bb to
0ce4fd3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/Aspire.Hosting/ApplicationModel/ArgumentsExecutionConfigurationGatherer.cs:57
ContainerResourcealso implementsIResourceWithArgs, so this blanket return silently drops entrypoint arguments explicitly declared on ordinary containers and from their published manifests. That contradictsWithEntrypointArgs' documented process/publish behavior. Suppress or remove the mirrored annotation specifically duringPublishAsDockerFileconversion instead of disabling the API for every container.
if (resource is ContainerResource)
{
return;
src/Aspire.Hosting/Dcp/ExecutableCreator.cs:306
- This unconditionally treats a
ProjectResourceas fallback-capable when the owned entrypoint resolves empty. For a custom debug type, preparation has already omitted the normaldotnet run --projectscaffold, so both the advertised Process fallback and the launch-configuration failure path can execute baredotnet <app-args>. Treat an owned empty entrypoint as an incomplete process command forProjectResourcetoo.
if (modelResource is ProjectResource)
{
return true;
}
src/Aspire.Hosting.Python/PythonAppResourceBuilderExtensions.cs:1068
WithEntrypointis a shipped API whose previous contract explicitly reset all command-line arguments. Replacing that clear callback with a separate prefix callback now retains arguments registered before this call, so switching a script/module can pass stale arguments to the new entrypoint. Preserve the reset for ordinary arguments, or handle this as an intentional breaking API change with migration guidance.
builder.WithEntrypointArgs("python", static context =>
This comment has been minimized.
This comment has been minimized.
|
Alex Crome (@afscrome) I saw your comment, still thinking/working on how to reconcile this with #18904 |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (5)
src/Aspire.Hosting/ResourceBuilderExtensions.cs:4794
- This is a new, non-trivial fluent API, but its public XML documentation has no usage example or
<code>block. Add a minimal example showing how the prefix andownedByLaunchConfigurationTypeare declared so integration authors can use the ownership contract correctly.
/// </remarks>
src/Aspire.Hosting/Dcp/ExecutableCreator.cs:242
- This broad catch also handles cancellation from the launch-configuration producer and converts it into Process fallback. Cancellation should terminate resource creation rather than changing execution mode; exclude cancellation requested through
cancellationTokenfrom the fallback catch.
catch (Exception ex)
src/Aspire.Hosting/ResourceBuilderExtensions.cs:4769
- The new public API throws for null
builder/callbackand an emptyownedByLaunchConfigurationType, but its XML documentation omits those exceptions. Document both so IntelliSense describes the validation contract.
This issue also appears on line 4794 of the same file.
/// <returns>A reference to the <see cref="IResourceBuilder{T}"/>.</returns>
src/Aspire.Hosting/ApplicationModel/DebugSupportExtensions.cs:82
- This public helper validates both parameters with
ThrowIfNull, but the XML documentation does not describe the resulting exception. Add the missing exception contract for IntelliSense consumers.
/// <returns><see langword="true"/> when the launch configuration supplies the tool invocation; otherwise, <see langword="false"/>.</returns>
src/Aspire.Hosting/Dcp/ExecutableCreator.cs:784
DotnetToolResourcereturns from the special branch above before this per-prefix logic runs. SinceWithLaunchToolArgsaccepts everyIResourceWithArgs, a dotnet tool with matching debug support will still pass its owned prefix to the IDE, andshowInCommandLineis ignored. Apply the executable/display decisions in the dotnet-tool branch as well and add coverage for that resource type.
launchArgs.AddRange(appHostArgList.Select((a, i) =>
|
Alex Crome (@afscrome) I have merged a change that should address your concerns. AI-supplied details follow, but hopefully they make sense. Good questions — yes, it overlaps, and deliberately so. I've reshaped this PR in response so the two fit together. The way I'd frame it: #18904 is the API-shape question (which Worth noting: Go already lands on option 2 after this PR. Should
|
runs (Spec.Args) |
Source column | details pane | |
|---|---|---|---|
DotnetTool prefix, today |
yes | hidden | visible |
| launch tool args, IDE-owned | no | visible | absent |
| launch tool args, process execution | yes | visible | visible |
So to answer directly: these args stay on the main view and are not pushed to the properties pane only. The difference from DotnetTool is defensible rather than accidental — when the IDE owns the prefix it isn't in Spec.Args, so it's absent from EffectiveArgs too. Hiding it from the Source column as well would erase run ./cmd/api from the dashboard entirely and the user would see a bare go plus app args. Your hiding is lossless because the dotnet tool exec prefix does run — it's still there in the details pane.
After this change that's a parameter on the declaration instead of something you infer from two call sites in ExecutableCreator, so DotnetTool can migrate and keep its current rendering by passing showInCommandLine: false.
One thing you may want to know about
The -- scan has a latent bug:
var argSeparator = appHostArgList.Select((a, i) => (index: i, value: a.Value))
.FirstOrDefault(x => x.value == DotnetToolResourceExtensions.ArgumentSeparator);
var args = appHostArgList.Select((a, i) => (arg: a, display: i > argSeparator.index));When there's no -- in the list, FirstOrDefault returns default — (index: 0, value: null) — so i > 0 hides argument 0. Reachable whenever the tool annotation has been removed (BuildToolExecArguments returns early and emits no separator) but the resource still has args. A structural count can't drift this way, so it goes away on migration.
Summary of what's left for #18904
- The additive, user-facing
WithXyzArgslayer (WithToolArgs,WithCargoArgs, …) - A framework-emitted segment separator, so integrations stop hand-rolling
-- DotnetToolResourcemigrating off the textual--scan- A decision on Go's
WithAppArgsnow that plainWithArgsdoes the right thing there
Happy to fold any of that into this PR instead if you'd rather see the whole shape at once — I kept it out to keep this one reviewable and behavior-preserving.
|
Doing final code review check... |
Karol Zadora-Przylecki (karolz-ms)
left a comment
There was a problem hiding this comment.
Found one correctness issue in cross-consumer launch-tool argument composition.
Karol Zadora-Przylecki (karolz-ms)
left a comment
There was a problem hiding this comment.
Reviewed the current state of the branch (HEAD 7116888), skipping items already raised and addressed in earlier threads.
2 findings: 1 correctness bug, 1 test-coverage gap.
Everything else I traced checks out: FallbackExecutionTypes parity with the three removed preparation-time sites, restart cache invalidation for the new annotation (DcpExecutor.ForgetCachedCallbackResults), the manifest path (goes through ExecutionConfigurationBuilder, so it gets the prefix), the deliberate container skip, the DotnetToolResource built-in-invocation index scans, and the WithEntrypoint refactor in the Python integration.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c2832706-3a43-42eb-a327-d853af5ea638
Tests selector (audit mode)The full test matrix and all jobs still run in audit mode. The tests and jobs below are what selective CI would run under enforcement. 48 / 100 test projects · 4 jobs, from 26 changed files. Selected test projects (48 / 100)
Selected jobs (4)
How these were chosen — grouped by what changed
🔧 show 38
🧪 🔧 📦 affected project 🔧 🔧 🔧 🧪 🧪 🧪 🧪 🧪 🧪 🧪 Job reasons
Selection computed for commit |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Aspire.Hosting/ResourceBuilderExtensions.cs:4810
- The new public fluent API has no
<example>/<code>block, despite the PR checklist marking that documentation complete. Its replacement and launch-configuration ownership semantics are non-obvious, so add a compilable example that shows the tool prefix and matchingWithDebugSupporttype.
/// </remarks>
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
9f58a1a
into
main
|
/backport to release/13.5 |
|
Started backporting to |
|
Pull request created: #1458
|
|
📝 Documentation has been drafted in microsoft/aspire.dev#1458 targeting Updated the
Note This draft PR needs human review before merging. |
Description
WithDebugSupport'sargsCallbacksubtracted the tool entrypoint (go run <pkg>,python -m <mod>) via an ordinaryWithArgscallback — so it only worked if registered after the callback that added the prefix, it left the app model IDE-only, and it permanently forfeited Process fallback.This fixes the issue by separating the "prefix" (a.k.a. "entrypoint") arguments from ordinary arguments.
Fixes #18929
Checklist
<remarks />and<code />elements on your triple slash comments?