commercialhaskell / rio

A standard library for Haskell
Other
838 stars 54 forks source link

Fix #205 On Windows, findExecutable uses ProcessContext consistently #206

Closed mpilgrem closed 4 years ago

mpilgrem commented 4 years ago

The findExecutable name = limb of RIO.Process.findExecutable no longer filters by the System.Directory.executable test and, so, takes into account fully the list of executable extensions in the ProcessContext.

This also extends the Haddock documentation for ProcessContext, mkProcessContext, mkDefaultProcessContext and findExecutable.

On Windows, an excutable with a filename with no extension cannot be executed from the command line. mkProcessContext and findExecutable are modified accordingly. mkProcessContext now excludes "" from the head of the list of extensions and the limbs of findExecutable use a isWindows && FP.hasExtension test to decide whether to include the name itself in the list of names tried.

The isWindows function is moved from a where to be a generally-available helper function.

mpilgrem commented 4 years ago

I have made changes to this request (and updated the commit message to reflect them) but I need to do some testing on Windows and non-Windows machines. I will comment further when I have completed that.

The review comments about an exported identifier allowing the user to override the relevant part of a ProcessContext made me realise that it would be helpful, at least, to export an exeExtensions utility that was the ProcessContext equivalent of System.Directory.exeExtension. Users would have a way to know what extensions, part of the ProcessContext, were affecting the behaviour of findExecutable.

mpilgrem commented 4 years ago

I have tested successfully on Windows and non-Windows (macOS). The details of my testing are set out below.

On Windows (version 10.0.18362.535):

I confirmed the behaviour of commands at the Windows' Command Prompt, as follows:

>REM Create a small sample application, myProg.exe
>echo main = putStrLn "Hello World!" > myProg.hs
>stack exec ghc -- myProg.hs
[1 of 1] Compiling Main             ( myProg.hs, myProg.o )
Linking myProg.exe ...

>myProg
Hello World!

>rename myProg.exe myProg
>myProg
'myProg' is not recognized as an internal or external command,
operable program or batch file.

>rename myProg myProg.app
>myProg.app
Hello World!

The same behaviour occurs if a path is given (eg >mydir\myProg.app) or if the file is on the PATH.

I tested findExecutable with a simple application, rioTest.exe based on:

module Main (main) where

import System.Environment (getArgs)

import RIO
import RIO.Process

main :: IO ()
main = test =<< (head <$> getArgs)

test :: String -> IO ()
test name = runSimpleApp $ do
  result <- findExecutable name
  liftIO $ case result of
    Left err -> print err
    Right p -> print p

I tested the following combinations of file names/locations and arguments to findExecutable:

File rioTest arg Result Pass?
myProg.exe myProg Located Y
myProg myProg Executable named myProg not found on path: Y
myProg.app myProg.app Located Y
mydir\myProg.exe mydir\myProg Located Y
mydir\myProg mydir\myProg Did not find executable at specified path: Y
mydir\myProg.app mydir\myProg.app Located Y
PATH\myProg.exe myProg Located Y
PATH\myProg.exe myProg.exe Located Y
PATH\myProg myProg Executable named myProg not found on path: Y
PATH\myProg.app myProg.app Located Y
PATH\mydir\myProg.exe mydir\myProg Did not find executable at specified path: Y

On non-Windows (macOS version 10.13.6)

I created a small sample executable, as for Windows above (echo 'main = putStrLn "Hello World!"' > myProg.hs) and a non-executable file test (touch test) which had the same name as an executable on the PATH; and tested with the same rioTest executable, as for Windows above.

On macOS, . is not on the PATH in Terminal by default and, for non-Windows, the (original and current) mkProcessContext does not add "." to the head of pcPath. So, I added . to the head of the PATH (PATH=.:$PATH).

I tested the following combinations of file names/locations and arguments to findExecutable:

File rioTest arg Result Pass?
myProg (. not on PATH) myProg Executable named myProg not found on path: Y
myProg (. now on PATH) myProg Located Y
myProg.exe myProg Executable named myProg not found on path: Y
myProg.exe myProg.exe Located Y
test test Non-execuatable ignored and test command elsewhere on the path located Y
myProg.hs myProg.hs Executable named myProg.hs not found on path: Y
mydir/myProg mydir/myProg Located Y
mydir/myProg.exe mydir/myProg.exe Located Y
mydir/myProg.exe mydir/myProg Did not find executable at specified path: mydir/myProg Y
mydir/test mydir/test Did not find executable at specified path: mdir/test Y
PATH/myProg (other than ./myProg already tested above) myProg Located Y
mpilgrem commented 4 years ago

Subject to the discussion of where/how to document the behaviour of mkProcessContext on Windows when PATHEXT is absent from the environment, I think the revised pull request reflects the most recent review. In particular, I have refactored findExecutable to remove the code duplication (and updated the commit message accordingly). I have reperformed the tests that I describe above, on Windows and non-Windows.