CmdLine parsing was extracted from XMake and the implementation is visible to dotnet #12704
Conversation
src/MSBuild/XMake.cs
Outdated
| try | ||
| { | ||
| GatherAllSwitches(commandLine, out var switchesFromAutoResponseFile, out var switchesNotFromAutoResponseFile, out string fullCommandLine); | ||
| CommandLineParser.GatherAllSwitches(commandLine, out var switchesFromAutoResponseFile, out var switchesNotFromAutoResponseFile, out string fullCommandLine, out s_exeName); |
There was a problem hiding this comment.
The new signature of GatherAllSwitches outputs exeName as the 5th parameter, but this is directly assigned to the static field s_exeName. This creates an implicit side effect where the method both returns data and modifies static state. Consider having the method return exeName as part of a result tuple or record, and let the caller assign it to s_exeName. This would make the method more testable and its behavior more explicit.
| CommandLineParser.GatherAllSwitches(commandLine, out var switchesFromAutoResponseFile, out var switchesNotFromAutoResponseFile, out string fullCommandLine, out s_exeName); | |
| CommandLineParser.GatherAllSwitches(commandLine, out var switchesFromAutoResponseFile, out var switchesNotFromAutoResponseFile, out string fullCommandLine, out string exeName); | |
| s_exeName = exeName; |
There was a problem hiding this comment.
@rainersigwald, I kept the change minimal—s_exeName is only used once, and that’s when diagnostic verbosity is enabled and ConsoleLogger is active. I could either store the value in the parser or find another way to retrieve it without parsing the command line.
There was a problem hiding this comment.
Anything new in this file, or all clean moves?
There was a problem hiding this comment.
Clean moves except assigning the s_exeName and getting the s_exePath value (I added a comment that the line was copied). IIRC, 3 lines of code were changed slightly.
There was a problem hiding this comment.
I have concerns about the usage of static members here - MSBuild is often hosted in other environments that are not one-shot in the way that MSBuild.exe is, and if they use this API repeatedly (e.g. for preparing multiple separate builds over time), they will mutate that state.
There was a problem hiding this comment.
The main method here is GatherAllSwitches, which resets the state. I get your concern since all internal methods are exposed, so I’ve made the parser instantiable.
There was a problem hiding this comment.
I also removed usingSwitchesFromAutoResponseFile as this field wasn't used anywhere.
The `usingSwitchesFromAutoResponseFile` field has been removed from the `CommandLineParser` class, along with the associated logic that set its value. This includes the removal of the check for `switchesFromAutoResponseFile.HaveAnySwitchesBeenSet()` and the assignment to this field. The `ResetGatheringSwitchesState` method has been updated to no longer reset this field, as it is no longer needed. These changes simplify the class by eliminating redundant state management related to auto-response file switches, improving maintainability.
| /// <param name="switchesNotFromAutoResponseFile"></param> | ||
| /// <param name="fullCommandLine"></param> | ||
| /// <returns>Combined bag of switches.</returns> | ||
| internal void GatherAllSwitches( |
There was a problem hiding this comment.
The dotnet CLI callers of this aren't going to have a string, they have string[] - it would be ideal if this API exposed only arrays of args so that we didn't have to keep converting back and forth. That may bring too much churn though.
There was a problem hiding this comment.
I’ve added a new Parse method that should be a bit more user-friendly 🙂. The parsing logic could definitely use a proper overhaul, but for now this is a quick and temporary fix.
| /// <param name="switchesNotFromAutoResponseFile"></param> | ||
| /// <param name="fullCommandLine"></param> | ||
| /// <returns>Combined bag of switches.</returns> | ||
| internal void GatherAllSwitches( |
Context
In some cases dotnet needs to process MSBuild cmdline switches including response files.
Changes Made
The parsing logic was pulled out from XMake to isolate it in the internal type, making it easily reusable in the .NET SDK project.
Testing
All existing tests are passing.
Notes
The goal of this PR isn’t to refactor the entire API—that’s something we can consider tackling later on.