KirillOsenkov / MSBuildStructuredLog

A logger for MSBuild that records a structured representation of executed targets, tasks, property and item values.
MIT License
1.42k stars 188 forks source link

Evaluation question: .FindEvaluation() vs. .EvaluationFolder.Children.OfType<ProjectEvaluation>() #599

Closed NickCraver closed 2 years ago

NickCraver commented 2 years ago

In writing some tooling based on the lib, I've found many projects to have an EvaluationId of -1 so Build.FindEvaluation(id) doesn't work, but the evaluation is there if I look up via Build.EvaluationFolder.Children.OfType<ProjectEvaluation>().SingleOrDefault(pe => pe.SourceFilePath == p.SourceFilePath). So currently I'm doing:

var eval = build.FindEvaluation(p.EvaluationId) ?? build.EvaluationFolder.Children.OfType<ProjectEvaluation>().SingleOrDefault(pe => pe.SourceFilePath == p.SourceFilePath);

I guess my questions are:

I see this is populated by build context but haven't dug in yet...figured I'd ask if this was expected. If not, happy to dig - I have a consistent repro :)

KirillOsenkov commented 2 years ago

If you do File -> Statistics, what's the version of the binlog? And what's the version of MSBuild used? (shown at the top of the binlog)

There was a bug in MSBuild where only projects built in the central node were properly associated with their evaluation Id. Feel free to email me the binlog and I can take a look.

In the latest version it is expected that every project has an evaluation associated with it. Projects can get evaluated several times (depending on the set of properties passed, e.g. evaluating with the property TargetFramework=net472 and TargeFramework=net6.0 will produce two distinct evaluations. So by picking an evaluation just by project path you're picking one at random and it's not clear whether you'll get lucky. It'll be fine most of the time since the bulk of eval results is identical across all evaluations of a project.

NickCraver commented 2 years ago

Oh good point - alrighty so MSBuild is 17.2.1.25201 here and binlog version is showing as 14:

image

I think I figured my issue out though - I was crawling via build.VisitAllChildren<Project> and using the first instance where not everything has an evaluation ID (looks like we don't go back and put it everywhere a project exists...should we?). Changing instead to only crawl over p.EvaluationId >= 0 yields what I want and everything is present. So I guess the question mutates to if we should ensure EvaluationId is present on all Project nodes for hyperlinking and such works? Not sure what's involved, but happy to take a look.

Unrelated, but in looking through this comparing target frameworks, I think it'd be nice if those were in the UI for older style projects (<TargetFramework> and <TargetFrameworks> are utilized today, but not <TargetFrameworkVersion>) - thoughts on if we should recognize that? Today, the badge of "net462" beside a project indicates it's also an SDK style by nature, so "fixing" this and translating would remove that...or we could leave it raw and get the benefit of both, but usage downstream may be muddied. Some examples:

Today:

image

With translation:

image

Without translation:

image

After debugging some things, I'd personally love the indicator there and prefer it without translation (or adding something to indicate "old" .csproj...but that could annoy people forever on that system). Thoughts on adding such a thing? I have all versions working - happy to PR!

To be explicit, "translation" is defined as:

// Complete list from https://docs.microsoft.com/en-us/visualstudio/msbuild/msbuild-target-framework-and-target-platform?view=vs-2022#target-framework-and-profile
// Translated to https://docs.microsoft.com/en-us/dotnet/standard/frameworks
project.TargetFramework = kvp.Value switch
{
    "v2.0" => "net20",
    "v3.0" => "net30",
    "v3.5" => "net35",
    "v4.5" => "net45",
    "v4.5.1" => "net451",
    "v4.5.2" => "net452",
    "v4.6" => "net46",
    "v4.6.1" => "net461",
    "v4.6.2" => "net462",
    "v4.7" => "net47",
    "v4.7.1" => "net471",
    "v4.7.2" => "net472",
    "v4.8" => "net48",
    _ => null,
};

...which I wouldn't be wild about committing to code anyway, because it naturally breaks on new versions.

KirillOsenkov commented 2 years ago

So, regarding missing evaluation ID - could you send me a binlog with that? I want to understand why that instance of Project doesn't seem to have it. You do have the latest version of MSBuild so you shouldn't be impacted by that bug any more.

I think I like the badge for TargetFrameworkVersion as well - I didn't add it purely because I didn't work with legacy projects myself much. If you PR that, that'd be great. No reason to not show this info. Without translation seems fine, as it conveys more information (translating erases the fact that this was legacy).

Don't think people care much about whether the project is legacy or not, and if they do, we can find another way to identify that, perhaps an uglier project icon for legacy projects, or a skull with cross bones, or a radiation sign.

NickCraver commented 2 years ago

I had to finish testing a thing before rotating a secret that crept into that binlog but all good now - binlog emailed and #603 open for the badges :)

KirillOsenkov commented 2 years ago

Thanks, I think the conclusion for now is that if the project results were satisfied from the cache and the actual build was never invoked, there's no evaluation Id for those projects and we need to filter them out.