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.

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

Matcher.Match(string) regression between 8.0.0 and 9.0.0 Preview 1 #100762

Closed
ViktorHofer opened this issue Apr 8, 2024 · 3 comments · Fixed by #101083
Closed

Matcher.Match(string) regression between 8.0.0 and 9.0.0 Preview 1 #100762

ViktorHofer opened this issue Apr 8, 2024 · 3 comments · Fixed by #101083

Comments

@ViktorHofer
Copy link
Member

ViktorHofer commented Apr 8, 2024

Description

The breaking change notice lists the affected API but based on the below sample, the following API is also impacted:

public static Microsoft.Extensions.FileSystemGlobbing.PatternMatchingResult Match (this Microsoft.Extensions.FileSystemGlobbing.Matcher matcher, string file);

This blocks the VMR's smoke tests from upgrading to a newer version of M.E.FileSystemGlobbing: https://github.com/dotnet/installer/blob/962083ecfff385af0a76aa510fe4bef0d1a820fe/src/SourceBuild/content/test/Microsoft.DotNet.SourceBuild.SmokeTests/Utilities.cs#L31-L41

Note that only the Matcher API usage with the rootDir overload in the VMR smoke tests was called out in the PR that changed this behavior.

The following documentation line was also called out in the PR:

files IEnumerable
Collection of file names. If relative paths rootDir will be prepended to the paths.

I believe this isn't accurate. The InMemoryDirectoryInfo.FullName property is set to rootDir but not the files field that holds the passed in files: https://github.com/dotnet/runtime/pull/94528/files#diff-fb4fcf4a62cfcaf3eb5bb701c276f5b63f750dbf0a171c07692223fe36a5a2c2L53

cc @jozkee

Reproduction Steps

Program.cs

using Microsoft.Extensions.FileSystemGlobbing;

Matcher matcher = new();
matcher.AddInclude("./sdk/9.0.100-preview.4.24207.1/.version"); // For simplicity this is the same path as below but in the actual source, this uses a wilcard instead of the hardcoded version.

Console.WriteLine("Matches: " + matcher.Match("./sdk/9.0.100-preview.4.24207.1/.version").HasMatches.ToString());
// Prints false with Microsoft.Extensions.FileSystemGlobbing/9.0.0-preview.2.24128.5
// Prints true with Microsoft.Extensions.FileSystemGlobbing/8.0.0

app.csproj

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net9.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
  </PropertyGroup>

  <ItemGroup>
    <!-- <PackageReference Include="Microsoft.Extensions.FileSystemGlobbing" Version="8.0.0" /> -->
    <PackageReference Include="Microsoft.Extensions.FileSystemGlobbing" Version="9.0.0-preview.2.24128.5" />
  </ItemGroup>

</Project>

Expected behavior

When matching on relative paths in memory without supplying a rootDir, matches should be found.

Actual behavior

When matching on relative paths in memory without supplying a rootDir, matches aren't found because the rootDir (CWD) is now always prepended.

Regression?

Yes, relevant change: #94528

Known Workarounds

Change both the include pattern and the match string to a rooted path.

Configuration

No response

Other information

No response

Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-extensions-filesystem
See info in area-owners.md if you want to be subscribed.

@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Apr 8, 2024
@jeffhandley jeffhandley added this to the 9.0.0 milestone Apr 8, 2024
@dotnet-policy-service dotnet-policy-service bot removed the untriaged New issue has not been triaged by the area owner label Apr 8, 2024
@jozkee
Copy link
Member

jozkee commented Apr 15, 2024

Apologize for the inconvenience this has caused you. The API you mention should've not been affected by #94528.

What's causing file to not match is the leading ./. I excluded relative paths from using GetFullPath in order to decouple them from CWD. However, GetFullPath should still be used on relative paths (combined with rootDir, instead of CWD) so normalization provided by GetFullPath can remove those redundant segments, which are not accounted for in subsequent code.

@ViktorHofer
Copy link
Member Author

Sounds good. Should we add the bug label?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants