Add location string to drive enumerating wildcard warning #7553
Conversation
|
LocationString does include the column + line number, in addition to the file name. And right, I wanted Remove to be separate from Exclude in a second PR |
| @@ -353,6 +354,7 @@ private static void VerifyDriveEnumerationWarningLoggedUponCreateItemExecution(s | |||
| t.Execute().ShouldBeTrue(); | |||
| engine.Warnings.ShouldBe(1); | |||
| engine.AssertLogContains("MSB5029"); | |||
| engine.AssertLogContains($"file {engine.ProjectFileOfTaskNode}"); | |||
There was a problem hiding this comment.
This will fail on a non-English machine; we generally prefer to either assert that the localized resource is present or just
| engine.AssertLogContains($"file {engine.ProjectFileOfTaskNode}"); | |
| engine.AssertLogContains(engine.ProjectFileOfTaskNode); |
| @@ -279,7 +279,7 @@ | |||
| <comment>{StrBegin="MSB5028: "}UE: The project filename is provided separately to loggers.</comment> | |||
| </data> | |||
| <data name="WildcardResultsInDriveEnumeration" xml:space="preserve"> | |||
| <value>MSB5029: The value "{0}" of the "{1}" attribute in element <{2}> is a wildcard that results in enumerating all files on the drive, which was likely not intended. Check that referenced properties are always defined.</value> | |||
| <value>MSB5029: The value "{0}" of the "{1}" attribute in element <{2}> in file {3} is a wildcard that results in enumerating all files on the drive, which was likely not intended. Check that referenced properties are always defined.</value> | |||
There was a problem hiding this comment.
nit: quote?
| <value>MSB5029: The value "{0}" of the "{1}" attribute in element <{2}> in file {3} is a wildcard that results in enumerating all files on the drive, which was likely not intended. Check that referenced properties are always defined.</value> | |
| <value>MSB5029: The value "{0}" of the "{1}" attribute in element <{2}> in file "{3}" is a wildcard that results in enumerating all files on the drive, which was likely not intended. Check that referenced properties are always defined.</value> |
| ProjectCollection projectCollection = new ProjectCollection(); | ||
| MockLogger collectionLogger = new MockLogger(); | ||
| projectCollection.RegisterLogger(collectionLogger); | ||
| Project project = new Project(projectCollection); | ||
| Project project = new Project(testProject.ProjectFile, null, null, projectCollection); |
There was a problem hiding this comment.
What is the behavior on an in-memory project? Is it still reasonable? Should this be a new test instead of a change to the existing one?
There was a problem hiding this comment.
There didn't seem to be any sort of visible effect on the contents of the in-memory project, so I don't think this test is actually reasonable. Reverting to original
| @@ -248,6 +249,7 @@ private static string[] GetFileList | |||
| case ILoggingService loggingService: | |||
| LogDriveEnumerationWarningWithLoggingService( | |||
| loggingService, | |||
| includeLocation, | |||
There was a problem hiding this comment.
why no exclude here when there is on the others?
There was a problem hiding this comment.
It seems like in Evaluator.cs, only the unevaluated Include attributes are looked at without any mention of Exclude specs, so I excluded it from the logger case that Evaluator uses, which is ILoggingService.
mruxmohan4
force-pushed
the
dev/mruxmohan/add-location-string
branch
from
1cd906d
to
e818da4
Compare
Forgind
added
the
merge-when-branch-open
Fixes #7029 by associating a file location string with the WildcardResultsInDriveEnumeration resource name.
Context
For https://github.com/dotnet/project-system/blob/main/build/import/Workarounds.targets#L8, it was difficult to detect the location at which the drive enumerating wildcard pattern was occurring even though a warning was logged, since 1) there was no location string added to the warning, and 2) the Exclude rather than the Remove element was shown in the warning. To resolve 1), it would be better to associate the location string with the
WildcardResultsInDriveEnumerationresource name.Changes Made
Added location string to the
WildcardResultsInDriveEnumerationresource name in order to view the location for drive enumerating wildcard warnings and errors.Testing
Modified ProjectItem, ObjectModelHelper (for ProjectItemInstance), and CreateItem unit tests to ensure that the project file location was placed in the warning or error message.
Notes
Any
<i Remove=<drive enumerating wildcard> />will still be considered as anExcludeattribute in the warning or error message, since is treated as equivalent to<Foo Include=... Exclude=... />, which still makes the warning or error message slightly unclear for drive enumerating wildcard used in Remove cases. However, the wildcarded value will still be caught and logged/thrown based on whether the environment variable is set.