Close #32 Searching for a file on the system path

[Bartleby2718]

Notes

Picked up this ticket because it seemed interesting, even though it was not in the 1.7 milestone.

Context

There are a few different suggested approaches in this StackOverflow thread.

1. I went with the most upvoted answer, which seemed most reasonable and straightforward to me. Filing a PR to see if a variation of it works on Linux. I have not found any executables that didn't work with this approach (on Windows 11).
2. Invoking Process.Start on the executable itself (or calling where on Windows, which on Linux) is probably too expensive.
3. There's also a mention of the built-in API PathFindOnPath in shlwapi.dll, but I wasn't sure if it was reliable.

Open Questions

1. Should we use PathFindOnPath, or is searching for PATH suffice?
2. Should we validate the length of the executable?
3. What other test cases do we want?

[Bartleby2718]

Not sure if this part adds any value here and in the other test.

[Bartleby2718]

This changes the behavior, but my understanding is that using dotnet (or dotnet.exe) wouldn't have worked in the first place. Is this still a breaking change?

[madelson]

The proposal wasn't to make this default behavior. This should be opt-in behavior.

When this behavior is enabled, it should behave like a shell would.

[Bartleby2718]

@madelson What is the expected behavior if o => o.SearchSystemPath() was used but the fully qualified path was passed in as the first argument?

[madelson]

What is the expected behavior if o => o.SearchSystemPath() was used but the fully qualified path was passed in as the first argument?

We want to follow the actual behavior on the command line for guidance. I believe that the current directory is searched before the system path, but I could have that backwards.

Certainly a fully-qualified path should work without a search.

There are also relative paths like ./foo.exe or ../bar/foo.exe. I believe these work today (relative to the CWD); they should continue to work just like they do in a shell.

[Bartleby2718]

The user may have included the extension, in which case we shouldn't be adding more.

Given that there may be an executable like myExe.exe.exe, we shouldn't be checking whether an extension already exists in the executable variable.

[madelson]

What is the OS precedence behavior here?

E.g. if I type dotnet.exe and there is a file dotnet.exe and dotnet.exe.exe on my path, which one gets executed?

What if dotnet.exe.exe is in a folder which is higher precedence?

What if there is a file dotnet and dotnet.exe?

We should strive to match the OS behavior exactly, documenting and testing each decision point.

[Bartleby2718]

@madelson The test failures don't seem related to the updated code path, so requesting review!

[Bartleby2718]

@madelson Any feedback here?

[madelson]

By checking File.Exists, that means we'll skip a directory that matches. Is that the OS behavior?

For example, I type dotnet and there is a directory dotnet or dotnet.exe. What does the OS do?

[madelson]

Looks like a good start! We need to be super careful implementing this to match the OS behavior. I don't want something that kinda-sorta matches.

We also need to make this an opt-in behavior that can be enabled via an option. Something like:

o => o.SearchSystemPath()

[Bartleby2718]

Note to self:

Windows

• Running echo %PATHEXT% in command line prompt returns .COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC.
• For each of the 11 extensions on Windows, create a echoext file that prints out just the extension (e.g. echoext.exe should print out exe when executed; echoext.bat would print out bat)
  1. com: couldn't create one
  2. exe: created a .NET console app

// dotnet new -f net8.0
// dotnet publish -r win-x64 -p:PublishTrimmed=true -p:PublishSingleFile=true
Console.WriteLine("exe");

3. bat (batch)

@echo off
echo bat

4. cmd (command)

@echo cmd

5. vbs (VBScript)

WScript.Echo "vbs"

6. vbe (VBScript Encoded)
7. js (JScript)

WScript.Echo("js");

8. jse (JScript Encoded)
9. wsf (Windows Script File)
10. wsh (Windows Script Host)
11. msc (Management Saved Console)

• I confirmed that the precedence matches the environment variable PATHEXT, at least for the first five. For example, if echoext.com is not present, echoext.exe is executed when I ran echoext, meaning .exe takes precedence over all other executable files. Also, if I manually update the environment variable PATHEXT and start a new command line prompt, then the updated precedence was used. I think this is enough to conclude that the order of the extensions in PATHEXT is the source of truth.

[Bartleby2718]

@madelson I'm a bit confused. I just ran the following script on LINQPad 7, it worked as expected.

async Task Main()
{
	var command = Command.Run("dotnet", new[] { "tool", "list", "-g" });
	await command.Task;
	command.GetOutputAndErrorLines().Dump();
}

dotnet --version or git --version also worked, which suggests that this "feature" is already there in MedallionShell. Do you have an example that shows this feature isn't there yet in MedallionShell?

[Bartleby2718]

Never mind; I see that npm --version works on command line prompt, git bash, and powershell on my Windows machine, but Command.Run does fail.

[Bartleby2718]

@madelson Should we continue to test against Mono? I went down the rabbit hole of testing my changes on AppVeyor and concluded that this is the question I need to answer.

Context

1. File.GetUnixFileMode was added in .NET 7, so I started targeting net8.0 (to simplify the implementation) on top of net6.0 in MedallionShell.csproj and MedallionShell.Tests.csproj.
2. We use nunit-console to test against Mono on AppVeyor.
3. nunit-console isn't being actively maintained due to the lack of volunteers.
4. Consequences:

• The latest version 3.17.0 (branched off from 3.15, as opposed to 3.16) doesn't seem to work on Mono.
• 3.16.3 doesn't support net8.0.
  • We'll eventually be targeting net8.0 and beyond (even if it weren't for my changes).

5. I tried to fix nunit-console 3.17.0 by porting the same changes in 3.16, but it's been tricky because the two versions have diverged quite a bit.

• I think I may be able to get it to work with net462, but I'm not sure if Mono 4 should be compatible with net6.0 or net8.0

The reason we're thinking about this in the first place is that we use nunit-console (presumably because dotnet test is not enough).

I also realized .NET 6 may not be compatible with Mono, which made me wonder if we really need to support Mono use case in the first place.

[madelson]

dotnet --version or git --version also worked, which suggests that this "feature" is already there in MedallionShell. Do you have an example that shows this feature isn't there yet in MedallionShell?

Never mind; I see that npm --version works on command line prompt, git bash, and powershell on my Windows machine, but Command.Run does fail.

I've also observed things like this, and have never gone through the process of tracking down exactly what is happening with the built-in behavior. However, figuring that out is pretty essential for building this feature (and part of why I hadn't built it yet was because of all the nuance and complexity around that!).

[Bartleby2718]

@madelson Thanks for the input. What do you think about the Mono comment above then?

[madelson]

Should we continue to test against Mono? I went down the rabbit hole of testing my changes on AppVeyor and concluded that this is the question I need to answer.

@Bartleby2718 Thanks for the detailed investigation on this. Mono is still a thing because it is used as the runtime for Blazor and I maybe for Xamarin. That said, I don't think we were actually testing against that version of Mono.

Originally, the key reason for Mono testing was really about Linux testing. I wouldn't want to lose that since cross-platform bugs are a serious potential issue for a library like this.

If we remove Mono testing, I'd want to take a serious look at any remaining Mono code forks and ask whether we can remove them since I want to keep the number of untested code branches to an absolute minimum, ideally zero. Looking at the code, it seems like we only branch on Mono for a small number of things, one of which seems related to #117.

If we're going to invest in cleaning up the Mono support and replacing it with robust .NET 8 support, I'd want that to happen in a separate PR.