Add Environment.ProcessPath

[jkotas]

Fixes #40862

[Dotnet-GitSync-Bot]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

Tagging subscribers to this area: @eiriktsarpalis, @jeffhandley
See info in area-owners.md if you want to be subscribed.

[stephentoub]

Is this usable most places Process.GetCurrentProcess().MainModule.FileName; is currently used? If yes, can we add an analyzer for that? Looking at source.dot.net, that pattern is quite common.

[jkotas]

Is this usable most places Process.GetCurrentProcess().MainModule.FileName; is currently used?

Yes, it should be.

If yes, can we add an analyzer for that?

Sounds good to me. Is the best way to do it to copy&paste UseEnvironmentProcessId analyzer and fix up the few places in it? It feels like a lot of code duplication to me, but it seems to be the common pattern in the analyzer repo.

[stephentoub]

Is the best way to do it to copy&paste UseEnvironmentProcessId analyzer and fix up the few places in it?

It's ok for a single analyzer/fixer to support multiple diagnostic IDs. Given that both of these are detecting patterns on Process usage and recommending Environment usage instead, I think it would make sense for the same code to handle both (assuming there is indeed a lot of overlap), albeit triggering different diagnostic IDs. @mavasani can provide alternative guidance if he thinks another approach is better.

[mavasani]

• Single analyzer can report multiple diagnostic IDs, so just adding a new descriptor with a new ID to the analyzer should be fine.
• Single diagnostic ID can also be mapped to multiple different diagnostic messages, if required. This is normally used when the same diagnostic ID with slightly different messages based on the context in which each diagnostic fires will serve the users better. Additionally, we believe user actions such as disabling the diagnostic completely or bumping it to a warning/error would always be desired to work identically for all these cases.

[jkotas]

@CoffeeFlux Do you have any thoughts on the invariants that this API should maintain and how to implement it?

[CoffeeFlux]

Various thoughts on issues raised above:

We should definitely be calling realpath and guaranteeing an absolute path with symlinks resolved. I'm including $ORIGIN and friends under symlinks, not that I think we're likely to be returning them anyway.

Returning null for wasm also seems straightforward, though of some note here is that this only holds true for browser and not necessarily standalone wasm runtimes, should we ever want to support those.

Call the API, rename the executable. call the API again. Should it return the original executable path or the new executable path?

This one seems like a no-brainer to me. The returned value should not change over the course of program execution, so it should return the original executable path. I don't see any value in keeping it up to date with filesystem or cwd changes. The easiest way to implement this is to cache the value after the first call, and we should do that.

The other questions are more interesting.

Change current directory, call the API for the first time. Should it be guaranteed to work?

I personally think it should return something that isn't cwd-sensitive if at all possible, to the point that I'd consider checking args[0] before using cwd-sensitive detection. It might be hard to 100% guarantee this though, so I would consider it best-effort.

Rename the executable, call the API for the first time. Should it return the original executable path or the new executable path?

Once again, returning the original executable seems desirable if possible, and I think this is also the simplest implementation for Windows, OSX, and Linux? I once again lean towards saying this is best-effort rather than a 100% guarantee.

If we decide we want to make that a hard guarantee, I don't see any good alternative to putting it in the runtime and fetching it once during startup. Again, I personally don't think we have to absolutely guarantee that, but if we want to this is the only real option.

Another complication is Android, which does not have an 'executable' in the traditional sense when launching apps normally; everything forks from Zygote and loads the relevant APK. This means /proc/self/exe will actually return /system/bin/app_process{32,64}, which isn't very useful for anyone. Alternatives include the path to the APK, the path to the entry point DSO, the path to the DLL with the main activity, and the package name, but a bunch of those are either weird or not very useful themselves. Maybe we should just accept that the concept of an "application executable path" is moot on Android and return app_process rather than trying to get creative? This is maybe bolstered by the fact that for command-line invocation, something like adb shell /path/to/dotnet app.dll, /proc/self/exe will indeed return the dotnet executable. There are also a lot of ways an Android app can be started, which means a ton of entry points and ton of potential APKs, assemblies, DSOs, and java files, so maybe the simplest option here really is the best since it'll at least be consistent.

Another consideration for /proc/self/exe, from the man page:

If the pathname has been 
unlinked, the symbolic link will contain the string
'(deleted)' appended to the original pathname.

This isn't an issue when trying to open it, because /proc is special and will just give you access directly despite appearing to be a symlink, but this is obviously not desirable for a returned path. I don't know how this interacts with realpath, but we should check to be sure on both glib and musl, and if either of them returns the (deleted) we should be excising that ourselves.

[jkotas]

consider checking args[0] before using cwd-sensitive detection

Do you mean args[0] passed to the C main method? It is not guaranteed to be an absolute path unfortunately.

Returning null for wasm also seems straightforward, though of some note here is that this only holds true for browser

Agree. I should have said browser, the implementation file is *.Browser.cs actually.

Maybe we should just accept that the concept of an "application executable path" is moot on Android and return app_process rather than trying to get creative?

Agree. This API is intentionally policy free and returns the executable name from the OS point of view. There is a second API proposal (#41341) to return the "directory that user wanted" that is computed via some to be determined policy.

we should check to be sure on both glib and musl, and if either of them returns the (deleted)

(deleted) is appended by the /proc file system, glibc and musl do not have much to do with it. Calling realpath on a path with (deleted) appended to it is going to fail unless you got an actual file with (deleted) appended to the name.

So what I am hearing:

• Cache the value to guarantee that this API returns the same path for the process lifetime.
• Avoid sensitivity to cwd as much as possible.
• Do not try to be robust against executable renames and deletes. If the executable is renamed or deleted, the return value of this API is undefined. The exact behavior is OS-specific: The API can return the old name, new name, null or something else (e.g. the old name w/ (deleted) appended to it).

Sounds reasonable? I can behind this.

[CoffeeFlux]

Sorry, what I meant with the glib and musl comment is whether either of them is aware this can happen and handles it inside realpath. I just gave up and checked the implementation and it definitely looks like it will fail, so we'd need to either handle that ourselves or accept that it will be broken.

[am11]

I once again lean towards saying this is best-effort rather than a 100% guarantee.

Agreed, it could work with cache as well. So if we do not get the perfect (real and absolute) path on some platform in some edge-case scenario against the first call, we could still cache the value and use it for the lifetime of application (best effort was made just once in lifetime of app).

[jkotas]

@am11 This should be ready for review if you would like to take a look.

[CoffeeFlux]

Will go through it tomorrow.

[am11]

Thanks @jkotas for this API. I have tested the changes on illumos. I think it is quite fair to return null for non-absolute or other forms of unresolved cases as done here, instead of prepending cwd in fallback path (which is not clear how to even reproduce with .NET hosting). 👍

[jkotas]

I think it is quite fair to return null for non-absolute or other forms of unresolved cases as done here

realpath will resolve the non-absolute paths if possible.

[stephentoub]

Thanks

[CoffeeFlux]

Mostly just some random nits. LGTM, thanks!

[CoffeeFlux]

Nit: I think a normal variable is more appropriate here than a mid-function define, unless there's a good reason for this to be using the preprocessor that I'm missing. Codegen should be identical.

[jkotas]

Opened #42948 on the analyzer. Thank you all for the discussion about the design and codereview feedback.