Support -getProperty and friends with file-based apps
#50530
Conversation
| private static Option<string[]?> MSBuildMultiOption(string name) | ||
| => new ForwardedOption<string[]?>($"--{name}", $"-{name}", $"/{name}") | ||
| { | ||
| Hidden = true, |
There was a problem hiding this comment.
@zivkan all of these getX options are marked hidden here - they won't show in help output as a result.
There was a problem hiding this comment.
Oh. But don't all "unknown" parameters get forwarded to MSBuild anyway? What's the benefit of having build, clean, pack, etc all define these hidden options? Why not every option listed in dotnet msbuild -h? What makes these 4 special, other than it's the focus of this PR?
There was a problem hiding this comment.
We've been explicitly modeling specific parameters that impact the way MSbuild behaves so that we can detect and mimic that behavior for the file-based apps usage model, which doesn't actually invoke the MSBuild CLI. We also have an issue to model, intercept, and interpret the -bl arg for similar reasons. In general, if the dotnet CLI needs to do some processing of an argument that MSbuild accepts, we need to model, parse, and interpret it based on our needs - which may involve just forwarding it back to MSBuild, but may also mean modifying it in some way, or using it internally, etc.
There was a problem hiding this comment.
It also helps us inside the CLI codebase
a) ensure that we keep certain invariants around key MSbuild properties intact - like making sure that --restoreProperty values are always a superset of --property values, or that a Target isn't set to be built twice in a single invocation
b) helps us correctly parse and handle complex interactions around implicit Restore vs explicit, separate restore for TFM and RID-specific SDK scenarios that MSBuild proper has no opinion on
c) makes it easier for us to do parsing-based testing, because we can assert on a model instead of random raw strings
d) ensures that we can enforce correct-by-construction calls to the lower-level MSbuild forwarding commands for the key CLI commands - we force all MSbuild forwarding commands to use the MSBuildArgs struct so that we can encourage centralized parsing and handling of these structures, and the forwarding app just has to map from the structure to a string[] of args to give to the MSbuild process
e) more reasons that I'm forgetting
in short, it's been a huge boon for the CLI codebase's handling of MSbuild semantics
| var verbosity = MSBuildArgs.Verbosity ?? VerbosityOptions.quiet; | ||
| var consoleLogger = TerminalLogger.CreateTerminalOrConsoleLogger([$"--verbosity:{verbosity}", .. MSBuildArgs.OtherMSBuildArgs]); | ||
| var consoleLogger = minimizeStdOut | ||
| ? new SimpleErrorLogger() |
There was a problem hiding this comment.
the rest of this looks good, but I don't actually know what the behavior of the SimpleErrorLogger is. What does this look like in the case of errors?
There was a problem hiding this comment.
It writes each error on a single line something like this:
It's the exact same logger MSBuild uses in this situation btw
|
@RikkiGibson @333fred for reviews, thanks |
| { | ||
| Log.WriteLine($"File '{file}':"); | ||
| Log.WriteLine(text); | ||
| File.Delete(Path.Join(testInstance.Path, file)); |
There was a problem hiding this comment.
Is this delete needed to clean up the test? Containing directory is not being deleted?
There was a problem hiding this comment.
This is used twice during the test, so the containing directory is kept (and so is the Program.cs file). Only the intermediate files that might be generated by -getResultOutputFile are deleted so we verify they are equivalently emitted again (to check equivalence between file-based and project-based programs).
Resolves #50501.