-
Notifications
You must be signed in to change notification settings - Fork 418
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
Improve MSBuild discovery for future scenarios #1328
Changes from all commits
931aa68
e675db1
f70205e
7456116
c320ddf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,9 +77,27 @@ private static Dictionary<string, string> CreateGlobalProperties( | |
|
||
var projectInstance = evaluatedProject.CreateProjectInstance(); | ||
var msbuildLogger = new MSBuildLogger(_logger); | ||
|
||
var loggers = new List<MSB.Framework.ILogger>() | ||
{ | ||
msbuildLogger | ||
}; | ||
|
||
if (_options.GenerateBinaryLogs) | ||
{ | ||
var binlogPath = Path.ChangeExtension(projectInstance.FullPath, ".binlog"); | ||
var binaryLogger = new MSB.Logging.BinaryLogger() | ||
{ | ||
CollectProjectImports = MSB.Logging.BinaryLogger.ProjectImportsCollectionMode.Embed, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is actually the default behaviour so maybe we can skip explicitly specifying it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I figured the default behavior was "None": https://github.com/Microsoft/msbuild/blob/master/src/Build/Logging/BinaryLogger/BinaryLogger.cs#L47. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I guess it is the default: https://github.com/Microsoft/msbuild/blob/master/src/Build/Logging/BinaryLogger/BinaryLogger.cs#L68 That's OK, let's keep the code specific so there's no question. I think our use case is definitely to Embed. |
||
Parameters = binlogPath | ||
}; | ||
|
||
loggers.Add(binaryLogger); | ||
} | ||
|
||
var buildResult = projectInstance.Build( | ||
targets: new string[] { TargetNames.Compile, TargetNames.CoreCompile }, | ||
loggers: new[] { msbuildLogger }); | ||
loggers); | ||
|
||
var diagnostics = msbuildLogger.GetDiagnostics(); | ||
|
||
|
@@ -135,13 +153,19 @@ private static void SetTargetFrameworkIfNeeded(MSB.Evaluation.Project evaluatedP | |
} | ||
} | ||
|
||
private static string GetLegalToolsetVersion(string toolsVersion, ICollection<MSB.Evaluation.Toolset> toolsets) | ||
private string GetLegalToolsetVersion(string toolsVersion, ICollection<MSB.Evaluation.Toolset> toolsets) | ||
{ | ||
// It's entirely possible the the toolset specified does not exist. In that case, we'll try to use | ||
// the highest version available. | ||
var version = new Version(toolsVersion); | ||
// Does the expected tools version exist? If so, use it. | ||
foreach (var toolset in toolsets) | ||
{ | ||
if (toolset.ToolsVersion == toolsVersion) | ||
{ | ||
return toolsVersion; | ||
} | ||
} | ||
|
||
// If not, try to find the highest version available and use that instead. | ||
|
||
bool exists = false; | ||
Version highestVersion = null; | ||
|
||
var legalToolsets = new SortedList<Version, MSB.Evaluation.Toolset>(toolsets.Count); | ||
|
@@ -159,25 +183,20 @@ private static string GetLegalToolsetVersion(string toolsVersion, ICollection<MS | |
{ | ||
highestVersion = toolsetVersion; | ||
} | ||
|
||
if (toolsetVersion == version) | ||
{ | ||
exists = true; | ||
} | ||
} | ||
} | ||
|
||
if (highestVersion == null) | ||
if (legalToolsets.Count == 0 || highestVersion == null) | ||
{ | ||
throw new InvalidOperationException("No legal MSBuild toolsets available."); | ||
_logger.LogError($"No legal MSBuild tools available, defaulting to {toolsVersion}."); | ||
return toolsVersion; | ||
} | ||
|
||
if (!exists) | ||
{ | ||
toolsVersion = legalToolsets[highestVersion].ToolsPath; | ||
} | ||
var result = legalToolsets[highestVersion].ToolsVersion; | ||
|
||
_logger.LogInformation($"Using MSBuild tools version: {result}"); | ||
|
||
return toolsVersion; | ||
return result; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a great addition, will definitely be useful in troubleshooting