Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Don't expand full drive globs with false condition #5669

Merged
merged 5 commits into from
Aug 25, 2020
Merged

Don't expand full drive globs with false condition #5669

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
83ba4fe
Don't expand full drive globs with false condition
ladipro Aug 21, 2020
af3d70c
PR feedback: Use TestEnvironment
ladipro Aug 23, 2020
45af1f0
Add an escape hatch
ladipro Aug 23, 2020
585793e
Update src/Build.OM.UnitTests/Definition/ProjectItem_Tests.cs
ladipro Aug 24, 2020
3b76fd8
PR feedback: Make full FS scan detection fully slash-agnostic
ladipro Aug 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
40 changes: 40 additions & 0 deletions src/Build.OM.UnitTests/Definition/ProjectItem_Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,46 @@ public void RecursiveDirWithSemicolonSeparatedInclude()
}
}

[Theory]
[InlineData(@"<i Condition='false' Include='\**\*.cs'/>")]
[InlineData(@"<i Condition='false' Include='/**/*.cs'/>")]
public void FullFileSystemScanGlobWithFalseCondition(string itemDefinition)
{
IList<ProjectItem> items = ObjectModelHelpers.GetItemsFromFragment(itemDefinition, allItems: false, ignoreCondition: true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we break this at some point in the future, it will seem like a hang rather than a failing test. Do you think it would be reasonable to add a timeout?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not in favor of adding explicit timeouts to tests. My reasons are as follows:

  1. [Philosophical] While some tests might seem more susceptible to hangs than others, it's as easy to break something by causing it to hang as it is to break it by causing it to return a wrong result. The test infra should have a reasonable default for the maximum duration, applied to all tests. Guessing how much time something should take is guaranteed to backfire - tests run on stronger/weaker hardware, in parallel, and so on. A lot of non-determinism in play for us to be able to claim that "this test should take a maximum of 10 seconds, otherwise it is a hang".

  2. [Technical] The hang we're expecting in this test case would have the program spend an excessive amount of time in the Project constructor. A long-running synchronous method is an API problem, let alone a long-running constructor. I guess that's an unfortunate legacy we have to live with. If I were to add a timeout, I'd run something like a Thread.Sleep() in parallel, and if the sleep finishes before the test case does, I would:

a) Abort the hanging thread, risking that it leaves the disk or process in a corrupted or inconsistent state.
b) Let the thread run to completion but move on to executing other tests, marking this one failed.

In either case I wouldn't be able to trust the tests that execute after this because something may be amiss or interfering with normal test execution. For better or for worse, in general a hanging test case kills the entire test run.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do log all long test executions, though we don't currently have a hard timeout.

items.ShouldBeEmpty();
}

[Theory]
[InlineData(@"<i Condition='false' Include='somedir\**\*.cs'/>")]
[InlineData(@"<i Condition='false' Include='somedir/**/*.cs'/>")]
public void PartialFileSystemScanGlobWithFalseCondition(string itemDefinition)
{
string directory = null;
string file = null;

try
{
directory = Path.Combine(Path.GetTempPath(), "somedir");
if (File.Exists(directory))
{
File.Delete(directory);
}

file = Path.Combine(directory, "a.cs");
Directory.CreateDirectory(directory);

File.WriteAllText(file, String.Empty);

IList<ProjectItem> items = ObjectModelHelpers.GetItemsFromFragment(itemDefinition.Replace("somedir", directory), allItems: false, ignoreCondition: true);
items.ShouldNotBeEmpty();
}
finally
ladipro marked this conversation as resolved.
Show resolved Hide resolved
{
File.Delete(file);
FileUtilities.DeleteWithoutTrailingBackslash(directory);
}
}

/// <summary>
/// Basic exclude case
/// </summary>
Expand Down
2 changes: 2 additions & 0 deletions src/Build/Evaluation/ItemSpec.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,5 +498,7 @@ public GlobFragment(string textFragment, string projectDirectory)
: base(textFragment, projectDirectory)
{
}

