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

Make internal directory separator helpers public #31570

Open
cdmihai opened this Issue Aug 2, 2018 · 19 comments

Comments

Projects
None yet
8 participants
@cdmihai
Copy link

commented Aug 2, 2018

TrimEndingDirectorySeparator
EndsInDirectorySeparator

MSbuild has a lot of slash manipulation, and sometimes we append slashes just for comparison purposes. Using these would help cut down on allocations.

API Signatures

namespace System.IO
{
    public static class Path
    {
        public static string TrimEndingDirectorySeparator(string path);
        public static ReadOnlySpan<char> TrimEndingDirectorySeparator(ReadOnlySpan<char> path);
        public static bool EndsInDirectorySeparator(string path);
        public static bool EndsInDirectorySeparator(ReadOnlySpan<char> path);
    }
}

Details

To be consistent none of these will throw. If you pass in a null or empty for the string Trim you will get null or empty pack. Null does not end in separator so EndsIn will return false.

@cdmihai cdmihai closed this Aug 2, 2018

@cdmihai cdmihai changed the title proposal Proposal: Make internal directory separator helpers public Aug 3, 2018

@cdmihai cdmihai reopened this Aug 3, 2018

@cdmihai

This comment has been minimized.

Copy link
Author

commented Aug 3, 2018

@JeremyKuhne

This comment has been minimized.

Copy link
Member

commented Aug 3, 2018

@cdmihai Is this the signature you're asking for?

namespace System.IO
{
    public static class Path
    {
        public static string TrimEndingDirectorySeparator(string path);
        public static ReadOnlySpan<char> TrimEndingDirectorySeparator(ReadOnlySpan<char> path);
        public static bool EndsInDirectorySeparator(string path);
        public static bool EndsInDirectorySeparator(ReadOnlySpan<char> path);
    }
}
@bartonjs

This comment has been minimized.

Copy link
Member

commented Aug 6, 2018

