Runtime: Add Path.Exists -- to replace inefficient FileExists || DirectoryExists

[JeremyKuhne]

We should implement the code for this internally and look at adding a public API for it. Probably Path.Exists(string path).

The internal helper should avoid any sort of path validation/normalization if possible. A bad path, by definition, doesn't exist. At least on the Windows side pre normalizing isn't needed (and wasteful). We should also consider exposing overloads such as Path.Exists(string path, bool normalize) to allow users to skip this as well (or perhaps a config switch).

[JeremyKuhne]

See https://github.com/dotnet/corefx/pull/19716#discussion_r116282222

[ghost]

I would just call it Path.Exists(). It would instantly replace most of my File.Exists() calls, so it'd be nice not to have a unwieldly name like DirectoryOrFileExists().

[kellypleahy]

I started working on this, but I'm wondering. I can test on Windows and Linux, but how best to get tests for WinRT to run? Can I do that on a linux / windows box?

[kellypleahy]

@karelz so.... who should I talk to about how to test the PR I want to do for this one? Specifically, I'm wondering about windows/winRT since I'm doing the changes on linux. I can test on windows as well as I have a machine with windows, but I don't have anything that does winRT.

[kellypleahy]

I would just call it Path.Exists(). It would instantly replace most of my File.Exists() calls, so it'd be nice not to have a unwieldly name like DirectoryOrFileExists().

@AtsushiKan It seems like Path is probably in mscorlib and not CoreFX? I don't see an easy path to add Path.Exists given that the implementation of it would presumably live on FileSystem? Am I missing something? cc @karelz

[kellypleahy]

@stephentoub mentioned wanting to move Path.cs to corefx at some point here: https://github.com/dotnet/coreclr/issues/1104#issuecomment-111193983

I wonder if that has to happen before this can really be done properly as a public API, or is there some magic type-forwarding foo or something else I need to know about to do Path.Exists?

[stephentoub]

@stephentoub mentioned wanting to move Path.cs to corefx at some point here

The Path implementation did move to corefx, and then with all of the .NET Standard 2.0 compatibility work, it moved back.

I wonder if that has to happen before this can really be done properly as a public API

It doesn't. The API can be added in coreclr, and then exposed from the reference assembly and tested in corefx:
https://github.com/dotnet/corefx/blob/master/src/System.Runtime.Extensions/ref/System.Runtime.Extensions.cs#L1155

But before a PR is submitted for any new APIs, we need to run it through the API review process.
https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/api-review-process.md

[kellypleahy]

It doesn't. The API can be added in coreclr, and then exposed from the reference assembly and tested in corefx:
https://github.com/dotnet/corefx/blob/master/src/System.Runtime.Extensions/ref/System.Runtime.Extensions.cs#L1155

OK... I get that an API review would be needed. Now, I just want to understand how this works mechanically. Are you saying that the API would be added and implemented in coreclr and we would only have the tests in corefx, or are you saying the implementation would somehow live in corefx?

I started down the path of implementing the support in the different FileSystem internal classes. However, then I realized I probably can't expose Path.Exists even if I wanted to using those as the implementation, and instead the implementation would need to live in coreclr.

[stephentoub]

@kellypleahy, it can be confusing, I know.

When adding an API, there are potentially three pieces: the implementation (src), the reference assembly (ref), and the tests (tests) (and I say potentially three because for some libraries there is no ref). The latter two live in corefx; the former may also live in corefx, or for very core types may live in coreclr. With Path, the implementation is here in coreclr:
https://github.com/dotnet/coreclr/blob/master/src/mscorlib/shared/System/IO/Path.cs
The reference assembly is here in corefx:
https://github.com/dotnet/corefx/blob/master/src/System.Runtime.Extensions/ref/System.Runtime.Extensions.cs#L1155
and the tests are here in corefx:
https://github.com/dotnet/corefx/blob/master/src/System.Runtime.Extensions/tests/System/IO/PathTests.cs

[karelz]

Here's rough info/notes about adding API into CoreCLR and exposing it in CoreFX: https://github.com/dotnet/corefx/wiki/Contributions#expose-api-across-coreclrcorefx

We will need API review first -- that requires formal API proposal.

@JeremyKuhne please add the "next-steps" info into up-for-grabs bugs in future (see triage rules). Also please use just 1 issue type.

who should I talk to about how to test the PR I want to do for this one? Specifically, I'm wondering about windows/winRT since I'm doing the changes on linux. I can test on windows as well as I have a machine with windows, but I don't have anything that does winRT.