public bool IsFullFileSystemScan => (TextFragment.StartsWith(@"\**\") || TextFragment.StartsWith(@"/**/"));
ladipro marked this conversation as resolved.
Show resolved Hide resolved
}
}
57 changes: 30 additions & 27 deletions src/Build/Evaluation/LazyItemEvaluator.IncludeOperation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,34 +89,37 @@ protected override ImmutableList<I> SelectItems(ImmutableList<ItemData>.Builder
}
else if (fragment is GlobFragment globFragment)
{
string glob = globFragment.TextFragment;

if (excludePatternsForGlobs == null)
{
excludePatternsForGlobs = BuildExcludePatternsForGlobs(globsToIgnore, excludePatterns);
}

string[] includeSplitFilesEscaped;
if (MSBuildEventSource.Log.IsEnabled())
{
MSBuildEventSource.Log.ExpandGlobStart(_rootDirectory, glob, string.Join(", ", excludePatternsForGlobs));
}
using (_lazyEvaluator._evaluationProfiler.TrackGlob(_rootDirectory, glob, excludePatternsForGlobs))
{
includeSplitFilesEscaped = EngineFileUtilities.GetFileListEscaped(
_rootDirectory,
glob,
excludePatternsForGlobs
);
}
if (MSBuildEventSource.Log.IsEnabled())
{
MSBuildEventSource.Log.ExpandGlobStop(_rootDirectory, glob, string.Join(", ", excludePatternsForGlobs));
}

foreach (string includeSplitFileEscaped in includeSplitFilesEscaped)
if (_conditionResult || !globFragment.IsFullFileSystemScan)
{
itemsToAdd.Add(_itemFactory.CreateItem(includeSplitFileEscaped, glob, _itemElement.ContainingProject.FullPath));
string glob = globFragment.TextFragment;

if (excludePatternsForGlobs == null)
{
excludePatternsForGlobs = BuildExcludePatternsForGlobs(globsToIgnore, excludePatterns);
}

string[] includeSplitFilesEscaped;
if (MSBuildEventSource.Log.IsEnabled())
{
MSBuildEventSource.Log.ExpandGlobStart(_rootDirectory, glob, string.Join(", ", excludePatternsForGlobs));
}
using (_lazyEvaluator._evaluationProfiler.TrackGlob(_rootDirectory, glob, excludePatternsForGlobs))
{
includeSplitFilesEscaped = EngineFileUtilities.GetFileListEscaped(
_rootDirectory,
glob,
excludePatternsForGlobs
);
}
if (MSBuildEventSource.Log.IsEnabled())
{
MSBuildEventSource.Log.ExpandGlobStop(_rootDirectory, glob, string.Join(", ", excludePatternsForGlobs));
}

foreach (string includeSplitFileEscaped in includeSplitFilesEscaped)
{
itemsToAdd.Add(_itemFactory.CreateItem(includeSplitFileEscaped, glob, _itemElement.ContainingProject.FullPath));
}
}
}
else
Expand Down
11 changes: 7 additions & 4 deletions src/Shared/UnitTests/ObjectModelHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1042,11 +1042,11 @@ internal static string[] GetTempFiles(int number, DateTime lastWriteTime)
/// <summary>
/// Get items of item type "i" with using the item xml fragment passed in
/// </summary>
internal static IList<ProjectItem> GetItemsFromFragment(string fragment, bool allItems = false)
internal static IList<ProjectItem> GetItemsFromFragment(string fragment, bool allItems = false, bool ignoreCondition = false)
{
string content = FormatProjectContentsWithItemGroupFragment(fragment);

IList<ProjectItem> items = GetItems(content, allItems);
IList<ProjectItem> items = GetItems(content, allItems, ignoreCondition);
return items;
}

Expand All @@ -1058,11 +1058,14 @@ internal static string GetConcatenatedItemsOfType(this Project project, string i
/// <summary>
/// Get the items of type "i" in the project provided
/// </summary>
internal static IList<ProjectItem> GetItems(string content, bool allItems = false)
internal static IList<ProjectItem> GetItems(string content, bool allItems = false, bool ignoreCondition = false)
{
var projectXml = ProjectRootElement.Create(XmlReader.Create(new StringReader(CleanupFileContents(content))));
Project project = new Project(projectXml);
IList<ProjectItem> item = Helpers.MakeList(allItems ? project.Items : project.GetItems("i"));
IList<ProjectItem> item = Helpers.MakeList(
ignoreCondition ?
(allItems ? project.ItemsIgnoringCondition : project.GetItemsIgnoringCondition("i")) :
(allItems ? project.Items : project.GetItems("i")));

return item;
}
Expand Down