Add limited support for binarylogger to MSBuildWorkspace#80120
Add limited support for binarylogger to MSBuildWorkspace#80120
Conversation
With the move to using a separate BuildHost we lost binlog support in MSBuildWorkspace. The issue being serializing arbitrary MSBuild ILogger implementations to send OOP. This change check the logger's type and if it is BinaryLogger we send it across using a BinLogPathProvider.
|
Does this fix #72202? |
| return logger?.GetType().FullName == "Microsoft.Build.Logging.BinaryLogger"; | ||
| } | ||
|
|
||
| private sealed class DefaultBinLogPathProvider(string? logFilePath) : IBinLogPathProvider |
There was a problem hiding this comment.
Aren't there other parameters we also need to deal with for the BinaryLogger that might end up here?
There was a problem hiding this comment.
From the BinaryLogger docs
The only supported parameter is the output log file path (for example, "msbuild.binlog").
|
One of our partners hit a scenario where this option would help diagnose an unexpected behavior with |
Co-authored-by: Rikki Gibson <rigibson@microsoft.com>
| return logger?.GetType().FullName == "Microsoft.Build.Logging.BinaryLogger"; | ||
| } | ||
|
|
||
| internal sealed class DefaultBinLogPathProvider : IBinLogPathProvider |
There was a problem hiding this comment.
I admit it's not entirely clear to me what the "Default" is doing in this type name.
There was a problem hiding this comment.
Yes, having an interface and a default impl seems like overkill here.
See if this feature might be helpful:
https://github.com/dotnet/msbuild/pull/11910/files
The optional LogFile specifies the path where the binary log is saved. To generate a distinct log file for each build, the token "{}" can be added into the path, for example: LogFile=output-{}-log.binlog. Each "{}" in the log path is replaced with a unique string using the timestamp, running process Id and random string stamp.
| var newLogPaths = Enumerable.Range(0, 10) | ||
| .Select(_ => provider.GetNewLogPath()) | ||
| .ToImmutableHashSet(); | ||
| Assert.Equal(10, newLogPaths.Count); | ||
|
|
||
| foreach (var newLogPath in newLogPaths) | ||
| { | ||
| var newLogDirectory = Path.GetDirectoryName(newLogPath); | ||
| var newLogFileName = Path.GetFileNameWithoutExtension(newLogPath); | ||
| var newLogExtension = Path.GetExtension(newLogPath); | ||
|
|
||
| Assert.Equal(LogDirectory, newLogDirectory); | ||
| Assert.StartsWith(LogFileName, newLogFileName); | ||
| Assert.Equal(DefaultExtension, newLogExtension); | ||
| } |
There was a problem hiding this comment.
Are these all the same asserts? Can we switch this to just be a theory with the different inputs of the file name?
|
|
||
| var buildHostProcessManager = new BuildHostProcessManager(Properties, loggerFactory: _loggerFactory); | ||
| var binLogPathProvider = IsBinaryLogger(msbuildLogger) | ||
| ? new DefaultBinLogPathProvider(msbuildLogger.Parameters!) |
There was a problem hiding this comment.
Note it's possible for the parameters to contain more than just the file path:
I'm almost tempted to say we should just reflect on the logger, and if it has a FilePath property just use it, and fall back to using the Parameters property if it's not present. (FYI to @KirillOsenkov)
There was a problem hiding this comment.
The only supported parameter is the output log file path (for example, "msbuild.binlog")
The word "supported" is doing a lot of work. (see doc)
There was a problem hiding this comment.
Yeah the doc just needs to be updated. Seems safe to just set the FilePath, if you don't set it at all it will default to msbuild.binlog. There's a feature where if you include {} in the file path it will generate a unique name for the binlog.
Co-authored-by: Jason Malinowski <jason.malinowski@microsoft.com>
With the move to using a separate BuildHost we lost binlog support in MSBuildWorkspace. The issue being that we need to serialize arbitrary MSBuild ILogger implementations to send OOP. This change checks if the logger is a BinaryLogger and sends it across using a BinLogPathProvider.
fixes #72202