Allow MSBuildWorkspace to output a binary log #42319

jmarolf · 2020-03-10T16:55:37Z

THIS ADDS A NEW API!!!!

In the course of human events, it sometimes becomes necessary to get a binlog in order to understand what has gone wrong.
Tools like dotnet-format (or anything using MSBuildWorkspace) are unable to get a binlog today.

Moves the msbuild dependencies from 15.1 -> 15.3 (the release BinaryLogger was added)
Add a new api to allow Solution/Project load to output a binary log

JoeRobich

Seems pretty straightforward.

src/Workspaces/Core/MSBuild/MSBuild/MSBuildWorkspace.cs

JoeRobich · 2020-03-10T22:56:50Z

@jasonmalinowski Any thoughts about adding a binary logging options to MSBuildWorkspace?

src/Workspaces/Core/MSBuild/MSBuild/MSBuildProjectLoader.cs

jasonmalinowski

As called out, this needs better API to either specify the log path, or (my preference) just let you pass the ILoggers directly so they can choose any number of logging formats with appropriate logging levels, etc.

src/Workspaces/Core/MSBuild/MSBuild/MSBuildProjectLoader.cs

jmarolf · 2020-03-16T17:30:50Z

Updated this PR to let you pass in a logger during project or solution load mirroring the way the api works for progress reporting

I still kept the version of MSBuild at 15.3 because its unclear to me what our back-compat story is here. Famously the csc task does not work with older versions of MSBuild. It seems like we may be guiding users down the wrong path by not taking dependencies on newer versions of MSBuild if that allows them to get into a state where the msbuild they use (and therefore the compiler) is older than the language features they may want to use.

updated PR with suggested changes

jasonmalinowski

It appears this is missing a back-compat method since adding new parameter is a breaking change.

src/Workspaces/Core/MSBuild/MSBuild/Build/ProjectBuildManager.cs

src/Workspaces/Core/MSBuild/MSBuild/MSBuildWorkspace.cs

src/Workspaces/Core/MSBuild/MSBuild/MSBuildProjectLoader.Worker.cs

src/Workspaces/Core/MSBuild/MSBuild/MSBuildProjectLoader.cs

RikkiGibson

I wish I could approve a PR just on the basis of the PR description making me laugh

made requested changes

jmarolf · 2020-03-26T02:11:09Z

alright @jasonmalinowski can you take a look at this when you get the chance?

jmarolf · 2020-03-30T23:00:37Z

@jasonmalinowski I have the horrifying ability to merge this at any time. Please stop me.

cartermp · 2020-03-30T23:04:36Z

So do I

JoeRobich

I think maybe some cut and paste errors got in

src/Workspaces/Core/MSBuild/MSBuild/MSBuildProjectLoader.Worker_ResolveReferences.cs

src/Workspaces/Core/MSBuild/MSBuild/MSBuildProjectLoader.Worker.cs

src/Workspaces/Core/MSBuild/MSBuild/MSBuildProjectLoader.Worker_ResolveReferences.cs

…r_ResolveReferences.cs Co-Authored-By: Joey Robichaud <joseph.robichaud@microsoft.com>

JoeRobich

Looks good to me. Just had one question

JoeRobich · 2020-03-31T16:42:02Z

.editorconfig

@@ -39,6 +39,7 @@ indent_size = 2
 [*.{cs,vb}]
 # Sort using and Import directives with System.* appearing first
 dotnet_sort_system_directives_first = true
+dotnet_separate_import_directive_groups = false


Missed this earlier, is it intentional?

yeah, I get yelled at if I add spaces to the usings so this is an implicit style that wasn't captured in the editorcofig. I see no place where separators in usings exist for roslyn

jasonmalinowski

Thanks @jmarolf for the work! Sorry I had a review started last night and functionally this all looks great...

jasonmalinowski · 2020-03-31T22:46:28Z

src/Workspaces/Core/MSBuild/MSBuild/Build/ProjectBuildManager.cs