We leave it on developers to decide if they want to test on 1 platform or more platforms locally. It is fine to test on just 1 platform (e.g. Linux) and look at results in CI for the other platforms (Windows, Mac, other Linux distros). I clarified it in new contributor notes "It is ok to test on just 1 platform locally and use CI to test the other platforms. Whatever is more convenient for contributor."
WinRT/UWP is platform which cannot be tested locally today. We are working on making it possible. I hope it will happen in next couple of weeks.

[kellypleahy]

@karelz thanks for the clarification. Looking forward to working more on this stuff in the future and I'm just trying to get my bearings at this point. All this is very helpful. I'll see if there are tips we can distill into your "notes" on the wiki to help answer the same questions I'm having.

[JeremyKuhne]

@kellypleahy Did you just want to work on the new API? Or did you want to fix the existing ones as well? I'm going to create a new issue for the existing improvements, that might be the right way to get started / familiar with the code.

[kellypleahy]

@karelz Thanks for the comments about testing. Another possible recommendation (tell me if I should file an issue). You said...

WinRT/UWP is platform which cannot be tested locally today. We are working on making it possible. I hope it will happen in next couple of weeks.

It would be really nice if there were a way to run targeted tests in your CI pool for when we are working on a PR. For instance, being able to just push to our local branch before we create the PR and ask something to run tests for specific assembly on that branch (so we can iterate quickly if possible). Think of it sorta like the code/build/test cycle that we would use locally, except it can run tests for us with multiple target platforms.

[karelz]

@kellypleahy did you check the dotnet-bot? I think it has capabilities like that today. (use "@dotnet-bot help" to get full list of commands, options, etc.)

[karelz]

BTW: I plan to have link to the static dotnet-bot docs also in our docs -- I just didn't find it yet 😮
Here's the intent: https://github.com/dotnet/corefx/wiki/Contributions#small-change

[kellypleahy]

use "@dotnet-bot help" to get full list of commands, options, etc.

@karelz where should I do that? I assume doing it in the issue isn't recommended, or will it just PM me the info? I just want to make sure before I do that I don't spam anyone :D

[karelz]

People do it regularly in PRs - e.g. https://github.com/dotnet/corefx/pull/20819#issuecomment-307166111.
I don't like the spam either, but until we have static link to the dotnet-bot help source code linked from decent documentation for contributors, that's the best we have and you should feel encouraged to use it.

[karelz]

BTW: I updated the docs to: "@dotnet-bot docs - use command "@dotnet-bot help" in PRs, or search some older one"

[danmosemsft]

Hi @kellypleahy are you planning to take this one? If so I'll assign it. If not that's fine we might have someone else who can.

[kellypleahy]

I think so... but other priorities have been in the way recently. If you have someone already ready to jump on this it might make sense to hand it off. I'm hoping to get back to this in a couple weeks or three.

[danmosemsft]

@kellypleahy OK. @JeremyKuhne this might be a candidate for one of your "team".

[JeremyKuhne]

Taking it for now as it should fit well with my team's current schedule.

[karelz]

@JeremyKuhne @danmosemsft which API shape is the final for API review? The top post is a bit vague ...

[karelz]

Questions from API review:

1. Is FileSystemInfo.Exists better place for the API?
2. Would it return true for named pipes and similar things?
3. Can it be implemented consistently and efficiently on both Windows and Linux? (esp. non-file resources)

[danmosemsft]

@JeremyKuhne note questions above.

[kellypleahy]

My personal opinion is that FileSystemInfo.Exists is the best place for this.

Re: (3) I believe that, yes, it can be implemented in a performant way on all systems, except maybe for the windows RT/UWP platform (not sure about this one).

Re: (2) - I started down the path of making a more general API that would allow you to retrieve the type of a file system entry. I think this is a good thing to have, but I don't think it was ready to be published. That said, I think it is the right "future" for the API, since I often find myself having to resort to using clib / windows API functions to do this stuff, but it might be hard to spec that out at this point. So, I was assuming I'd start by implementing that underlying API (that would be internal/private) and then using that to support FileSystemInfo.Exists properly.

I'm sorry I've gotten too busy to help with this, but I would love to still provide feedback on API design if you'll have me ;)

[danmosemsft]

Past API complete for this one. It is worth pursuing I think. @JeremyKuhne would drive it along.

[JeremyKuhne]

Yes, we should add an API for this. It isn't too difficult, hopefully we can get it in the next release.

[kellypleahy]

This was my work in progress, in case it is of use to someone:

https://github.com/kellypleahy/corefx/tree/feature/19717-path-exists

[JeremyKuhne]

Need to update and put back to review.