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

Merged
merged 3 commits into from
Jul 21, 2025

Conversation

jjonescz
Copy link
Member

@jjonescz jjonescz commented Jul 17, 2025

Better fix than #49588.
Currently only restore evaluation data are in the binlogs, not build evaluation data - this PR fixes that.

@jjonescz jjonescz added the Area-run-file Items related to the "dotnet run <file>" effort label Jul 17, 2025
@jjonescz jjonescz force-pushed the sprint-binlog-eval-2 branch from 662e468 to a13354b Compare July 17, 2025 18:04
@jjonescz jjonescz marked this pull request as ready for review July 17, 2025 18:23
@Copilot Copilot AI review requested due to automatic review settings July 17, 2025 18:23
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR ensures that both restore and build evaluation data are properly captured in binary logs for file-based applications. Previously, only restore evaluation data was being logged to binlogs, missing build evaluation data.

  • Restructures project creation to occur before BeginBuild() is called to ensure evaluation data is captured
  • Updates test assertions to verify exactly 2 evaluation events (restore + build) instead of just checking for existence
  • Removes conditional BeginBuild() calls and consolidates to a single call after project creation

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs Restructures project creation and build initialization to ensure evaluation data is captured in binlogs
test/dotnet.Tests/CommandTests/Run/RunFileTests.cs Updates test to verify exactly 2 evaluation events are logged

@jjonescz jjonescz requested a review from a team July 17, 2025 18:24
@jjonescz jjonescz marked this pull request as draft July 18, 2025 08:46
};
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?

Copy link
Member Author

@jjonescz jjonescz Jul 21, 2025

Choose a reason for hiding this comment

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

That wouldn't be a problem since the whole args array isn't captured inside the Lazy, only the one concrete immutable string arg is.

@@ -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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because commands like dotnet run --no-build -bl should not create the binlog file (or at least that's the existing behavior and there are tests verifying it).

@@ -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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, exactly. The facade type is already used like this in other code paths of the RunCommand.

@jjonescz jjonescz merged commit c997920 into dotnet:main Jul 21, 2025
28 checks passed
@jjonescz jjonescz deleted the sprint-binlog-eval-2 branch July 21, 2025 09:45
@jjonescz
Copy link
Member Author

/backport to release/10.0.1xx-preview7

Copy link
Contributor

Started backporting to release/10.0.1xx-preview7: https://github.com/dotnet/sdk/actions/runs/16448729729

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-run-file Items related to the "dotnet run <file>" effort
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants