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

Close #32 Searching for a file on the system path #111

Open
wants to merge 6 commits into
base: release-1.7
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions MedallionShell.Tests/GeneralTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,53 @@ namespace Medallion.Shell.Tests

public class GeneralTest
{
[Platform("Win", Reason = "Tests Windows-specific executables")]
[TestCase("dotnet", @"C:\Program Files\dotnet\dotnet.exe")]
[TestCase("dotnet.exe", @"C:\Program Files\dotnet\dotnet.exe")]
[TestCase("where.exe", @"C:\Windows\System32\where.exe")]
[TestCase("cmd", @"C:\Windows\System32\cmd.exe")]
[TestCase("cmd.exe", @"C:\Windows\System32\cmd.exe")]
[TestCase("powershell.exe", @"C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe")]
[TestCase("explorer.exe", @"C:\Windows\explorer.exe")]
[TestCase("git.exe", @"C:\Program Files\Git\cmd\git.exe")]
[TestCase("does.not.exist", null)]
// echo is not a program on Windows but an internal command in cmd.exe or powershell.exe.
// However, things like git may still install echo (e.g. C:\Program Files\Git\usr\bin\echo.EXE)
// so there's no guarantee for echo on Windows.
public void TestGetFullPathOnWindows(string executable, string? expected)
{
StringAssert.AreEqualIgnoringCase(expected, Shell.GetFullPathUsingSystemPathOrDefault(executable));
var command = Command.Run("where", executable);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

command.StandardOutput.ReadToEnd().Trim().ShouldEqual(
expected ?? string.Empty,
$"Exit code: {command.Result.ExitCode}, StdErr: '{command.Result.StandardError}'");
}

[Platform("Unix", Reason = "Tests Unix-specific executables")]
[TestCase("dotnet", "/usr/bin/dotnet")]
[TestCase("which", "/usr/bin/which")]
[TestCase("head", "/usr/bin/head")]
[TestCase("sh", "/bin/sh")]
[TestCase("ls", "/bin/ls")]
[TestCase("grep", "/bin/grep")]
[TestCase("sleep", "/bin/sleep")]
[TestCase("echo", "/bin/echo")]
[TestCase("does.not.exist", null)]
public void TestGetFullPathOnLinux(string executable, string? expected)
{
Shell.GetFullPathUsingSystemPathOrDefault(executable).ShouldEqual(expected);
var command = Command.Run("which", executable);
command.StandardOutput.ReadToEnd().Trim().ShouldEqual(
expected ?? string.Empty,
$"Exit code: {command.Result.ExitCode}, StdErr: '{command.Result.StandardError}'");
}

[Test]
public void TestCommandWithoutFullyQualifiedPath()
{
Assert.That(TestShell.Run("git", "--version").StandardOutput.ReadToEnd(), Does.StartWith("git version"));
Bartleby2718 marked this conversation as resolved.
Show resolved Hide resolved
}

[Test]
public void TestGrep()
{
Expand Down
27 changes: 26 additions & 1 deletion MedallionShell/Shell.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.IO;
using System.Linq;
using System.Runtime.InteropServices;
using System.Text;
using System.Threading;
using System.Threading.Tasks;
Expand Down Expand Up @@ -36,12 +38,17 @@ public Command Run(string executable, IEnumerable<object>? arguments = null, Act
{
Throw.If(string.IsNullOrEmpty(executable), "executable is required");

var executablePath = !executable.Contains(Path.DirectorySeparatorChar)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

&& GetFullPathUsingSystemPathOrDefault(executable) is { } fullPath
? fullPath
: executable;

var finalOptions = this.GetOptions(options);

var processStartInfo = new ProcessStartInfo
{
CreateNoWindow = true,
FileName = executable,
FileName = executablePath,
RedirectStandardError = true,
RedirectStandardInput = true,
RedirectStandardOutput = true,
Expand Down Expand Up @@ -76,6 +83,24 @@ public Command Run(string executable, IEnumerable<object>? arguments = null, Act
return command;
}

// internal for testing
internal static string? GetFullPathUsingSystemPathOrDefault(string executable)
Bartleby2718 marked this conversation as resolved.
Show resolved Hide resolved
{
var paths = Environment.GetEnvironmentVariable("PATH")!.Split(Path.PathSeparator);
Bartleby2718 marked this conversation as resolved.
Show resolved Hide resolved

if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
var pathExtensions = Environment.GetEnvironmentVariable("PATHEXT")!
Bartleby2718 marked this conversation as resolved.
Show resolved Hide resolved
.Split(Path.PathSeparator)
.Concat(new[] { string.Empty })
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.ToArray();
return paths.SelectMany(path => pathExtensions.Select(pathExtension => Path.Combine(path, executable + pathExtension)))
.FirstOrDefault(File.Exists);
Bartleby2718 marked this conversation as resolved.
Show resolved Hide resolved
}

return paths.Select(path => Path.Combine(path, executable)).FirstOrDefault(File.Exists);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}

private static void PopulateArguments(ProcessStartInfo processStartInfo, IEnumerable<object?>? arguments, Options options)
{
if (arguments is null) { return; }
Expand Down