jfischoff / tmp-postgres

Create temporary postgres instances
BSD 3-Clause "New" or "Revised" License
53 stars 18 forks source link

Automatically add Debian paths? #269

Open tomjaguarpaw opened 4 years ago

tomjaguarpaw commented 4 years ago

Using tmp-postgres with Debian is a bit awkward because initdb is not on the default PATH. What do you think about adding a feature that can add the paths at which initdb is typically found to the PATH? I've been using the following, but there are many possibilities.

-- Adds paths to PATH, runs the action and restores the original PATH
-- before returning.
withPath :: [String] -> IO a -> IO a
withPath paths f = bracket
  (do
      origPath <- System.Environment.lookupEnv "PATH"
      let newPath = maybe ps (++ ":" ++ ps) origPath
      System.Environment.setEnv "PATH" newPath
      pure origPath)
  (\origPath -> do
      let origPath' = maybe "" id origPath
      System.Environment.setEnv "PATH" origPath')
  (\_ -> f)

  where ps = intercalate ":" paths

-- withConfig, but adds to the PATH some typical locations that initdb
-- might be found (Debian doesn't put initdb on the PATH).
withConfigPath :: T.Config -> (T.DB -> IO a) -> IO (Either T.StartError a)
withConfigPath config f = withPath [ "/usr/lib/postgresql/11/bin"
                                   , "/usr/lib/postgresql/10/bin" ]
                                   (T.withConfig config f)
jfischoff commented 4 years ago

I tend to just put it on the PATH (like I did for the CI in this repo) or use PATH=/usr/lib/postgresql/11/bin stack test.

I guess I am not convinced the proposed solution is easer than those two options. I guess a third would be a wrapper script.

tomjaguarpaw commented 4 years ago

I guess I see a few reasons for this

  1. I don't like manually interacting with environment variables because they're global mutable state
  2. I don't like manually interacting with the shell or YAML files because they are untyped and generally not robust
  3. It would be nice if it simply worked out of the box for Debian users

One solution might be for the Config to have a new field indicating which initdb to use, one of

  1. Find it on PATH (status quo)
  2. Path to a specific initdb
  3. Search for it in well-known locations (including PATH, the Debian locations, maybe some others)

Then with/start could use option 3 but users could override it if necessary in withConfig/startConfig

jfischoff commented 4 years ago

I actually curious if you could do this now by augmenting the initDbConfig's environmentVariables to add the path there. And then maybe have a debianPathConfig :: IO Config types that could modified with other configs. Idk. Just spitballing.

I don't see prefixing the executable with the PATH as bad. If we include this configuration in the code we would have to maintain the paths as they change which would be annoying.

jfischoff commented 4 years ago

Having it working out of the box sounds nice though. No idea how many people gave up on this because it is didn't work immediately.

tomjaguarpaw commented 4 years ago

I actually curious if you could do this now by augmenting the initDbConfig's environmentVariables to add the path there.

This is a bit weird.

So I'm not sure environmentVariables does what it is intended to do.

tomjaguarpaw commented 4 years ago

Having it working out of the box sounds nice though. No idea how many people gave up on this because it is didn't work immediately.

Hard to say, but there is something to be said for the most simple usage of the API being the one that works in the most use cases.

tomjaguarpaw commented 4 years ago

This is a bit weird.

Hmm, that's not right. It's also used in startProcess. But it doesn't work for my purpose because it sets the PATH of the created process, not the PATH in which the executable is looked up.

jfischoff commented 4 years ago

So I'm not sure environmentVariables does what it is intended to do.

Completely possible that I am not remembering how to set it correctly.

Hmm, that's not right. It's also used in startProcess. But it doesn't work for my purpose because it sets the PATH of the created process, not the PATH in which the executable is looked up.

That's what I would think as well. However when there is no PATH there System.Process can't find the executable so it is used to find the executable and for the executables PATH ... or at least that is my guess. I'm not positive. I haven't extensively tested how the System.Process and the PATH works.

jfischoff commented 4 years ago

I think it is a good idea to add the paths by default. You have convinced me.

I would prefer if it was the default and there was a way to override it rather than extend the API with another function.

tomjaguarpaw commented 4 years ago

I think it is a good idea to add the paths by default. You have convinced me.

Great!

I would prefer if it was the default and there was a way to override it rather than extend the API with another function.

Yes, agreed.

phadej commented 2 years ago

One alternative is to add fields to Config for program "names". By default they can be postgres, initdb, and createb, but users could specify absolute paths. That will by pass $PATH issues completely. And will be handy if someone would like to test with different postgres versions.

ParetoOptimalDev commented 2 years ago

Having it working out of the box sounds nice though. No idea how many people gave up on this because it is didn't work immediately.

I have a coworker who gave up for that reason and used their "whatever time" for some other tech debt.