Skip to content

Commit

Permalink
Keep path in invalid project exceptions (#1287)
Browse files Browse the repository at this point in the history
When #1004 moved the standard XML reading approach to be stream-based
rather than file-based, the exceptions thrown on malformed XML
changed--System.Xml no longer knows the path, so it isn't included in the
XmlException. That caused MSBuild to fail to report the location of the
XML error in a nice way as it had done before.

Almost every case where we constructed a BuildEventFileInfo object already
had access to the full path, so I added a constructor that accepted that
as an argument and overrides the possibly-empty path returned from
XmlException.SourceUri.

Added a unit test to verify that the information is indeed preserved in the exception.

Fixes #1286.
  • Loading branch information
rainersigwald authored Oct 31, 2016
1 parent fd25bc9 commit 4e405a8
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 2 deletions.
10 changes: 10 additions & 0 deletions src/Shared/BuildEventFileInfo.cs
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,16 @@ internal BuildEventFileInfo(XmlException e)
_endColumn = 0;
}

/// <summary>
/// Creates an instance of this class using the information in the given XmlException and file location.
/// </summary>
internal BuildEventFileInfo(string file, XmlException e) : this(e)
{
ErrorUtilities.VerifyThrowArgumentNull(file, nameof(file));

_file = file;
}

#endregion

#region Properties
Expand Down
2 changes: 1 addition & 1 deletion src/XMakeBuildEngine/Construction/ProjectRootElement.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2059,7 +2059,7 @@ private XmlDocumentWithLocation LoadDocument(string fullPath, bool preserveForma

if (xmlException != null)
{
fileInfo = new BuildEventFileInfo(xmlException);
fileInfo = new BuildEventFileInfo(fullPath, xmlException);
}
else
{
Expand Down
2 changes: 1 addition & 1 deletion src/XMakeBuildEngine/Definition/Toolset.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1046,7 +1046,7 @@ private void LoadAndRegisterFromTasksFile(string searchPath, string[] defaultTas
catch (XmlException e)
{
// handle XML errors in the default tasks file
ProjectFileErrorUtilities.VerifyThrowInvalidProjectFile(false, new BuildEventFileInfo(e), taskFileError, e.Message);
ProjectFileErrorUtilities.VerifyThrowInvalidProjectFile(false, new BuildEventFileInfo(defaultTasksFile, e), taskFileError, e.Message);
}
catch (Exception e) when (ExceptionHandling.IsIoRelatedException(e))
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,5 +82,36 @@ public void ErrorCodeShouldAppearForCircularDependency()
File.Delete(file);
}
}

/// <summary>
/// Regression test for https://github.com/Microsoft/msbuild/issues/1286
/// </summary>
[Fact]
public void LogErrorShouldHavePathAndLocation()
{
string file = Path.GetTempPath() + Guid.NewGuid().ToString("N");

try
{
File.WriteAllText(file, ObjectModelHelpers.CleanupFileContents(@"
<Project ToolsVersion='msbuilddefaulttoolsversion' xmlns='msbuildnamespace'>
<Target Name=[invalid] />
</Project>"));

var _ = ObjectModelHelpers.BuildTempProjectFileExpectFailure(file);

Assert.True(false, "Loading an invalid project should have thrown an InvalidProjectFileException.");
}
catch (InvalidProjectFileException e)
{
Assert.Equal(3, e.LineNumber);
Assert.Equal(38, e.ColumnNumber);
Assert.Equal(file, e.ProjectFile); // https://github.com/Microsoft/msbuild/issues/1286
}
finally
{
File.Delete(file);
}
}
}
}

0 comments on commit 4e405a8

Please sign in to comment.