Skip to content

Ensure all evaluation data are in binlogs from file-based apps #49841

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 19 additions & 20 deletions src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,6 @@ public override int Execute()
}

Dictionary<string, string?> savedEnvironmentVariables = [];
ProjectCollection? projectCollection = null;
try
{
// Set environment variables.
Expand All @@ -191,17 +190,20 @@ public override int Execute()
}

// Set up MSBuild.
ReadOnlySpan<ILogger> binaryLoggers = binaryLogger is null ? [] : [binaryLogger];
projectCollection = new ProjectCollection(
ReadOnlySpan<ILogger> binaryLoggers = binaryLogger is null ? [] : [binaryLogger.Value];
IEnumerable<ILogger> loggers = [.. binaryLoggers, consoleLogger];
var projectCollection = new ProjectCollection(
MSBuildArgs.GlobalProperties,
[.. binaryLoggers, consoleLogger],
loggers,
ToolsetDefinitionLocations.Default);
var parameters = new BuildParameters(projectCollection)
{
Loggers = projectCollection.Loggers,
Loggers = loggers,
LogTaskInputs = binaryLoggers.Length != 0,
};

BuildManager.DefaultBuildManager.BeginBuild(parameters);

// Do a restore first (equivalent to MSBuild's "implicit restore", i.e., `/restore`).
// See https://github.com/dotnet/msbuild/blob/a1c2e7402ef0abe36bf493e395b04dd2cb1b3540/src/MSBuild/XMake.cs#L1838
// and https://github.com/dotnet/msbuild/issues/11519.
Expand All @@ -213,8 +215,6 @@ public override int Execute()
hostServices: null,
BuildRequestDataFlags.ClearCachesAfterBuild | BuildRequestDataFlags.SkipNonexistentTargets | BuildRequestDataFlags.IgnoreMissingEmptyAndInvalidImports | BuildRequestDataFlags.FailOnUnresolvedSdk);

BuildManager.DefaultBuildManager.BeginBuild(parameters);

var restoreResult = BuildManager.DefaultBuildManager.BuildRequest(restoreRequest);
if (restoreResult.OverallResult != BuildResultCode.Success)
{
Expand All @@ -229,12 +229,6 @@ public override int Execute()
CreateProjectInstance(projectCollection),
targetsToBuild: MSBuildArgs.RequestedTargets ?? ["Build"]);

// For some reason we need to BeginBuild after creating BuildRequestData otherwise the binlog doesn't contain Evaluation.
if (NoRestore)
{
BuildManager.DefaultBuildManager.BeginBuild(parameters);
}

var buildResult = BuildManager.DefaultBuildManager.BuildRequest(buildRequest);
if (buildResult.OverallResult != BuildResultCode.Success)
{
Expand All @@ -261,7 +255,7 @@ public override int Execute()
Environment.SetEnvironmentVariable(key, value);
}

binaryLogger?.Shutdown();
binaryLogger?.Value.ReallyShutdown();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is a method on the facade type, where the ordinary shutdown method doesn't do anything on it?

consoleLogger.Shutdown();
}

Expand Down Expand Up @@ -289,7 +283,7 @@ static Action<IDictionary<string, string>> AddRestoreGlobalProperties(ReadOnlyDi
};
}

static ILogger? GetBinaryLogger(IReadOnlyList<string>? args)
static Lazy<FacadeLogger>? GetBinaryLogger(IReadOnlyList<string>? args)
{
if (args is null) return null;
// Like in MSBuild, only the last binary logger is used.
Expand All @@ -298,12 +292,17 @@ static Action<IDictionary<string, string>> AddRestoreGlobalProperties(ReadOnlyDi
var arg = args[i];
if (LoggerUtility.IsBinLogArgument(arg))
{
return new BinaryLogger
// We don't want to create the binlog file until actually needed, hence we wrap this in a Lazy.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not?

return new(() =>
{
Parameters = arg.IndexOf(':') is >= 0 and var index
? arg[(index + 1)..]
: "msbuild.binlog",
};
var logger = new BinaryLogger
{
Parameters = arg.IndexOf(':') is >= 0 and var index
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to worry about the source of args being a mutable list that chances between when the Lazy is created and when it evaluates?

? arg[(index + 1)..]
: "msbuild.binlog",
};
return LoggerUtility.CreateFacadeLogger([logger]);
});
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/Cli/dotnet/LoggerUtility.cs
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,14 @@ internal static class LoggerUtility
// We need a custom logger to handle this, because the MSBuild API for evaluation and execution calls logger Initialize and Shutdown methods, so will not allow us to do this.
if (binaryLoggers.Count > 0)
{
var fakeLogger = ConfigureDispatcher(binaryLoggers);
var fakeLogger = CreateFacadeLogger(binaryLoggers);

return fakeLogger;
}
return null;
}

private static FacadeLogger ConfigureDispatcher(List<BinaryLogger> binaryLoggers)
public static FacadeLogger CreateFacadeLogger(List<BinaryLogger> binaryLoggers)
{
var dispatcher = new PersistentDispatcher(binaryLoggers);
return new FacadeLogger(dispatcher);
Expand Down
4 changes: 2 additions & 2 deletions test/dotnet.Tests/CommandTests/Run/RunFileTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -939,8 +939,8 @@ public void BinaryLog_EvaluationData()
new FileInfo(binaryLogPath).Should().Exist();

var records = BinaryLog.ReadRecords(binaryLogPath).ToList();
records.Any(static r => r.Args is ProjectEvaluationStartedEventArgs).Should().BeTrue();
records.Any(static r => r.Args is ProjectEvaluationFinishedEventArgs).Should().BeTrue();
records.Count(static r => r.Args is ProjectEvaluationStartedEventArgs).Should().Be(2);
records.Count(static r => r.Args is ProjectEvaluationFinishedEventArgs).Should().Be(2);
}

/// <summary>
Expand Down