Sandbox commands should temporarily add .cabal-sandbox/bin to PATH

[23Skidoo]

Some libraries (e.g. wx) use custom preprocessors during the build process. When such libraries are installed into the sandbox, the build fails because the required executables are missing.

We will need to add a cross-platform setEnv to Compat, because it's missing from System.Environment. We can use an existing implementation by @sol, which he proposed for inclusion in base.

[sol]

BTW: The existing implementation for setEnv from the unix package uses putEnv on some (exotic?) systems, and putEnv is severely broken (see GHC #7342).

[sol]

I haven't looked at any code, but I think if it works to just pass a modified environment when forking the process, then this would be preferable.

[23Skidoo]

I haven't looked at any code, but I think if it works to just pass a modified environment when forking the process, then this would be preferable.

Since we don't have a Shell monad yet, this will be much more intrusive than just calling setEnv once.

[23Skidoo]

BTW: The existing implementation for setEnv from the unix package uses putEnv on some (exotic?) systems

I removed the dependency on unix and am calling setenv directly:

foreign import ccall unsafe "setenv"
   c_setenv :: CString -> CString -> CInt -> IO CInt

Do you know which systems don't support setenv?

[sol]

Hm, not sure. I think the type is also different on some systems.

[23Skidoo]

I think the type is also different on some systems.

This is how setEnv is defined in the unix package:

setEnv :: String -> String -> Bool {-overwrite-} -> IO ()
#ifdef HAVE_SETENV
setEnv key value ovrwrt = do
  withFilePath key $ \ keyP ->
    withFilePath value $ \ valueP ->
      throwErrnoIfMinus1_ "setenv" $
        c_setenv keyP valueP (fromIntegral (fromEnum ovrwrt))

foreign import ccall unsafe "setenv"
   c_setenv :: CString -> CString -> CInt -> IO CInt
#else
setEnv key value True = putEnv (key++"="++value)
setEnv key value False = do
  res <- getEnv key
  case res of
    Just _  -> return ()
    Nothing -> putEnv (key++"="++value)
#endif

[sol]

Yes, I think you are right. Only unsetEnv has a different type on some systems.

[sol]

Here is what Git uses: https://github.com/git/git/blob/master/compat/setenv.c

The only "ugly" thing with this implementation is that it leaks memory, but I think this is of no practical importance.

[tibbe]

I'm a bit uncomfortable adding with adding things to the system path and not removing them again (see my comment on the pull request). What if some non-sandbox code that runs some time later during the cabal process' lifetime executes some other command? That command might now look for binaries in the wrong place.

[23Skidoo]

@tibbe OK, I'll fix that.

[23Skidoo]

What if some non-sandbox code that runs some time later during the cabal process' lifetime executes some other command? That command might now look for binaries in the wrong place.

Fixed.

[23Skidoo]

@sol

Here is what Git uses: https://github.com/git/git/blob/master/compat/setenv.c

Git is LGPL, so, IIUC, we can't just copy their code.

[sol]

I think that code is broken. AFAIK, getSearchPath looks at PATH.

Personally, I'd always write tests to ensure that my code behaves as expected.

[23Skidoo]

@sol

AFAIK, getSearchPath looks at PATH.

Yes.

-- | Get a list of filepaths in the $PATH.
getSearchPath :: IO [FilePath]
getSearchPath = fmap splitSearchPath (getEnv "PATH")

I think that code is broken.

Why?

[sol]

@23Skidoo Oh, sorry. I really missed the delete while skimming over the code. I think you can still trim it down to something like (untested):

withSandboxBinDirOnSearchPath :: FilePath -> IO a -> IO a
withSandboxBinDirOnSearchPath sandboxDir action = bracket addBinDir (setEnv "PATH") (const action)
  where
    addBinDir :: IO ()
    addBinDir = do
      let sandboxBin = sandboxDir </> "bin"
      oldPath <- getEnv "PATH"
      let newPath = sandboxBin ++ (searchPathSeparator:oldPath)
      setEnv "PATH" newPath
      return oldPath

This does not have the exact same semantics, though. The result is different if action modifies the path (which it hopefully does not).

[23Skidoo]

The result is different if action modifies the path.

Yes, that's why I implemented it this way.

[sol]

@23Skidoo But shouldn't that be delete (sandboxDir </> "bin") ? Or did I miss something else?

[23Skidoo]

@sol Nice catch, thanks!

[sol]

@23Skidoo My point was really about testing. Once you have a proper testing infrastructure in place writing a test for that function takes almost no time, guarantees correctness and spares you from wrong accusations ;).