Fix issue 60: confusing error messages in rename/createLink/createSymbolicLink

[luispedro]

This fixes #60. It builds on #65 (I cherry-picked the commit), while addressing the issues raised there.

[hs-viktor]

Please also rebase on master, so we run this through the most recent CI incarnation.

[luispedro]

Previous issues have been address and the branch has been rebased against main. CI looks good too.

[luispedro]

I'm so sorry, I really thought I had addressed that. Fixed now (and I will add a test)

[luispedro]

Added a test, which would have caught the previously mistake of creating symlinks when normal links were desired

[hs-viktor]

Actually, I just noticed that similar changes also need to happen in System/Posix/Files/ByteString.hsc. Please make these consistent. There the filenames will be ByteStrings (RawFilePath), and throw errors via System.Posix.ByteString.FilePath.

[hs-viktor]

Actually, I just noticed that similar changes also need to happen in System/Posix/Files/ByteString.hsc. Please make these consistent. There the filenames will be ByteStrings (RawFilePath), and throw errors via System.Posix.ByteString.FilePath.

The new function for two paths should probably be added to that module and its non-bytestring twin.

[luispedro]

Updated the code to use <> & have a signature.

The ByteString module stuff might have to wait until the evening as it seems a bit more involved..

[luispedro]

ByteString module updated as well (including tests)

[hs-viktor]

Very nice. Thanks.

[nh2]

@luispedro The error message created by this is wrong, see new bug #353.

[vdukhovni]

Actually, this should be:

  throwErrnoPathIfMinus1_ ("createSymbolicLink to " <> name1) name2 (c_symlink s1 s2)

Because we should make a reasonable effort to record the file location (source of the symlink) where the error occurred, that can be as or more useful than the descriptive string.

Example results for comparison:

λ> do { tryIOError (createSymbolicLink "/foo" "/bar") ; throwErrnoPathIfMinus1_ "createSymlink to /foo" "/bar" (pure (-1)) }
*** Exception: /bar: createSymlink to /foo: permission denied (Permission denied)

λ> do { tryIOError (createSymbolicLink "/foo" "/bar") ; throwErrnoIfMinus1_ "createSymlink /bar to /foo" (pure (-1)) }
*** Exception: createSymlink /bar to /foo: permission denied (Permission denied)

This is different from the rename case where either of the paths could be the source of the problem, but one could make a case for the source path as a sensible default, and consider also recording that.

[nh2]

@vdukhovni I don't fully understand what you're saying: Isn't this simply slightly rewording the same sentence?

(I have nothing against the wording, it's good as well, but "we should make a reasonable effort to record the file location (source of the symlink)" is true either way, isn't it?)

[vdukhovni]

@vdukhovni I don't fully understand what you're saying: Isn't this simply slightly rewording the same sentence?

(I have nothing against the wording, it's good as well, but "we should make a reasonable effort to record the file location (source of the symlink)" is true either way, isn't it?)

My issue isn't with how the resulting error is rendered by its Show method, but rather with its structure as an IOException.
See below:

λ> :set -XLambdaCase 
λ> import GHC.IO.Exception 
λ> import Control.Exception
λ> import  GHC.Internal.Foreign.C.Error 
λ> import System.Posix.Files
λ> import System.IO.Error 
λ> handler :: IOException -> Maybe IOException; handler = Just
λ> eperm :: IO (Either IOException ()); eperm = try $ createSymbolicLink "/foo" "/bar"
λ> raisePath = throwErrnoPathIfMinus1_ "createSymlink to /foo" "/bar" $ pure (-1)
λ>
λ> tryJust handler (createSymbolicLink "/foo" "/bar") >>= \ case { Left e -> pure $ ioe_filename e; Right _ -> pure (Just "no error") }
Nothing
λ> tryJust handler (eperm >> raisePath) >>= \ case { Left e -> pure $ ioe_filename e; Right _ -> pure (Just "no error") }
Just "/bar"

The difference is in the ice_filename element of the IOException:

λ> :info IOException
type IOException :: *
data IOException
  = IOError {ioe_handle :: Maybe GHC.Internal.IO.Handle.Types.Handle,
             ioe_type :: IOErrorType,
             ioe_location :: String,
             ioe_description :: String,
             ioe_errno :: Maybe GHC.Internal.Foreign.C.Types.CInt,
             ioe_filename :: Maybe FilePath}
        -- Defined in ‘GHC.Internal.IO.Exception’

By using throwErrnoPathIfMinus1_ we do a better job of populating the IOException, giving a non-vacuous ioe_filename field. We can examine all the fields and observe the difference in context, showing just ice_filename not populated (also with the current code the two names swapped per your correct bug report).

λ> tryJust handler (eperm >> raisePath) >>= \ case { Left e -> pure . Just $ (,,,,,) <$> ioe_handle <*> ioe_type <*> ioe_location <*> ioe_description <*> ioe_errno <*> ioe_filename $ e; _ -> pure Nothing }
Just (Nothing,permission denied,"createSymlink to /foo","Permission denied",Just 13,Just "/bar")
λ> 
λ> tryJust handler (createSymbolicLink "/foo" "/bar") >>= \ case { Left e -> pure . Just $ (,,,,,) <$> ioe_handle <*> ioe_type <*> ioe_location <*> ioe_description <*> ioe_errno <*> ioe_filename $ e; _ -> pure Nothing }
Just (Nothing,permission denied,"createSymbolicLink '/foo' to '/bar'","Permission denied",Just 13,Nothing)

[nh2]

@vdukhovni Thanks for the example!

I agree, we definitely should populate ioe_filename as you say.