I think we were adding Memory-specific Trim as well (#30592). So probably also

public static ReadOnlyMemory<char> TrimEndingDirectorySeparator(ReadOnlyMemory<char> path);
@danmosemsft

This comment has been minimized.

Copy link
Member

commented Sep 21, 2018

@JeremyKuhne is this ready to mark api-ready-for-review?

@JeremyKuhne

This comment has been minimized.

Copy link
Member

commented Sep 21, 2018

I think we were adding Memory-specific Trim as well (#30592). So probably also

I'd rather tackle Memory in a separate issue as we'd want to have the other path modifying methods overloaded too.

@JeremyKuhne is this ready to mark api-ready-for-review?

I missed the thumbs up. I'll update the ask and change the status.

@terrajobst

This comment has been minimized.

Copy link
Member

commented Sep 25, 2018

The EndsIn* methods seem problematic. The first ones seems fine but we should add support memory.

namespace System.IO
{
    public static class Path
    {
        public static string TrimEndingDirectorySeparator(string path);
        public static ReadOnlySpan<char> TrimEndingDirectorySeparator(ReadOnlySpan<char> path);
        public static ReadOnlyMemory<char> TrimEndingDirectorySeparator(ReadOnlyMemory<char> path);
    }
}
@jbhensley

This comment has been minimized.

Copy link
Collaborator

commented Oct 27, 2018

Just to clarify, is this API approved for this:

public static string TrimEndingDirectorySeparator(string path);
public static ReadOnlySpan<char> TrimEndingDirectorySeparator(ReadOnlySpan<char> path);
public static bool EndsInDirectorySeparator(string path);
public static bool EndsInDirectorySeparator(ReadOnlySpan<char> path);

or this:

public static string TrimEndingDirectorySeparator(string path);
public static ReadOnlySpan<char> TrimEndingDirectorySeparator(ReadOnlySpan<char> path);
public static ReadOnlyMemory<char> TrimEndingDirectorySeparator(ReadOnlyMemory<char> path);
@JeremyKuhne

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

@jbhensley: #31570 (comment) has the approved API:

namespace System.IO
{
    public static class Path
    {
        public static string TrimEndingDirectorySeparator(string path);
        public static ReadOnlySpan<char> TrimEndingDirectorySeparator(ReadOnlySpan<char> path);
        public static ReadOnlyMemory<char> TrimEndingDirectorySeparator(ReadOnlyMemory<char> path);
    }
}
@MarcoRossignoli

This comment has been minimized.

Copy link
Collaborator

commented Oct 31, 2018

@jbhensley are you on this issue or I can tackle it?

@jbhensley

This comment has been minimized.

Copy link
Collaborator

commented Oct 31, 2018

Was going to take a look, but got sidetracked by something else and haven't actually started. All yours.

@MarcoRossignoli

This comment has been minimized.

Copy link
Collaborator

commented Nov 3, 2018

I'm working on this...I got an issue on testing...I followed usual step to build my local CoreClr and added new api on ApiCompat file for netcoreapp to build. When I try to run tests I get:

D:\git\corefx\src\System.Runtime.Extensions
λ msbuild /t:rebuildandtest /v:m
 pushd D:\git\corefx\bin\tests\System.Runtime.Extensions.Tests\netcoreapp-Windows_NT-Debug-x64\                                                                                                                                              
 call D:\git\corefx\bin\testhost\netcoreapp-Windows_NT-Debug-x64\\dotnet.exe xunit.console.dll System.Runtime.Extensions.Tests.dll  -xml testResults.xml -notrait category=nonnetcoreapptests -notrait category=nonwindowstests  -notrait ca 
 tegory=OuterLoop -notrait category=failing                                                                                                                                                                                                  
 popd                                                                                                                                                                                                                                        
 ===========================================================================================================                                                                                                                                 
 Error:                                                                                                                                                                                                                                      
   An assembly specified in the application dependencies manifest (Microsoft.NETCore.App.deps.json) was not found:                                                                                                                           
     package: 'runtime.win-x64.Microsoft.NETCore.App', version: '2.1.3'                                                                                                                                                                      
     path: 'runtimes/win-x64/native/CoreConsole.exe'                                                                                                                                                                                         
 ----- end 11:47:19.12 ----- exit code -2147450740 ----------------------------------------------------------                                                                                                                                
:\git\corefx\Tools\tests.targets(571,5): warning MSB3073: The command "D:\git\corefx\bin/tests/System.Runtime.Extensions.Tests/netcoreapp-Windows_NT-Debug-x64//RunTests.cmd D:\git\corefx\bin/testhost/netcoreapp-Windows_NT-Debug-x64/" ex 
ted with code -2147450740. [D:\git\corefx\src\System.Runtime.Extensions\tests\System.Runtime.Extensions.Tests.csproj]                                                                                                                        
:\git\corefx\Tools\tests.targets(579,5): error : One or more tests failed while running tests from 'System.Runtime.Extensions.Tests' please check D:\git\corefx\bin/tests/System.Runtime.Extensions.Tests/netcoreapp-Windows_NT-Debug-x64/te 
tResults.xml for details! [D:\git\corefx\src\System.Runtime.Extensions\tests\System.Runtime.Extensions.Tests.csproj]                                                                                                                         
 AssemblyResolveTests -> D:\git\corefx\bin\AnyOS.AnyCPU.Debug\AssemblyResolveTests\netstandard\AssemblyResolveTests.dll                                                                                                                      
 Using D:\git\corefx\bin\testhost\netcoreapp-Windows_NT-Debug-x64\ as the test runtime folder.                                                                                                                                               
 Executing in D:\git\corefx\bin\tests\AssemblyResolveTests\netcoreapp-Windows_NT-Debug-x64\                                                                                                                                                  
 ----- start 11:47:19.68 ===============  To repro directly: =====================================================                                                                                                                           
 pushd D:\git\corefx\bin\tests\AssemblyResolveTests\netcoreapp-Windows_NT-Debug-x64\                                                                                                                                                         
 call D:\git\corefx\bin\testhost\netcoreapp-Windows_NT-Debug-x64\\dotnet.exe xunit.console.dll AssemblyResolveTests.dll  -xml testResults.xml -notrait category=nonnetcoreapptests -notrait category=nonwindowstests  -notrait category=Oute 
 rLoop -notrait category=failing                                                                                                                                                                                                             
 popd                                                                                                                                                                                                                                        
 ===========================================================================================================                                                                                                                                 
 Error:                                                                                                                                                                                                                                      
   An assembly specified in the application dependencies manifest (Microsoft.NETCore.App.deps.json) was not found:                                                                                                                           
     package: 'runtime.win-x64.Microsoft.NETCore.App', version: '2.1.3'                                                                                                                                                                      
     path: 'runtimes/win-x64/native/CoreConsole.exe'                                                                                                                                                                                         
 ----- end 11:47:19.71 ----- exit code -2147450740 ----------------------------------------------------------                                                                                                                                
:\git\corefx\Tools\tests.targets(571,5): warning MSB3073: The command "D:\git\corefx\bin/tests/AssemblyResolveTests/netcoreapp-Windows_NT-Debug-x64//RunTests.cmd D:\git\corefx\bin/testhost/netcoreapp-Windows_NT-Debug-x64/" exited with c 
de -2147450740. [D:\git\corefx\src\System.Runtime.Extensions\tests\AssemblyResolveTests\AssemblyResolveTests.csproj]                                                                                                                         
:\git\corefx\Tools\tests.targets(579,5): error : One or more tests failed while running tests from 'AssemblyResolveTests' please check D:\git\corefx\bin/tests/AssemblyResolveTests/netcoreapp-Windows_NT-Debug-x64/testResults.xml for deta 
ls! [D:\git\corefx\src\System.Runtime.Extensions\tests\AssemblyResolveTests\AssemblyResolveTests.csproj]                                                                                                                                     
 PerfRunner -> D:\git\corefx\bin\AnyOS.AnyCPU.Debug\PerfRunner\netstandard\PerfRunner.exe                                                                                                                                                    
 System.Runtime.Extensions.Performance.Tests -> D:\git\corefx\bin\AnyOS.AnyCPU.Debug\System.Runtime.Extensions.Performance.Tests\netstandard\System.Runtime.Extensions.Performance.Tests.dll                                                 
 TestApp -> D:\git\corefx\bin\AnyOS.AnyCPU.Debug\TestApp\netstandard\TestApp.exe                                                                                                                                                             
 VoidMainWithExitCodeApp -> D:\git\corefx\bin\AnyOS.AnyCPU.Debug\VoidMainWithExitCodeApp\netstandard\VoidMainWithExitCodeApp.exe                                                                                                             

What I'm missing?

/cc @JeremyKuhne @jkotas
EDIT: Maybe owner of build/test are @weshaggard @ericstj but I'm not sure, in that case sorry for noise.

@jkotas

This comment has been minimized.

Copy link
Member

commented Nov 3, 2018

Most likely caused by stale intermediate files. Try running clean.cmd.

@MarcoRossignoli

This comment has been minimized.

Copy link
Collaborator

commented Nov 3, 2018

Most likely caused by stale intermediate files. Try running clean.cmd.

@jkotas I ran some tests but no luck...the issue raise only when I'm building with my local clr...I did this steps:
CoreClr side:

git clean -fdx
git checkout master
git fetch --all
git merge upstream/master
git push
build -skiptests -release
...
BUILD: Product binaries are available at D:\git\coreclr\bin\Product\Windows_NT.x64.Release

CoreFx side:

git clean -fdx
git checkout master
git fetch --all
git merge upstream/master
git push
build.cmd -- /p:CoreCLROverridePath=d:\git\coreclr\bin\Product\Windows_NT.x64.Release\

Thank's in advance for your patience!

@jkotas

This comment has been minimized.

Copy link
Member

commented Nov 3, 2018

@MarcoRossignoli It works fine for me (I just had to sync coreclr back to 4410262 - there is a breaking change on its way from coreclr to corefx). No idea why it fails for you.

@jkotas

This comment has been minimized.

Copy link
Member

commented Nov 3, 2018

ReadOnlyMemory<char> TrimEndingDirectorySeparator(ReadOnlyMemory<char> path)

I agree with @JeremyKuhne earlier comment that the Memory<char> overloads on Path should be tackled separately. We would need a whole bunch of them to make the story hang together.

None of the existing APIs on Path support Memory<char>. One-off corner case Path API that takes Memory<char> does not look useful.

@MarcoRossignoli

This comment has been minimized.

Copy link
Collaborator

commented Nov 3, 2018

Ok, I'll remove ROM overload and I'll try again local Clr build with your version and I'll let you know...weird I never had this problem. Thank's!

@MarcoRossignoli

This comment has been minimized.

Copy link
Collaborator

commented Nov 4, 2018

@jkotas maybe I found the issue...but I don't understand why, if I launch test from

D:\git\corefx\src\System.Runtime.Extensions
λ msbuild /t:rebuildandtest /v:m

It fails(seems download some nuget packages related to .net core 3.0)
but if I launch from tests it works

D:\git\corefx\src\System.Runtime.Extensions\tests
λ msbuild /t:rebuildandtest /v:m

However I get some fail for tests i.e. System.IO.Tests.PathTests_Windows.GetFullPath_CommonUnRooted_Windows(path: "C:tmp\\foo\\..", basePath: "C:\\git\\corefx", expected: "C:\\git\\corefx\\tmp") [FAIL] but seems related to expected drive name, I've got all on D

@JeremyKuhne

This comment has been minimized.

Copy link
Member

commented Nov 20, 2018

Based on dotnet/coreclr#20805 (comment) in the PR by @MarcoRossignoli we should probably have enough available that users can build path comparison more successfully.

namespace System.IO
{
    public static class Path
    {
        // Existing approved
        public static string TrimEndingDirectorySeparator(string path);
        public static ReadOnlySpan<char> TrimEndingDirectorySeparator(ReadOnlySpan<char> path);

        // Proposed additions (as previously suggested)
        public static bool EndsInDirectorySeparator(string path);
        public static bool EndsInDirectorySeparator(ReadOnlySpan<char> path);
    }
}

The Trim methods are important as they consider rooting and won't break the semantics of a path. You don't want to trim @"/" to @"" on Unix, nor do you want to trim the separator off of @"C:\" on Windows (which will introduce the current directory).

EndsIn allows easier building of application specific comparison logic. While ultimately we'd like to add richer helper methods for path comparison, it is extremely difficult to make an API that is safe and broadly accurate. How does case sensitivity apply? What about equivalent volume names on Windows? Any such API would also suffer a performance penalty as we'd have to ensure normalized state for all input (i.e. call Path.GetFullPath).

Summary is that EndsIn will always be useful for some users and other helper methods that will remove the need for others is still quite a ways out.

@JeremyKuhne

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

Marking this back to api-ready-for-review so that we revisit (as described in my comment above).

@JeremyKuhne JeremyKuhne added this to the 3.0 milestone Mar 8, 2019

@karelz karelz changed the title Proposal: Make internal directory separator helpers public Make internal directory separator helpers public Mar 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.