Skip to content
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

Use repository root for output path calculation when the output isn't a child of the working directory #9805

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

baronfel
Copy link
Member

@baronfel baronfel commented Feb 29, 2024

Fixes #9800

Context

This PR teaches Terminal Logger to look for a specific kind of MSBuild Item for a project during the build. This kind of item (SourceRoot) points to the computed source code repository of the build, which we can use to calculate the output path to render for a project when that project doesn't build to location that's a child of the current directory. Here's what it looks like when building a child project in a repository using the SDK Artifacts path layout:

image

Changes Made

Terminal Logger Projects now keep track of a SourceRoot span. This span is calculated by looking for the first SourceRoot item that has SourceControl metadata on it. This data is filled in by Sourcelink, which is part of the .NET SDK.

Terminal Logger is opted into TaskParameterEventArgs messages via the IEventSource3.IncludeTaskInputs() method.

During output rendering, if the output path isn't a child of the working directory, the SourceRoot is checked as a fallback, if it exists. If so, the relative path between the working directory and the SourceRoot is computed and combined with the portion of the output path that maps to the SourceRoot.

Testing

I need to add testing for this scenario.

Notes

This will only work when /m:1 currently because the messages that contain task inputs are not High importance. To fix this, we will need to make TerminalLogger a proper forwarding logger, not using the ConfigurableForwardingLogger infrastructure. I logged #9806 to capture this need.

Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Overall looks great!

It'd be nice to extract the logic relativizing the path

src/MSBuild/TerminalLogger/TerminalLogger.cs Outdated Show resolved Hide resolved
src/MSBuild/TerminalLogger/TerminalLogger.cs Outdated Show resolved Hide resolved
src/MSBuild/TerminalLogger/Project.cs Outdated Show resolved Hide resolved
src/MSBuild/TerminalLogger/TerminalLogger.cs Show resolved Hide resolved
src/MSBuild/TerminalLogger/TerminalLogger.cs Outdated Show resolved Hide resolved
@baronfel baronfel force-pushed the use-sourceroot-for-relative-paths branch from 97e1be2 to 407bb74 Compare March 19, 2024 19:59
{
foreach (DictionaryEntry property in e.Properties)
{
if (property.Key is "GenerateFullPaths" &&
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a case-insensitive comparison to match e.GlobalProperties which has a case insensitive key comparer?

Copy link
Member Author

@baronfel baronfel Mar 19, 2024

Choose a reason for hiding this comment

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

Oh I'm going nuts on the implementation still trying to find something that consistently works with ProjectEvaluationFinishedEventArgs, I wouldn't check any of this as 'final' yet.

I'm running into a kind of frustrating scenario with this PR switching from ProjectStarted to ProjectEvaluationFinishedEventArgs.

The Project items we create are keyed to specific ProjectContextIds from the BuildEventContext, but those ProjectContextIds are not the same between evaluation and execution. Neither are the ProjectInstanceIds - so I'm not sure what the key could be to derive some information at evaluation time and then use it after execution time.

Copy link
Member

Choose a reason for hiding this comment

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

Have you tried matching on EvaluationId? It looks like the value passed to ProjectStarted matches the one in ProjectEvaluationFinishedEventArgs.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ladipro there's something here that's making me pretty concerned. I put breakpoints on 3 locations in the terminal logger

  • where we handle ProjectEvaluationFinishedEventArgs
  • where we handle ProjectStartedEventArgs
  • where we handle TaskParameterEventArgs

and at no point is there an Id that remains the same across all of them. Here's some data from a recent test run to highlight the problem:

Restore evaluation (ProjectEvaluationFinishedEventArgs)
    {Node=1 Submission=0 ProjectContext=-2 ProjectInstance=-1 Eval=4 Target=-1 Task=-1}
Restore execution (ProjectStarted)
    {Node=1 Submission=0 ProjectContext=4 ProjectInstance=2 Eval=4 Target=-1 Task=-1}
_IsProjectRestoreSupported evaluation (ProjectEvaluationFinishedEventArgs)
    {Node=1 Submission=0 ProjectContext=-2 ProjectInstance=-1 Eval=7 Target=-1 Task=-1}
_IsProjectRestoreSupported execution (ProjectStarted)
    {Node=1 Submission=0 ProjectContext=7 ProjectInstance=3 Eval=7 Target=-1 Task=-1}
_GenerateRestoreProjectPathWalk execution (ProjectStarted)
    {Node=1 Submission=0 ProjectContext=10 ProjectInstance=3 Eval=7 Target=-1 Task=-1}
_IsProjectRestoreSupported execution (ProjectStarted)
    {Node=1 Submission=0 ProjectContext=13 ProjectInstance=3 Eval=7 Target=-1 Task=-1}
_GenerateRestoreGraphProjectEntry execution (ProjectStarted)
    {Node=1 Submission=0 ProjectContext=16 ProjectInstance=3 Eval=7 Target=-1 Task=-1}
_GenerateProjectRestoreGraph execution (ProjectStarted)
    {Node=1 Submission=0 ProjectContext=19 ProjectInstance=3 Eval=7 Target=-1 Task=-1}
'actual' evaluation (ProjectEvaluationFinishedEventArgs)
    {Node=1 Submission=1 ProjectContext=-2 ProjectInstance=-1 Eval=10 Target=-1 Task=-1}
'actual' execution (ProjectStarted)
    {Node=1 Submission=1 ProjectContext=22 ProjectInstance=4 Eval=10 Target=-1 Task=-1}
TaskItem emit (TaskParameterEventArgs) context - Why is Eval -1?
    {Node=1 Submission=1 ProjectContext=22 ProjectInstance=4 Eval=-1 Target=135 Task=-1}

Each line is the build context at each state - my conclusion is there is no single Id that we can use to keep a shared context across evaluation, execution, and arbitrary task messages. I think we might need to keep/track both ProjectInstance and Eval Ids and be able to look up TerminalLogger's Project based on either of those.

Copy link
Member Author

Choose a reason for hiding this comment

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

I figured it out - I made a parallel tracker for data we are interested in from evaluation time (which right now is just TFM) and then use the BuildMessageContext.Eval property as the key for that evaluation data. This is working great and should be a good model for us in the future - we can use the new EvaluationData structure to hold specific data that's relevant just from an evaluation, and the Project structure should contain all data that changes or is computed at target execution time.

src/MSBuild/TerminalLogger/TerminalLogger.cs Outdated Show resolved Hide resolved
@@ -10,16 +15,15 @@ namespace Microsoft.Build.Logging.TerminalLogger;
/// <summary>
/// Represents a project being built.
/// </summary>
[DebuggerDisplay("{OutputPath}({TargetFramework})")]
Copy link
Member

Choose a reason for hiding this comment

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

nit: You've removed the TargetFramework property.

@baronfel baronfel force-pushed the use-sourceroot-for-relative-paths branch from 14af38c to 1b72331 Compare September 6, 2024 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants