Fix issue with list of warnings or errors to treat differently can be added to the list multiple times#2671
Conversation
… added to the list multiple times When building two or more projects in the same build episode, the list of warnings to treat as errors or warnings to treat as messages is parsed multiple times. This fixes the issue by checking if the list is already populated for a given project instance. Fixes dotnet#2667
radical
left a comment
radical
left a comment
There was a problem hiding this comment.
It would be nice to explain in the commit message why it is expected and OK for these to be added more than once.
| try | ||
| { | ||
| ObjectModelHelpers.CreateFileInTempProjectDirectory("project1.proj", $@" | ||
| <Project ToolsVersion=""msbuilddefaulttoolsversion"" xmlns=""msbuildnamespace""> |
There was a problem hiding this comment.
Do we need the ToolsVersion and xmlns attributes here?
| /// https://github.com/Microsoft/msbuild/issues/2667 | ||
| /// </summary> | ||
| [Fact] | ||
| public void TreatWarningsAsMessagesWhenBuildingSameProjectMultipleTimes() |
There was a problem hiding this comment.
Maybe another test for WarningsAsErrors, to be thorough?
| { | ||
| try | ||
| { | ||
| ObjectModelHelpers.CreateFileInTempProjectDirectory("project1.proj", $@" |
There was a problem hiding this comment.
Nit: Consider using the brand new TestEnvironment for test isolation :)
Add unit test for warnings as errors
:sadface:
| CreatedFiles = Helpers.CreateFilesInDirectory(TestRoot, files); | ||
| } | ||
|
|
||
| internal MockLogger BuildProjectExpectFailure(IDictionary<string, string> globalProperties = null, string toolsVersion = null) |
There was a problem hiding this comment.
Nitpicking a bit, I guess. Why internal and not public? This is a tests only class.
There was a problem hiding this comment.
Also, are these two wrappers useful? And it's not obvious from the name that this would return a logger.
There was a problem hiding this comment.
MockLogger is internal so the methods had to be internal as well. These methods reflect other functionality we have in ObjectModelHelpers
|
As-is this would target 15.6. That ok? We can go through escrow if needed for 15.5. |
|
@radical you mention this is failing NuGet restore in some cases. Do you know how big of an impact we're going to see? I'm assuming its low since you're the first person to report it... |
|
@jeffkl I am able to reproduce this with a simple solution which has project A depending on a library project B. And B has |
|
Thanks @radical. We're going to attempt to get this in 15.5 pending approval |
… added to the list multiple times (dotnet#2671) * Fix issue with list of warnings or errors to treat differently can be added to the list multiple times When building two or more projects in the same build episode, the list of warnings to treat as errors or warnings to treat as messages is parsed multiple times. This fixes the issue by checking if the list is already populated for a given project instance. Fixes dotnet#2667
… added to the list multiple times (#2671) (#2704) * Fix issue with list of warnings or errors to treat differently can be added to the list multiple times When building two or more projects in the same build episode, the list of warnings to treat as errors or warnings to treat as messages is parsed multiple times. This fixes the issue by checking if the list is already populated for a given project instance. Fixes #2667
5 participants
When building two or more projects in the same build episode, the list of warnings to treat as errors or warnings to treat as messages is parsed multiple times.
This fixes the issue by checking if the list is already populated for a given project instance.
Fixes #2667