commercialhaskell / rio

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

Windows: RIO.Process.findExecutable ignores PATHEXT and is inconsistent #205

Closed mpilgrem closed 4 years ago

mpilgrem commented 4 years ago

On Windows, RIO.Process.mkProcessContext, and consequently RIO.Process.mkDefaultProcessContext, make use of Windows' environment variable PATHEXT, or default to the list .COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC, to set the process context 'executable' extensions because:

pcExeExtensions =
            if isWindows
                then let pathext = fromMaybe
                           ".COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC"
                           (Map.lookup "PATHEXT" tm)
                      in map T.unpack $ "" : T.splitOn ";" pathext
                else [""]

However, RIO.Process.findExecutable, on the findExecutable name = limb only - not the findExecutable name0 | any FP.isPathSeparator name0 = limb, filters out all extensions other than .bat", ".cmd", ".com", ".exe (or upper case equivalents):

findExecutable
  :: (MonadIO m, MonadReader env m, HasProcessContext env)
  => String            -- ^ Name of executable
  -> m (Either ProcessException FilePath) -- ^ Full path to that executable on success
findExecutable name0 | any FP.isPathSeparator name0 = do
    pc <- view processContextL
    let names0 = map (name0 ++) (pcExeExtensions pc)
        testNames [] = return $ Left $ ExecutableNotFoundAt name0
        testNames (name:names) = do
            exists <- liftIO $ D.doesFileExist name
            if exists
                then do
                    path <- liftIO $ D.canonicalizePath name
                    return $ return path
                else testNames names
    testNames names0
findExecutable name = do
    pc <- view processContextL
    m <- readIORef $ pcExeCache pc
    epath <- case Map.lookup name m of
        Just epath -> return epath
        Nothing -> do
            let loop [] = return $ Left $ ExecutableNotFound name (pcPath pc)
                loop (dir:dirs) = do
                    let fp0 = dir FP.</> name
                        fps0 = map (fp0 ++) (pcExeExtensions pc)
                        testFPs [] = loop dirs
                        testFPs (fp:fps) = do
                            exists <- D.doesFileExist fp
                            existsExec <- if exists then liftM D.executable $ D.getPermissions fp else return False
                            if existsExec
                                then do
                                    fp' <- D.makeAbsolute fp
                                    return $ return fp'
                                else testFPs fps
                    testFPs fps0
            epath <- liftIO $ loop $ pcPath pc
            () <- atomicModifyIORef (pcExeCache pc) $ \m' ->
                (Map.insert name epath m', ())
            return epath
    return epath

because of the existsExec test: D.executable $ D.getPermissions fp makes use of System.Directory.getPermissions and that, for Windows, has (via getAccessPermissions):

getAccessPermissions :: FilePath -> IO Permissions
getAccessPermissions path = do
  m <- getFileMetadata path
  let isDir = fileTypeIsDirectory (fileTypeFromMetadata m)
  let w = hasWriteMode (modeFromMetadata m)
  let x = (toLower <$> takeExtension path)
          `elem` [".bat", ".cmd", ".com", ".exe"]
  pure Permissions
       { readable   = True
       , writable   = w
       , executable = x && not isDir
       , searchable = isDir
       }

A little knowledge is a dangerous thing, but I think RIO.Process.findExecutable should:

(1) also respect the pcExeExtensions in the ProcessContext; and

(2) take a consistent approach to if existsExec (applying it or not) in both of its limbs. Currently, findExecutable "app\\myProg" (first limb) behaves differently to findExecutable "myProg" (second limb) if myProg is an executable with no extension to its file name.

snoyberg commented 4 years ago

Seems reasonable, want to send a PR?

mpilgrem commented 4 years ago

I have proposed pull request #206.