-
Notifications
You must be signed in to change notification settings - Fork 800
[release/13.1] Fix Aspire.ProjectTemplates build failure from command line length limits #14009
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
- Added System.CommandLine package directive to replace-text.cs - Rewrote the script to use System.CommandLine's Option and RootCommand APIs - System.CommandLine automatically supports .rsp files via @ prefix - Updated Aspire.ProjectTemplates.csproj to generate .rsp file with arguments - Changed script invocation to pass .rsp file instead of command-line arguments - This prevents command line length issues when building templates on Windows Co-authored-by: DamianEdwards <[email protected]>
- Replace Environment.Exit() calls with proper return statements - Add try-catch block around the entire action handler - Return exit codes from the action handler instead of calling Environment.Exit() - This allows proper cleanup and makes the script more testable Co-authored-by: DamianEdwards <[email protected]>
…I expanding .rsp The dotnet CLI itself uses System.CommandLine and expands .rsp files before passing arguments, which means we still hit the command line limit when using 'dotnet script.cs'. Now the build process: 1. Builds the script to an executable using 'dotnet build' 2. Runs the compiled executable directly with '@response.rsp' 3. This avoids the dotnet CLI intermediary and prevents .rsp expansion The compiled executable is placed in artifacts/bin/replace-text/Debug/ Co-authored-by: DamianEdwards <[email protected]>
Co-authored-by: DamianEdwards <[email protected]>
Per feedback, the simpler approach is to use 'dotnet script.cs -- @response.rsp' where the '--' tells dotnet CLI to pass arguments directly to the script without interpretation. This avoids the CLI expanding the .rsp file and hitting command line limits. The script still needs to be built once (dotnet does this automatically when running .cs files), so we keep the build step to ensure it's compiled before use. Co-authored-by: DamianEdwards <[email protected]>
The build step is not needed. When using 'dotnet script.cs -- @response.rsp', dotnet automatically compiles and caches the script as needed. The explicit build step was redundant. Co-authored-by: DamianEdwards <[email protected]>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 14009Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 14009" |
| // Validate paths | ||
| if (paths.Count == 0) | ||
| { | ||
| Console.Error.WriteLine("Error: No file paths provided. Use --files to specify paths."); |
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.
Pretty minor, I'm assuming System.CommandLine validates this but we are losing this custom error that provides a useful hint, but I guess that's ok.
| <CommandArgs Include="--replacements" /> | ||
| <CommandArgs Include="$(ReplacementsArgs)" /> | ||
| <_RspLines Include="--files" /> | ||
| <_RspLines Include="@(SourceFiles)" /> |
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: should these be quoted?
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.
@DamianEdwards might be worth to consider this in main
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.
.rsp files are line delimited, so there is no need to quote paths since the whole path is on a single line and the whole line is treated as a single string.
|
Left a couple of nits, but this looks good otherwise and it is really just tell mode given it is infra only. |
|
@joperezr thanks, but I can't merge this 😄 |
Backport of #13578 to release/13.1
/cc @DamianEdwards @copilot
Customer Impact
Testing
Risk
Regression?