-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(#45761): Stop forwarding dotnet-watch run arguments to all commands #50558
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
base: release/10.0.1xx
Are you sure you want to change the base?
fix(#45761): Stop forwarding dotnet-watch run arguments to all commands #50558
Conversation
|
@dotnet-policy-service agree |
| var optionResult = (OptionResult)child; | ||
|
|
||
| // skip watch options: | ||
| if (!watchOptions.Contains(optionResult.Option)) |
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.
Inverting the logic allows a continue to short-circuit the loop.
| } | ||
|
|
||
| // skip forwarding run options to other commands. | ||
| if (isRunCommand == false && forwardToRunOptions.Contains(optionResult.Option)) |
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 meat and potatoes of this PR. Prevents options which only work with the run command from being passed to other commands.
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: !isRunCommand
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.
Can we pass command from the caller and generalize this to if (forwardToSubcommandOptions.Contains(optionResult.Option) && !command.Options.Contains(optionResult.Option)) ?
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.
Of course. Give me a bit and I'll get some commits up.
Further Thoughts
This is outside the scope of this fix, but a different PR could go a step further towards generalization by:
nixing forwardToSubcommandOptions in favor of watchableSubCommandOptions.
watchableSubCommandOptions are options that can show up in subcommands that could affect watch's settings.
For example: run --project and build [<PROJECT>|<SOLUTION>] could both set watch's project path.
| } | ||
| }); | ||
|
|
||
| // Options we need to know about that are passed through to the subcommand: |
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.
add ... if the subcommand supports them
| var longProjectOption = new Option<string>("--project") { Hidden = true, Arity = ArgumentArity.ZeroOrOne, AllowMultipleArgumentsPerToken = false }; | ||
| var launchProfileOption = new Option<string>("--launch-profile", "-lp") { Hidden = true, Arity = ArgumentArity.ZeroOrOne, AllowMultipleArgumentsPerToken = false }; | ||
| var noLaunchProfileOption = new Option<bool>("--no-launch-profile") { Hidden = true, Arity = ArgumentArity.Zero }; | ||
| Option[] forwardToRunOptions = |
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'd call this forwardToSubcommandOptions
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: You can replace https://github.com/dotnet/sdk/pull/50558/files#diff-6a6fa2fea5af87fda145d79165f529cef772a3227d58494098b7a22511c4aecaR96-R99 with
foreach (var option in forwardToSubcommandOptions)
{
rootCommand.Options.Add(option);
}| } | ||
|
|
||
| // skip forwarding run options to other commands. | ||
| if (isRunCommand == false && forwardToRunOptions.Contains(optionResult.Option)) |
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.
This is very concerning to me. Are we ignoring --project completely? That's worse than producing an error.
Also, it means we might be regressing the scenario for the MTP-based dotnet test which actually accepts --project.
@tmat I don't have much context on dotnet watch, but I think we might want to discuss this offline to ensure that we at least are not breaking the MTP-based dotnet test.
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.
@Youssef1313 No, we are not ignoring --project completely. This makes sure it is not passed to the subcommand. dotnet watch still needs it internally.
Also, please look at #45761 as it seems like there is a catch22 here. If we don't fix it, we leave dotnet watch [subcommand] --project broken for most normal dotnet CLI scenarios.
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.
What happens for Microsoft.Testing.Platform then? In that case I think it will need to be passed to test command, similar to run.
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, that's the catch 22.
Looking over docs, it looks like dotnet test is the only command that has a difference in MCP. Is there a way to tell if we're running in the MCP environment? If so, we could add that as a condition here.
But, I dislike the additional brittleness that would add to the check. I'd prefer if we could query for the subcommand's acceptable arguments without running the subcommand. If there is a better way to query the acceptable args, please point them out for me. I'll change course to use that.
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.
MCP is unrelated. MTP is "Microsoft.Testing.Platform" (so only relevant to test command - and is determined by global.json). I think you can just get the test command and look into its options. When you retrieve the test command, it will return the right thing depending on what is there in global.json.
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.
Apologies, MCP is a typo. I meant MTP.
Alright, I think I'm in the right place.
Test's parser has GetCommand
| public static Command GetCommand() |
Even better to go one level up to dotnet's parser with GetBuiltInCommand
Line 189 in f099c19
| public static Command GetBuiltInCommand(string commandName) => |
Then we'd be able to check any subcommand's arguments for pass-through viability.
Fixes: #45761, #45634, and maybe a regression of #34949.
Context
Watch CLI
Problem
dotnet-watchhas-p,--project,--launch-profile, and--no-launch-profileas possible arguments. A user can not use these arguments withoutdotnet-watchforwarding them. These arguments only work with theruncommand, and will cause an error if passed to any command butrun.