Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

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

Closed
23Skidoo opened this Issue · 19 comments

3 participants

@23Skidoo
Collaborator

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.

@23Skidoo 23Skidoo was assigned
@23Skidoo 23Skidoo referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@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
Collaborator

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
Collaborator

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?

@23Skidoo 23Skidoo referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@23Skidoo 23Skidoo referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@sol

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

@23Skidoo
Collaborator

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
Owner

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
Collaborator

@tibbe OK, I'll fix that.

@23Skidoo 23Skidoo referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@23Skidoo
Collaborator

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
Collaborator

@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
Collaborator

@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
Collaborator

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
Collaborator

@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 ;).

@23Skidoo 23Skidoo closed this in 7dc0a10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.