Get eval results 2

[Forgind]

Fixes #3911

(See #3911 (comment) for a spec.)

This has the backbone of the change I want. It's currently missing two parts: tests and formatting.

I also chose not to error when someone specifies getTargetResult without -target because I don't see any reason we shouldn't respect it if someone wants to just have a normal build with the default targets then get the result of some target(s) that built.

I see no reason to add a resultsFile command line switch, as > works great and is already well-known.

I'm also still thinking about what kind of formatting I think is really important. It currently does a simple format like "Property": "value". If we wanted to change it to json, I could support that, since it's easier to parse, but I don't think we should support both, as it's added complexity for minimal gain in my opinion.

Context

Changes Made

If someone specifies getProperty or getItem from the command line without specifying -target, it will return the values of the specified properties or items (including metadata) immediately after evaluation. If someone specifies one of those with -target or -targetResult, it gives that information after the build is finished.

Testing

Manual testing and unit tests

Notes

[Forgind]

Chatted with @baronfel offline, and we decided we can at least start with just supporting the json format and printing out all item metadata, including if its value is an empty string (as is common among built-in metadata).

[JanKrivanek]

FYI @vlada-shubina

[JanKrivanek]

Overall looks good.

I'd like to see the json printing functionality to be separated into standalone type.

Other than that - I feel it's ready to go

[JanKrivanek]

Overall looks good.

I'd like to see the json printing functionality to be separated into standalone type.

Other than that - I feel it's ready to go

[JanKrivanek]

Thanks for separating the outputing code + switching to System.Text.Json.

Overall - it looks like a very helpful feature! Let's ship it and collect feedback!

[rainersigwald]

Chatted with @baronfel offline, and we decided we can at least start with just supporting the json format and printing out all item metadata, including if its value is an empty string (as is common among built-in metadata).

I don't think I like this experience for what I expect to be a dominant use-case here:

❯ .\.dotnet\dotnet.exe msbuild .\src\Samples\PortableTask\PortableTask.csproj -getproperty:OutDir
{
  "Properties": {
    "OutDir": "S:\\msbuild\\artifacts\\bin\\Samples\\PortableTask\\Debug\\netstandard2.0\\"
  }
}

❯ .\.dotnet\dotnet.exe msbuild .\src\Samples\PortableTask\PortableTask.csproj -getproperty:TargetPath
{
  "Properties": {
    "TargetPath": "S:\\msbuild\\artifacts\\bin\\Samples\\PortableTask\\Debug\\netstandard2.0\\PortableTask.dll"
  }
}

That makes using this feature in shell-interpolation contexts much harder than I think it needs to be.

[baronfel]

I was thinking that most users were going to be more the 'using from some kind of tooling' kind (and so would have JSON/jq available for manipulation) vs the shell scripting/CI kind, though admittedly I have no data on that. The use cases we do have (Docker tooling in VSCode, a few internal teams who have reached out with interest) are the former.

For single properties it can feel like overhead, but as soon as you request several properties or items (which these do) then interpolation goes right out the window.

[rainersigwald]

Fascinating, I had the exact opposite impression: the multi-valued thing is the niche make-it-opt-in case and users mostly want clean output to stuff into, e.g. cp -r ${dotnet msbuild -getproperty:OutDir} $STAGING_DIRECTORY.

[rainersigwald]

Can you look into this very-bad error experience (there's a missing </Project> in Warn.proj):

❯ .\.dotnet\dotnet.exe msbuild .\Warn.proj -gettargetresult:Go
MSBUILD : error : This is an unhandled exception in MSBuild -- PLEASE UPVOTE AN EXISTING ISSUE OR FILE A NEW ONE AT https://aka.ms/msbuild/unhandled. [S:\msbuild\Warn.proj]
MSBUILD : error :     System.Xml.XmlException: Unexpected end of file has occurred. The following elements are not closed: Project. Line 11, position 1. [S:\msbuild\Warn.proj]
MSBUILD : error :    at System.Xml.XmlTextReaderImpl.Throw(Exception e) [S:\msbuild\Warn.proj]
MSBUILD : error :    at System.Xml.XmlTextReaderImpl.Throw(String res, String arg) [S:\msbuild\Warn.proj]
MSBUILD : error :    at System.Xml.XmlTextReaderImpl.ParseElementContent() [S:\msbuild\Warn.proj]
MSBUILD : error :    at System.Xml.XmlLoader.LoadNode(Boolean skipOverWhitespace) [S:\msbuild\Warn.proj]
MSBUILD : error :    at System.Xml.XmlLoader.LoadDocSequence(XmlDocument parentDoc) [S:\msbuild\Warn.proj]
MSBUILD : error :    at System.Xml.XmlDocument.Load(XmlReader reader) [S:\msbuild\Warn.proj]
MSBUILD : error :    at Microsoft.Build.Construction.XmlDocumentWithLocation.Load(XmlReader reader) in S:\msbuild\src\Build\ElementLocation\XmlDocumentWithLocation.cs:line 168 [S:\msbuild\
Warn.proj]
MSBUILD : error :    at Microsoft.Build.Construction.ProjectRootElement.LoadDocument(String fullPath, Boolean preserveFormatting, Boolean loadAsReadOnly) in S:\msbuild\src\Build\Constructi
on\ProjectRootElement.cs:line 2084 [S:\msbuild\Warn.proj]
MSBUILD : error :    at Microsoft.Build.Construction.ProjectRootElement..ctor(String path, ProjectRootElementCacheBase projectRootElementCache, Boolean preserveFormatting) in S:\msbuild\sr
c\Build\Construction\ProjectRootElement.cs:line 226 [S:\msbuild\Warn.proj]
MSBUILD : error :    at Microsoft.Build.Construction.ProjectRootElement.CreateProjectFromPath(String projectFile, ProjectRootElementCacheBase projectRootElementCache, Boolean preserveForma
tting) in S:\msbuild\src\Build\Construction\ProjectRootElement.cs:line 2045 [S:\msbuild\Warn.proj]
MSBUILD : error :    at Microsoft.Build.Construction.ProjectRootElement.<>c.<OpenProjectOrSolution>b__209_0(String path, ProjectRootElementCacheBase cache) in S:\msbuild\src\Build\Construc
tion\ProjectRootElement.cs:line 1789 [S:\msbuild\Warn.proj]
MSBUILD : error :    at Microsoft.Build.Evaluation.ProjectRootElementCache.GetOrLoad(String projectFile, OpenProjectRootElement loadProjectRootElement, Boolean isExplicitlyLoaded, Nullable
`1 preserveFormatting) in S:\msbuild\src\Build\Evaluation\ProjectRootElementCache.cs:line 349 [S:\msbuild\Warn.proj]
MSBUILD : error :    at Microsoft.Build.Evaluation.ProjectRootElementCache.Get(String projectFile, OpenProjectRootElement loadProjectRootElement, Boolean isExplicitlyLoaded, Nullable`1 pre
serveFormatting) in S:\msbuild\src\Build\Evaluation\ProjectRootElementCache.cs:line 280 [S:\msbuild\Warn.proj]
MSBUILD : error :    at Microsoft.Build.Construction.ProjectRootElement.OpenProjectOrSolution(String fullPath, IDictionary`2 globalProperties, String toolsVersion, ProjectRootElementCacheB
ase projectRootElementCache, Boolean isExplicitlyLoaded) in S:\msbuild\src\Build\Construction\ProjectRootElement.cs:line 1787 [S:\msbuild\Warn.proj]
MSBUILD : error :    at Microsoft.Build.Execution.ProjectInstance..ctor(String projectFile, IDictionary`2 globalProperties, String toolsVersion, BuildParameters buildParameters, ILoggingSe
rvice loggingService, BuildEventContext buildEventContext, ISdkResolverService sdkResolverService, Int32 submissionId, Nullable`1 projectLoadSettings) in S:\msbuild\src\Build\Instance\Proj
ectInstance.cs:line 516 [S:\msbuild\Warn.proj]
MSBUILD : error :    at Microsoft.Build.BackEnd.BuildRequestConfiguration.<>c__DisplayClass61_0.<LoadProjectIntoConfiguration>b__0() in S:\msbuild\src\Build\BackEnd\Shared\BuildRequestConf
iguration.cs:line 478 [S:\msbuild\Warn.proj]
MSBUILD : error :    at Microsoft.Build.BackEnd.BuildRequestConfiguration.InitializeProject(BuildParameters buildParameters, Func`1 loadProjectFromFile) in S:\msbuild\src\Build\BackEnd\Sha
red\BuildRequestConfiguration.cs:line 503 [S:\msbuild\Warn.proj]
MSBUILD : error :    at Microsoft.Build.BackEnd.BuildRequestConfiguration.LoadProjectIntoConfiguration(IBuildComponentHost componentHost, BuildRequestDataFlags buildRequestDataFlags, Int32
 submissionId, Int32 nodeId) in S:\msbuild\src\Build\BackEnd\Shared\BuildRequestConfiguration.cs:line 438 [S:\msbuild\Warn.proj]
MSBUILD : error :    at Microsoft.Build.BackEnd.RequestBuilder.BuildProject() in S:\msbuild\src\Build\BackEnd\Components\RequestBuilder\RequestBuilder.cs:line 1126 [S:\msbuild\Warn.proj]
MSBUILD : error :    at Microsoft.Build.BackEnd.RequestBuilder.BuildAndReport() in S:\msbuild\src\Build\BackEnd\Components\RequestBuilder\RequestBuilder.cs:line 810 [S:\msbuild\Warn.proj]
MSBUILD : error MSB1025: An internal failure occurred while running MSBuild.
System.Collections.Generic.KeyNotFoundException: The given key 'Go' was not present in the dictionary.
   at System.Collections.Concurrent.ConcurrentDictionary`2.ThrowKeyNotFoundException(TKey key)
   at System.Collections.Concurrent.ConcurrentDictionary`2.get_Item(TKey key)
   at Microsoft.Build.CommandLine.JsonOutputFormatter.AddTargetResultsInJsonFormat(String[] targetNames, BuildResult result) in S:\msbuild\src\MSBuild\JsonOutputFormatter.cs:line 117
   at Microsoft.Build.CommandLine.MSBuildApp.Execute(String[] commandLine) in S:\msbuild\src\MSBuild\XMake.cs:line 868
This is an unhandled exception in MSBuild Engine -- PLEASE OPEN A BUG AGAINST THE MSBUILD TEAM.
System.Collections.Generic.KeyNotFoundException: The given key 'Go' was not present in the dictionary.
   at System.Collections.Concurrent.ConcurrentDictionary`2.ThrowKeyNotFoundException(TKey key)
   at System.Collections.Concurrent.ConcurrentDictionary`2.get_Item(TKey key)
   at Microsoft.Build.CommandLine.JsonOutputFormatter.AddTargetResultsInJsonFormat(String[] targetNames, BuildResult result) in S:\msbuild\src\MSBuild\JsonOutputFormatter.cs:line 117
   at Microsoft.Build.CommandLine.MSBuildApp.Execute(String[] commandLine) in S:\msbuild\src\MSBuild\XMake.cs:line 868
Unhandled exception: System.Collections.Generic.KeyNotFoundException: The given key 'Go' was not present in the dictionary.
   at System.Collections.Concurrent.ConcurrentDictionary`2.ThrowKeyNotFoundException(TKey key)
   at System.Collections.Concurrent.ConcurrentDictionary`2.get_Item(TKey key)
   at Microsoft.Build.CommandLine.JsonOutputFormatter.AddTargetResultsInJsonFormat(String[] targetNames, BuildResult result) in S:\msbuild\src\MSBuild\JsonOutputFormatter.cs:line 117
   at Microsoft.Build.CommandLine.MSBuildApp.Execute(String[] commandLine) in S:\msbuild\src\MSBuild\XMake.cs:line 868
   at Microsoft.Build.CommandLine.MSBuildApp.Main(String[] args) in S:\msbuild\src\MSBuild\XMake.cs:line 266
   at Microsoft.DotNet.Cli.Utils.MSBuildForwardingAppWithoutLogging.ExecuteInProc(String[] arguments)

[rainersigwald]

Even when the project works, if there's an error in the project it doesn't seem great:

❯ .\.dotnet\dotnet.exe msbuild .\Warn.proj -gettargetresult:Go
S:\msbuild\Warn.proj(9,14): error MSB4064: The "Message" parameter is not supported by the "Warning" task loaded from assembly: Microsoft.Build.Tasks.Core, Version=15.1.0.0, Culture=neutra
l, PublicKeyToken=b03f5f7f11d50a3a from the path: S:\msbuild\.dotnet\sdk\7.0.203\Microsoft.Build.Tasks.Core.dll. Verify that the parameter exists on the task, the <UsingTask> points to the
 correct assembly, and it is a settable public instance property.
S:\msbuild\Warn.proj(9,5): error MSB4063: The "Warning" task could not be initialized with its input parameters.
{
  "Target Results": {
    "Go": {
      "Result": "Failure",
      "Items": [
        {},
        {},
        {}
      ]
    }
  }
}

Or a warning (in this thread: Rainer tries to make a project throw a warning):

❯ .\.dotnet\dotnet.exe msbuild .\Warn.proj -gettargetresult:Go
S:\msbuild\Warn.proj(9,5): warning : Everything is awful!
{
  "Target Results": {
    "Go": {
      "Result": "Success",
      "Items": [
        {},
        {},
        {}
      ]
    }
  }
}

Note that this also shows that return values from targets aren't working correctly.

[baronfel]

Some open design questions:

Is JSON the correct format for all invocations of the command?

We have two intended uses cases - quick CI invocations ('where's my output path?') and intense developer tool integrations ('I need this array of properties and items after evaluating the GenerateContainer target'). For the latter, JSON makes sense, but for the former you want something clean.

Proposal: for single-property-only requests, emit the value on stdout unchanged. For all other requests, use JSON

Default Build Target behaviors

The current implementation performs an evaluation if no target is explicitly specified on the command line. To return results after executing a target, the /t flag must be used to provide the target for execution. This differs from the standard behavior for MSBuild invocations - without the new flags calling the command without a target will execute the default target for the project, if one is specified/discovered during evaluation. Is this confusing for end users?

Warning and Error handling

As Rainer shows above, we need to decide what to do with warnings and errors. There are a few options:

• emit warnings and errors to stdout
  • this requires tools to handle this, and hurts the CI/scripting scenario
• emit warnings and errors to stderr
  • this is in line with more CLI tooling expectations, but would be a larger unit of work
• emit warnings and errors as part of the JSON formatted response
  • this would be easy to parse for consumers, but could bloat the response, and coupled with the format-switching behavior above might make it hard to anticipate for CI/scripting scenarios what shape your data would be in.

[ladipro]

No single blocking issue but if we have time I think there is room for improvement.

• There seems to be no getTargetResult test coverage at all.
• A few nits that scream "technical debt" to me.
• Unclear interaction of the new switches with existing switches, e.g. /getProperty:.. /bl silently ignores /bl. Expected?

[Forgind]

Thanks for the review! I pushed a change to take most of your feedback into account 🙂

• There seems to be no getTargetResult test coverage at all.

GetTargetResultSwitchIdentificationTest is intended to test getTargetResult from a switch-processing perspective. I can extend ExecuteAppWithGetPropertyAndItem with some getTargetResult parts, though. I'll do that when I get the chance.

• Unclear interaction of the new switches with existing switches, e.g. /getProperty:.. /bl silently ignores /bl. Expected?

Can you talk more about this one? If you specify msbuild /getProperty:... /bl, it doesn't silently ignore the /bl part, but it also doesn't actually execute a build, so there isn't really much to log, and it doesn't bother outputting a binlog for that. That part is expected. If you run an actual build (i.e., by specifying /getTargetResult or /t), it does create a binlog as far as I can tell.

[ladipro]

Thanks for the review! I pushed a change to take most of your feedback into account 🙂

Thank you for making the changes!

• There seems to be no getTargetResult test coverage at all.

GetTargetResultSwitchIdentificationTest is intended to test getTargetResult from a switch-processing perspective. I can extend ExecuteAppWithGetPropertyAndItem with some getTargetResult parts, though. I'll do that when I get the chance.

Please do, because otherwise 1/3 of the feature (the actual functionality, no just the command line switch) would have no test coverage.

• Unclear interaction of the new switches with existing switches, e.g. /getProperty:.. /bl silently ignores /bl. Expected?

Can you talk more about this one? If you specify msbuild /getProperty:... /bl, it doesn't silently ignore the /bl part, but it also doesn't actually execute a build, so there isn't really much to log, and it doesn't bother outputting a binlog for that. That part is expected. If you run an actual build (i.e., by specifying /getTargetResult or /t), it does create a binlog as far as I can tell.

As a user, I could reasonably expect to combine the /getProperty and /bl switches to create an evaluation-only binlog.

[Forgind]

@ladipro,

I managed to get it to output a binlog even if you don't build, but it isn't quite right: even if everything goes well, the binlog will still claim the build failed.

I talked with baronfel about it just now, and he said that's "in the bucket of 'fix for rc2'", so I pushed the change. I'm working on figuring out how to fix it still, but if it otherwise seems good to you, I'd appreciate merging it without that fixed, and I'll fix it in a follow-up PR.

[ladipro]

Thank you, no issues with pushing binlog support out to RC2. I would still prefer to merge with tests for getTargetResult but feel free to add them later too.

[JanKrivanek]

Re-reviewd the final state - it looks ready to go to me!

[KirillOsenkov]

Do we have a work item to document this new feature in the command-line help?

I'm wondering if it's expected that the feature produces a binlog if the Directory.Build.rsp contains /bl. It makes sense technically but it was unexpected, at least for properties and items. I think it's fine, just wondering if we thought about it.

[Forgind]

Do we have a work item to document this new feature in the command-line help?

@baronfel (or maybe @ghogen?)

I'm wondering if it's expected that the feature produces a binlog if the Directory.Build.rsp contains /bl. It makes sense technically but it was unexpected, at least for properties and items. I think it's fine, just wondering if we thought about it.

That's as-desired for me, at least. It was intentional that if the user specifies /bl in addition to one of these new flags that /bl would still be respected as normal. bl can come in normally through the command line or be added separately, and I don't think we should discriminate between those.

[ladipro]

Filed #9710 to track updating the help message.