@@ -186,7 +192,7 @@ public void EndBatchBuild()
            MSB.Execution.BuildManager.DefaultBuildManager.EndBuild();

            // unload project so collection will release global strings
-            _batchBuildProjectCollection.UnloadAllProjects();
+            _batchBuildProjectCollection?.UnloadAllProjects();


Why a ?. here? It feels like a Contract.ThrowIfNull() is more appropriate because we shouldn't have been able to get here without a build already going, right?

yeah, I agree. I'll create a mop up PR

In reply to: 401259147 [](ancestors = 401259147)

jasonmalinowski · 2020-03-31T22:50:15Z

src/Workspaces/Core/MSBuild/MSBuild/Build/ProjectBuildManager.cs

-        private static async Task<(MSB.Evaluation.Project project, DiagnosticLog log)> LoadProjectAsync(
-            string path, MSB.Evaluation.ProjectCollection projectCollection, CancellationToken cancellationToken)
+        private static async Task<(MSB.Evaluation.Project? project, DiagnosticLog log)> LoadProjectAsync(
+            string path, MSB.Evaluation.ProjectCollection? projectCollection, CancellationToken cancellationToken)


When could projectCollection ever be null?

its nulled out and re-allocated at the start of each Start/End build. so it can easily be null if things are ever called slightly out of order. How does roslyn typically handle complex lifetimes like this that the compiler can't track? I was inclined to say that if your object has a complex lifetime beyond some kind of Initialize method then it should be considered nullable. Happy to refactor this so the lifetime isn't so complex though.

In reply to: 401260471 [](ancestors = 401260471)

jasonmalinowski · 2020-03-31T22:54:19Z

src/Workspaces/Core/MSBuild/MSBuild/MSBuildProjectLoader.cs

@@ -153,7 +158,7 @@ private DiagnosticReportingMode GetReportingModeForUnrecognizedProjects()
            if (!_pathResolver.TryGetAbsoluteSolutionPath(solutionFilePath, baseDirectory: Directory.GetCurrentDirectory(), DiagnosticReportingMode.Throw, out var absoluteSolutionPath))
            {
                // TryGetAbsoluteSolutionPath should throw before we get here.
-                return null;
+                return null!;


¯_(ツ)_/¯

In reply to: 401261909 [](ancestors = 401261909)

jmarolf requested review from a team as code owners March 10, 2020 16:55

jmarolf mentioned this pull request Mar 10, 2020

adding ability to produce a binlog dotnet/format#605

Merged

JoeRobich approved these changes Mar 10, 2020

View reviewed changes

src/Workspaces/Core/MSBuild/MSBuild/MSBuildWorkspace.cs Outdated Show resolved Hide resolved

jaredpar reviewed Mar 10, 2020

View reviewed changes

src/Workspaces/Core/MSBuild/MSBuild/MSBuildProjectLoader.cs Outdated Show resolved Hide resolved

jmarolf mentioned this pull request Mar 11, 2020

Advice about msbuild task warnings dotnet/format#584

Open

jasonmalinowski previously requested changes Mar 11, 2020

View reviewed changes

src/Workspaces/Core/MSBuild/MSBuild/MSBuildProjectLoader.cs Outdated Show resolved Hide resolved

jmarolf requested review from jasonmalinowski and JoeRobich March 16, 2020 17:23

jmarolf force-pushed the feature/ouput-binary-log-for-msbuildworkspace branch from 59a3938 to 02bed3a Compare March 16, 2020 17:54

jinujoseph added the Area-IDE label Mar 17, 2020

jasonmalinowski previously requested changes Mar 18, 2020

View reviewed changes

RikkiGibson reviewed Mar 23, 2020

View reviewed changes

jmarolf added 9 commits March 23, 2020 16:33

upgrade MSBuild to a version that contains BinaryLogger

4980272

add API to produce binary Log

3d641d8

allow user to pass in custom msbuild logger

dc48489

fix up package references

12c9715

remove newlines

827eb12

update editorconfig to change using style

e65f144

nullable annotate methods

49ccccb

updating constructors for back-compat

997e5db

have ProjectBuildManager own logging

20c6a60

jmarolf force-pushed the feature/ouput-binary-log-for-msbuildworkspace branch from 203938d to 20c6a60 Compare March 23, 2020 23:33

skip test

e134698

JoeRobich requested changes Mar 31, 2020

View reviewed changes

jmarolf and others added 5 commits March 30, 2020 19:42

Update src/Workspaces/Core/MSBuild/MSBuild/MSBuildProjectLoader.Worke…

85471fe

…r_ResolveReferences.cs Co-Authored-By: Joey Robichaud <joseph.robichaud@microsoft.com>

Update src/Workspaces/Core/MSBuild/MSBuild/MSBuildProjectLoader.Worke…

f5fbec2

…r_ResolveReferences.cs Co-Authored-By: Joey Robichaud <joseph.robichaud@microsoft.com>

Update src/Workspaces/Core/MSBuild/MSBuild/MSBuildProjectLoader.Worke…

975b649

…r_ResolveReferences.cs Co-Authored-By: Joey Robichaud <joseph.robichaud@microsoft.com>

Update src/Workspaces/Core/MSBuild/MSBuild/MSBuildProjectLoader.Worke…

3942482

…r_ResolveReferences.cs Co-Authored-By: Joey Robichaud <joseph.robichaud@microsoft.com>

reduce indentation

11b67ef

jmarolf requested a review from JoeRobich March 31, 2020 14:33

JoeRobich approved these changes Mar 31, 2020

View reviewed changes

jmarolf merged commit 6f01902 into dotnet:master Mar 31, 2020

jmarolf deleted the feature/ouput-binary-log-for-msbuildworkspace branch March 31, 2020 17:43

ghost added this to the Next milestone Mar 31, 2020

jasonmalinowski reviewed Mar 31, 2020

View reviewed changes

sharwell modified the milestones: Next, temp, 16.6.P3 Apr 6, 2020

jasonmalinowski mentioned this pull request Apr 17, 2020

Addition of ILogger-accepting method in MSBuildWorkspace is a source breaking change #43437

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow MSBuildWorkspace to output a binary log #42319

Allow MSBuildWorkspace to output a binary log #42319

jmarolf commented Mar 10, 2020

JoeRobich left a comment

JoeRobich commented Mar 10, 2020

jasonmalinowski left a comment

jmarolf commented Mar 16, 2020

jasonmalinowski left a comment

RikkiGibson left a comment

jmarolf commented Mar 26, 2020

jmarolf commented Mar 30, 2020

cartermp commented Mar 30, 2020

JoeRobich left a comment

JoeRobich left a comment

JoeRobich Mar 31, 2020

jmarolf Mar 31, 2020

jasonmalinowski left a comment

jasonmalinowski Mar 31, 2020

jmarolf Apr 1, 2020

jasonmalinowski Mar 31, 2020

jmarolf Apr 1, 2020

jasonmalinowski Mar 31, 2020

jmarolf Apr 1, 2020

Allow MSBuildWorkspace to output a binary log #42319

Allow MSBuildWorkspace to output a binary log #42319

Conversation

jmarolf commented Mar 10, 2020

THIS ADDS A NEW API!!!!

JoeRobich left a comment

Choose a reason for hiding this comment

JoeRobich commented Mar 10, 2020

jasonmalinowski left a comment

Choose a reason for hiding this comment

jmarolf commented Mar 16, 2020

jasonmalinowski left a comment

Choose a reason for hiding this comment

RikkiGibson left a comment

Choose a reason for hiding this comment

jmarolf commented Mar 26, 2020

jmarolf commented Mar 30, 2020

cartermp commented Mar 30, 2020

JoeRobich left a comment

Choose a reason for hiding this comment

JoeRobich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmalinowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment